Re: RFR 4823133: RandomAccessFile.length() is not thread-safe

[vyom]

Hi Martin,

thanks for review, i undated the 
webrev(http://cr.openjdk.java.net/~vtewari/4823133/webrev0.2/ 
) as per your 
comment after confirming that the corresponding "fd" if opened with 
"open64".


Thanks,
Vyom


On Thursday 17 December 2015 12:38 AM, Martin Buchholz wrote:

Calling naked fstat without _FILE_OFFSET_BITS=64 is a bug.  Don't you
need to call fstat64 if it's available?

+jlong
+handleGetLength(FD fd)
+{
+struct stat sb;
+if (fstat(fd, ) == 0) {
+return sb.st_size;
+} else {
+return -1;
+}
+}

On Wed, Dec 16, 2015 at 5:02 AM, vyom  wrote:

Hi,

Updated the webrev(http://cr.openjdk.java.net/~vtewari/4823133/webrev0.1/
) as per Martin
review comment removed the _FILE_OFFSET_BITS from source.

Thanks,
Vyom



On Tuesday 15 December 2015 10:55 PM, Martin Buchholz wrote:

_FILE_OFFSET_BITS is generally an all-or-nothing thing, because it
affects interoperability between translation units.  It would be good
to convert all of the JDK build to use -D_FILE_OFFSET_BITS=64, but
that would be a big job.  So traditionally the JDK has instead used
the functions made available via _LARGEFILE64_SOURCE.  But that is
also a JDK-wide job now, because every call to plain stat in the
source code is broken on 32-bit systems with 64-bit inodes, which are
becoming more common.

I recommend the _FILE_OFFSET_BITS=64 strategy, but it's probably a job
for build-dev, not core-libs-dev.




On Tue, Dec 15, 2015 at 8:31 AM, Roger Riggs 
wrote:

Hi Yvom,

Minor comments:

src/java.base/share/native/libjava/RandomAccessFile.c:
   - "length fail" might be clearer as "GetLength failed"

src/java.base/unix/native/libjava/io_util_md.c:

   - Please add a comment before the define of FILE_OFFSET_BITS to
indicate
where it is used and why it is there.
   - BTW, are there any unintended side effects?
 Perhaps a different issue but perhaps 64 bit offsets should be used
everywhere

src/java.base/windows/native/libjava/io_util_md.c
   - Line 592: Using INVALID_HANDLE_VALUE is better than -1 and is used
elsewhere in the file
 BTW, Testing for invalid handle might be unnecessary since the call
to
GetFileSizeEx will fail
 if it is invalid, yielding the same result.

Roger


On 12/10/2015 5:52 AM, vyom wrote:

Hi All,

Please review my changes for below bug.

Bug: JDK-4823133 : RandomAccessFile.length() is not thread-safe

Webrev:http://cr.openjdk.java.net/~vtewari/4823133/webrev0.0/


This change ensure that  length() does not temporarily changes the file
pointer and it will make sure that there is no race
condition in case of multi thread uses.

Thanks,
Vyom

Re: RFR [9] 8141615: Add new public methods to sun.reflect.ConstantPool

[Konstantin Shefov]

Hi Coleen

You have previously reviewed this enhancement and made a few comments
I have resolved them, so could you look at the webrevs again, please?

I have added tests, removed cp tags that are not in JVM spec (100 - 105) 
and made some other changes.


JDK: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.04
HOTSPOT: http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.02

Thanks
-Konstantin


On 12/16/2015 07:42 PM, Christian Thalinger wrote:

Looks good.  Thanks.


On Dec 16, 2015, at 1:13 AM, Konstantin Shefov  
wrote:

Christian

I have fixed the enum so it uses "ENUMENTRY(int)" format now and does linear 
search.
http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.04/

-Konstantin

On 12/15/2015 08:36 PM, Christian Thalinger wrote:

On Dec 14, 2015, at 11:11 PM, Konstantin Shefov > wrote:

Hi Christian

Thanks for reviewing, I have changed indents as you asked:

http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.03 


Thanks.  I’m still not comfortable with the enum.  It would be great if we 
could get the values from the VM like in JVMCI:

http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/c036c7f17e09/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotConstantPool.java#l101

but that would be overkill here.  But I would like to see the enum entries take 
the integer values as arguments, like here:

http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/c036c7f17e09/src/jdk.vm.ci/share/classes/jdk.vm.ci.sparc/src/jdk/vm/ci/sparc/SPARCKind.java#l27

and either do a simple linear search to find the entry or build up a table like 
the HotSpotConstantPool example above.


-Konstantin

On 12/15/2015 06:23 AM, Christian Thalinger wrote:

On Dec 11, 2015, at 1:54 AM, Konstantin Shefov > wrote:

Hello

Please review the new version on the patch.

New webrev:
Webrev hotspot: http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.02 

Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.02 


These newlines look ridiculous especially when it’s not even needed:

+  // Returns a class reference index for a method or a field.
+  public int  getClassRefIndexAt
+ (int index) { return getClassRefIndexAt0 
(constantPoolOop, index); }

Either fix the indent or just add them like regular methods should look like.


What has been changed:
1. Added tests for the new added methods.
2. Removed CP tag codes 100 - 105 from being passed to java and left only codes 
from the open JVM spec 
(https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4-140).

Thanks
-Konstantin

On 11/27/2015 07:48 PM, Konstantin Shefov wrote:

Coleen,
Thanks for review

On 11/24/2015 07:33 PM, Coleen Phillimore wrote:

I have a couple preliminary comments.  First, there are no tests added with all 
this new functionality.  Tests should be added with the functionality 
changeset, not promised afterward.

I will add tests.

Secondly, I do not like that JDK code knows the implementation details of the 
JVM's constant pool tags.  These should be only for internal use.

The package "sun.reflect" is for internal use only, so it shouldn’t be a 
problem.

My third comment is that I haven't looked carefully at this constant pool 
implementation but it seems very unsafe if the class is redefined and relies on 
an implementation detail in the JVM that can change.   I will have more 
comments once I look more at the jvmti specification.

thanks,
Coleen

On 11/24/15 9:48 AM, Konstantin Shefov wrote:

Hello

Please, review modified webrev.

I have added methods
getNameAndTypeRefIndexAt(int index) - to get name and type entry index from a 
method, field or indy entry index;
getClassRefIndexAt(int index) - to get class entry index from a method or field 
entry index;

I removed previously added method
getInvokedynamicRefInfoAt(int index) - as it is no more needed now.

New webrev:
Webrev hotspot: http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.01 

Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.01 


Thanks
-Konstantin

On 11/18/2015 02:11 PM, Konstantin Shefov wrote:

Remi,

Thanks for reviewing. Your suggestion sounds sensibly.
May be it also has sense to make a method "getMethodRefNameAndTypeIndex(int 
index)" to acquire name-and-type entry index for methods also.

-Konstantin

On 11/18/2015 12:04 AM, Remi Forax wrote:

Hi Konstantin,
Technically, getInvokedynamicRefInfoAt should be 
getNameAndTypeOfInvokedynamicRefInfoAt because it only 

Review request for JDK-8145545 : Typos in Javadoc of XmlAdapter

[Abhijit Roy]

Hi all,

 Please review a fix for Bug  - 
https://bugs.openjdk.java.net/browse/JDK-8145545 

Bug - Typos in Javadoc of XmlAdapter

Webrev - http://cr.openjdk.java.net/~ntv/ababroy/8145545/webrev.00/

 I have just rectified and modified those error. And moving forward it for 
review.

 

 

Regards,

Abhijit.

Re: RFR: JDK-8115868: 32-bit JVM failed to start from a large network filesystem

[Kumar Srinivasan]

On 12/16/2015 11:01 AM, Martin Buchholz wrote:

On Tue, Dec 15, 2015 at 2:12 PM, Bernd Eckenfels  wrote:

Hello,

I always like it when access() is used instead of stat() magic.

I noticed that the new ProgramExists in java_md_common.c does not
anymore reject directories (which are typically executable). Not sure
it this matters or is intentional, it is a change in semantic.

Right.  This is a small loss of robustness.  Changing stat into access
here isn't really right because not stat has a superset of
functionality, and (like I keep saying) all the stat calls in the JDK
are potentially broken and need fixing.


I agree this is a very small corner case, the caller (there is one call 
and will always
test for the existence of a file). So I am going to go with this, unless 
I hear objections.


Kumar

Re: [PING] PoC for JDK-4347142: Need method to set Password protection to Zip entries

[Yasumasa Suenaga]

Hi Jason,

Thank you for your comment.
I've fixed it in new webrev:
  http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.03/


Thanks,

Yasumasa


On 2015/12/17 0:33, Jason Mehrens wrote:

The null check of 'entry' at line 351 of ZipFile.getInputStream  is in conflict 
with line 350 and 348.


From: core-libs-dev  on behalf of Yasumasa 
Suenaga 
Sent: Wednesday, December 16, 2015 8:47 AM
To: Sergey Bylokhov; Xueming Shen
Cc: core-libs-dev@openjdk.java.net
Subject: Re: [PING] PoC for JDK-4347142: Need method to set Password
protection to Zip entries

Hi Sergey,

Thank you for your comment.

I added that description in new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.02/


Thanks,

Yasumasa


On 2015/12/16 22:19, Sergey Bylokhov wrote:

Should the new methods describe how they will work in case of null params?

On 16/12/15 16:04, Yasumasa Suenaga wrote:

I adapted this enhancement after JDK-8145260:
http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.01/

Could you review it?


Thanks,

Yasumasa


On 2015/12/12 21:23, Yasumasa Suenaga wrote:

Hi Sherman,

Our proposal is affected by JDK-8142508.
We have to change ZipFile.java and and ZipFile.c .
Thus we will create a new webrev for current (after 8142508) jdk9/dev
repos.

Do you have any comments about current webrev?
http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.00/

If you have comments, we will fix them in new webrev.


Thanks,

Yasumasa


On 2015/12/03 16:51, KUBOTA Yuji wrote:

Hi Sherman,

Thanks for your quick response :)

I aimed to implement the "traditional" at this proposal by the below
reasons.

   * We want to prepare API for encrypted zip files at first.
 * Many people use the "traditional" in problem-free scope like a
temporary file.
   * We do not know which implementation of the "stronger" is best for
openjdk.
 * PKWare claims that they have patents about the "stronger" on
Zip[1].
 * OTOH, WinZip have the alternative implementation of the
"stronger" [2][3].
   * Instead, we prepared the extensibility by ZipCryption interface to
implement other encrypt engine, such as the AES based.

Thus, I think this PoC should support the "traditional" only.
In the future, anyone who want to implement the "stronger" can easily
add their code by virtue of this proposal.

[1] https://pkware.cachefly.net/webdocs/APPNOTE/APPNOTE-6.3.3.TXT
  (1.4 Permitted Use & 7.0 Strong Encryption Specification)
[2]
https://en.wikipedia.org/wiki/Zip_(file_format)#Strong_encryption_controversy

[3] http://www.winzip.com/aes_info.htm

Thanks,
Yuji

2015-12-03 12:29 GMT+09:00 Xueming Shen :


Hi Yuji,

I will take a look at your PoC.  Might need some time and even bring
in the
security guy
to evaluate the proposal. It seems like you are only interested in the
"traditional PKWare
decryption", which is, based on the wiki, "known to be seriously
flawed, and
in particular
is vulnerable to known-plaintext attacks":-) Any request to support
"stronger" encryption
mechanism, such as the AES based?

Regards,
Sherman


On 12/2/15 6:48 PM, KUBOTA Yuji wrote:


Hi all,

We need reviewer(s) for this PoC.
Could you please review this proposal and PoC ?

Thanks,
Yuji

2015-11-26 13:22 GMT+09:00 KUBOTA Yuji :


Hi all,

* Sorry for my mistake. I re-post this mail because I sent before get
a response of subscription confirmation of core-libs-dev.

Our customers have to handle password-protected zip files. However,
Java SE does not provide the APIs to handle it yet, so we must use
third party library so far.

Recently, we found JDK-4347142: "Need method to set Password
protection to Zip entries", and we tried to implement it.

The current zlib in JDK is completely unaffected by this proposal.
The
traditional zip encryption encrypts a data after it is has been
compressed by zlib.[1] So we do NOT need to change existing zlib
implementation.

We've created PoC and uploaded it as webrev:

   http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.00/

   Test code is as below. This code will let you know how this PoC
works.

http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.00/Test.java

In NTT, a Japanese telecommunications company. We are providing many
enterprise systems to customers. Some of them, we need to
implement to
handle password-protected zip file. I guess that this proposal is
desired for many developers and users.

I'm working together with Yasumasa Suenaga, jdk9 committer
(ysuenaga).
We want to implement it if this proposal accepted.

[1]: https://pkware.cachefly.net/webdocs/APPNOTE/APPNOTE-6.3.3.TXT
(6.0  Traditional PKWARE Encryption)

Thanks,
Yuji

RFR: JDK-8145596 Enable debug symbols for all libraries

[Magnus Ihse Bursie]

For historical reasons, the FDS (Full Debug Symbols) project only 
enabled debug symbols for a few select libraries, since this was 
difficult to achieve in the old build. Also, the FDS project never 
enabled debug symbols for macosx on the JDK libraries.


With the new build system, debug symbols come for free for all 
libraries, and we actually have to do extra work to keep them out. :-&


We should just stop doing that. It hurts no-one to have proper debug 
information for all libraries, it might come in helpful, and just doing 
it everywhere would simplify build logic.


This is mainly a build change, but I'm cc:ing the component teams just 
in case.


This patch leverages JDK-8036003, but provides a cleaner implementation 
of this logic in the makefiles. Instead of the vague 
ENABLE_DEBUG_SYMBOLS, I have introduced three clearly defined variables:


COMPILE_WITH_DEBUG_SYMBOLS
COPY_DEBUG_SYMBOLS
ZIP_EXTERNAL_DEBUG_SYMBOLS

The various settings of --with-native-debug-symbols turns these 
variables on/off depending on what we want to achieve, and the makefiles 
check these variables to determine what compiler flags to use, whether 
to run objcopy (or similar) and whether to zip the extracted symbols, 
respectively.


A fourth variable (STRIP_DEBUG_SYMBOLS) is needed to fully complement 
this, but the way we handle stripping is complex in it's own right, and 
I've saved that for a separate patch.


Note that this patch intentionally does not affect the Hotspot build 
system. The variables for the hotspot build is kept unchanged. When the 
new build-infra based hotspot build system arrives, the functionality 
introduced in this patch will be automatically used. Until then, I 
prefer not to mess any more than necessary with the hotspot makefiles.


Bug: https://bugs.openjdk.java.net/browse/JDK-8145596
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8145596-fix-native-debug-symbols-properly/webrev.01


/Magnus

Re: RFR (S) 8145428: Optimize StringUTF16 compress/copy methods for C1

[Ulf Zibis]

Am 17.12.2015 um 08:54 schrieb Aleksey Shipilev:

On 12/17/2015 02:34 AM, Ulf wrote:

I'm wondering why moving the increment operation to an extra line wound
enhance performance.

Because C1 is very straightforward, and code movement like that is a
poor man's instruction scheduling, that pads out the data dependency
between index update and indexed access.


Thanks.
Shouldn't this simple optimization be addressed to javac?
Otherwise programmers always risk some performance when using the post-increment idiom, which IMHO 
is good code style.


-Ulf

RFR [9] 8145589: Test6277246.java fails to compile after JDK-8144479

[Chris Hegarty]

The removal of BASE64Encoder, and a related types, in 8144479 [1]
has triggered the failure of java/beans/Introspector/Test6277246.java.
Another internal type should be used instead of sun.misc.BASE64Encoder.
The sun.security.x509 package seems stable, and is being used in other
areas, like langtools and jigsaw/jake to test accessibility.


diff --git a/test/java/beans/Introspector/Test6277246.java 
b/test/java/beans/Introspector/Test6277246.java
--- a/test/java/beans/Introspector/Test6277246.java
+++ b/test/java/beans/Introspector/Test6277246.java
@@ -25,7 +25,7 @@
  * @test
  * @bug 6277246
  * @summary Tests problem with java.beans use of reflection
- * @modules java.base/sun.misc
+ * @modules java.base/sun.security.x509
  *  java.desktop
  * @run main/othervm Test6277246
  * @author Jeff Nisewanger
@@ -36,11 +36,10 @@
 import java.beans.Introspector;
 import java.beans.MethodDescriptor;
 import java.lang.reflect.Method;
-import sun.misc.BASE64Encoder;
 
 public class Test6277246 {
 public static void main(String[] args) throws IntrospectionException {
-Class type = BASE64Encoder.class;
+Class type = sun.security.x509.X509CertInfo.class;
 System.setSecurityManager(new SecurityManager());
 BeanInfo info = Introspector.getBeanInfo(type);
 for (MethodDescriptor md : info.getMethodDescriptors()) {
@@ -48,7 +47,7 @@
 System.out.println(method);
 
 String name = method.getDeclaringClass().getName();
-if (name.startsWith("sun.misc.")) {
+if (name.startsWith("sun.")) {
 throw new Error("found inaccessible method");
 }
 }

diff --git a/test/ProblemList.txt b/test/ProblemList.txt
--- a/test/ProblemList.txt
+++ b/test/ProblemList.txt
@@ -126,9 +126,6 @@
 java/beans/Introspector/8132566/OverridePropertyInfoTest.java generic-all
 java/beans/Introspector/8132566/OverrideUserDefPropertyInfoTest.java 
generic-all
 
-# 8145589
-java/beans/Introspector/Test6277246.java  generic-all
-
 
 
 # jdk_lang


-Chris.

[1] https://bugs.openjdk.java.net/browse/JDK-8144479

Re: RFR (XS) 8145539: (coll) AbstractMap.keySet and .values should not be volatile

[Aleksey Shipilev]

Hi Peter,

On 12/17/2015 03:19 PM, Peter Levart wrote:
> Wouldn't that change make a possible outcome of keySet() returning null
> in case it was invoked concurrently? Wouldn't you have to use a local
> variable to prevent that?

Ah yes! Silly me. I remember looking at most usages and seeing that
reads are performed only once, but missed that it should be applied
consistently, including the AbstractMap itself.

Fixed that, plus exploded inline assignments in subclasses :
 http://cr.openjdk.java.net/~shade/8145539/webrev.03/

Thanks,
-Aleksey

Re: Review Request 8144553: java/lang/StackWalker/StackWalkTest.java and MultiThreadStackWalk.java fail with stack overflows

[Daniel Fuchs]

Hi Mandy,

I believe it would be good to have some test that go over
the 1024 limit - as this has been useful to detect bugs
when we were actively prototyping the API.

So  maybe we should first try to reduce from 2000 to e.g. 1028?

best regards,

-- daniel

On 16/12/15 21:30, Mandy Chung wrote:

This is a test fix to reduce the stack depth to avoid StackOverFlow when 
running with -Xcomp on solaris sparc.

Webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8144553/webrev.00/index.html

I verified the tests on solaris-sparcv9 machine with fastdebug VM -Xcomp -ea 
-esa.

Mandy

Re: RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to use alternative to finalization

[Roger Riggs]

fyi,  I filed a separate bug[1] to add the reachabilityFence calls when 
they reach jdk-dev.


Roger

[1] https://bugs.openjdk.java.net/browse/JDK-8145459

On 12/17/2015 7:53 AM, Paul Sandoz wrote:

On 17 Dec 2015, at 13:07, Peter Levart  wrote:

I see no other issues left besides the reachability races that I talked about 
in previous messages. Do you know if Reference.reachabilityFence is being 
pushed before this API?


It’s already pushed to hs-comp. I dunno when it will wind it’s way down into 
jdk9/dev (nor vice versa if java.lang.ref.Cleaner is pushed to dev).

Paul.

Re: RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to use alternative to finalization

[Paul Sandoz]

> On 17 Dec 2015, at 13:07, Peter Levart  wrote:
> 
> I see no other issues left besides the reachability races that I talked about 
> in previous messages. Do you know if Reference.reachabilityFence is being 
> pushed before this API?
> 

It’s already pushed to hs-comp. I dunno when it will wind it’s way down into 
jdk9/dev (nor vice versa if java.lang.ref.Cleaner is pushed to dev).

Paul.

Re: RFR [9] 8145589: Test6277246.java fails to compile after JDK-8144479

[Roger Riggs]

Hi Chris,

Looks fine.

Roger


On 12/17/2015 9:46 AM, Chris Hegarty wrote:

The removal of BASE64Encoder, and a related types, in 8144479 [1]
has triggered the failure of java/beans/Introspector/Test6277246.java.
Another internal type should be used instead of sun.misc.BASE64Encoder.
The sun.security.x509 package seems stable, and is being used in other
areas, like langtools and jigsaw/jake to test accessibility.


diff --git a/test/java/beans/Introspector/Test6277246.java 
b/test/java/beans/Introspector/Test6277246.java
--- a/test/java/beans/Introspector/Test6277246.java
+++ b/test/java/beans/Introspector/Test6277246.java
@@ -25,7 +25,7 @@
   * @test
   * @bug 6277246
   * @summary Tests problem with java.beans use of reflection
- * @modules java.base/sun.misc
+ * @modules java.base/sun.security.x509
   *  java.desktop
   * @run main/othervm Test6277246
   * @author Jeff Nisewanger
@@ -36,11 +36,10 @@
  import java.beans.Introspector;
  import java.beans.MethodDescriptor;
  import java.lang.reflect.Method;
-import sun.misc.BASE64Encoder;
  
  public class Test6277246 {

  public static void main(String[] args) throws IntrospectionException {
-Class type = BASE64Encoder.class;
+Class type = sun.security.x509.X509CertInfo.class;
  System.setSecurityManager(new SecurityManager());
  BeanInfo info = Introspector.getBeanInfo(type);
  for (MethodDescriptor md : info.getMethodDescriptors()) {
@@ -48,7 +47,7 @@
  System.out.println(method);
  
  String name = method.getDeclaringClass().getName();

-if (name.startsWith("sun.misc.")) {
+if (name.startsWith("sun.")) {
  throw new Error("found inaccessible method");
  }
  }

diff --git a/test/ProblemList.txt b/test/ProblemList.txt
--- a/test/ProblemList.txt
+++ b/test/ProblemList.txt
@@ -126,9 +126,6 @@
  java/beans/Introspector/8132566/OverridePropertyInfoTest.java generic-all
  java/beans/Introspector/8132566/OverrideUserDefPropertyInfoTest.java 
generic-all
  
-# 8145589

-java/beans/Introspector/Test6277246.java  generic-all
-
  
  
  # jdk_lang



-Chris.

[1] https://bugs.openjdk.java.net/browse/JDK-8144479

Re: RFR: JDK-8145596 Enable debug symbols for all libraries

[Erik Joelsson]

Looks good to me.

Note that this patch will conflict with the quick fix I did in jdk9/hs, 
so you will need to either wait for my patch, push in a forest that has 
my patch, or make sure the integrator can handle it correctly.


/Erik

On 2015-12-17 14:43, Magnus Ihse Bursie wrote:
For historical reasons, the FDS (Full Debug Symbols) project only 
enabled debug symbols for a few select libraries, since this was 
difficult to achieve in the old build. Also, the FDS project never 
enabled debug symbols for macosx on the JDK libraries.


With the new build system, debug symbols come for free for all 
libraries, and we actually have to do extra work to keep them out. :-&


We should just stop doing that. It hurts no-one to have proper debug 
information for all libraries, it might come in helpful, and just 
doing it everywhere would simplify build logic.


This is mainly a build change, but I'm cc:ing the component teams just 
in case.


This patch leverages JDK-8036003, but provides a cleaner 
implementation of this logic in the makefiles. Instead of the vague 
ENABLE_DEBUG_SYMBOLS, I have introduced three clearly defined variables:


COMPILE_WITH_DEBUG_SYMBOLS
COPY_DEBUG_SYMBOLS
ZIP_EXTERNAL_DEBUG_SYMBOLS

The various settings of --with-native-debug-symbols turns these 
variables on/off depending on what we want to achieve, and the 
makefiles check these variables to determine what compiler flags to 
use, whether to run objcopy (or similar) and whether to zip the 
extracted symbols, respectively.


A fourth variable (STRIP_DEBUG_SYMBOLS) is needed to fully complement 
this, but the way we handle stripping is complex in it's own right, 
and I've saved that for a separate patch.


Note that this patch intentionally does not affect the Hotspot build 
system. The variables for the hotspot build is kept unchanged. When 
the new build-infra based hotspot build system arrives, the 
functionality introduced in this patch will be automatically used. 
Until then, I prefer not to mess any more than necessary with the 
hotspot makefiles.


Bug: https://bugs.openjdk.java.net/browse/JDK-8145596
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8145596-fix-native-debug-symbols-properly/webrev.01


/Magnus

Re: RFR (S) 8145428: Optimize StringUTF16 compress/copy methods for C1

[Roger Riggs]

Hi Alexsey,

The 'expected to run benchmarks' might the operative comment in the code.
'Common' knowledge sometimes isn't so common.

Roger


On 12/17/2015 2:54 AM, Aleksey Shipilev wrote:

On 12/17/2015 02:34 AM, Ulf wrote:

I'm wondering why moving the increment operation to an extra line wound
enhance performance.

Because C1 is very straightforward, and code movement like that is a
poor man's instruction scheduling, that pads out the data dependency
between index update and indexed access. I don't think it deserves a
comment -- it is expected one will run the benchmarks when changing that
code.

Thanks,
-Aleksey

RFR: 8145686: SimpleConsoleLogger and LogRecord should take advantage of StackWalker to skip classes implementing System.Logger

[Daniel Fuchs]

Hi,

Please find below a small enhancement for the method that infers
the calling class name and method name in  SimpleConsoleLogger
and LogRecord.

The idea is that since these methods now use the new StackWalker API
they should take advantage of that in order to skip classes that
implement System.Logger. This should make it possible for components
that need it to easily wrap a System.Logger (and even a j.u.l.Logger).

8145686: SimpleConsoleLogger and LogRecord should take advantage
 of StackWalker to skip classes implementing System.Logger
https://bugs.openjdk.java.net/browse/JDK-8145686

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8145686/webrev.00/

best regards,

-- daniel

Re: RFR [9] 8145589: Test6277246.java fails to compile after JDK-8144479

[Chris Hegarty]

On 17 Dec 2015, at 14:54, Roger Riggs  wrote:

> Hi Chris,
> 
> Looks fine.

Thanks Roger. Just a little addition to this review.

While not strictly necessary, I’d like to clean up another, string,
reference use in a reflective call. ( It is not causing a failure as
the security manager will restrict access to sun.*, but it may 
avoid confusion in the future.

diff --git a/test/java/beans/EventHandler/Test6277246.java 
b/test/java/beans/EventHandler/Test6277246.java
--- a/test/java/beans/EventHandler/Test6277246.java
+++ b/test/java/beans/EventHandler/Test6277246.java
@@ -39,7 +39,7 @@
 Class container = Class.forName("java.lang.Class");
 Class parameter = Class.forName("java.lang.String");
 Method method = container.getMethod("forName", parameter);
-Object[] arglist = new Object[] {"sun.misc.BASE64Encoder"};
+Object[] arglist = new Object[] {"sun.security.x509.X509CertInfo"};
 EventHandler eh = new EventHandler(Test6277246.class, "forName", 
"", "forName");
 Object object = eh.invoke(null, method, arglist);
 throw new Error((object != null) ? "test failure" : "test error”);

-Chris.

> Roger
> 
> 
> On 12/17/2015 9:46 AM, Chris Hegarty wrote:
>> The removal of BASE64Encoder, and a related types, in 8144479 [1]
>> has triggered the failure of java/beans/Introspector/Test6277246.java.
>> Another internal type should be used instead of sun.misc.BASE64Encoder.
>> The sun.security.x509 package seems stable, and is being used in other
>> areas, like langtools and jigsaw/jake to test accessibility.
>> 
>> 
>> diff --git a/test/java/beans/Introspector/Test6277246.java 
>> b/test/java/beans/Introspector/Test6277246.java
>> --- a/test/java/beans/Introspector/Test6277246.java
>> +++ b/test/java/beans/Introspector/Test6277246.java
>> @@ -25,7 +25,7 @@
>>   * @test
>>   * @bug 6277246
>>   * @summary Tests problem with java.beans use of reflection
>> - * @modules java.base/sun.misc
>> + * @modules java.base/sun.security.x509
>>   *  java.desktop
>>   * @run main/othervm Test6277246
>>   * @author Jeff Nisewanger
>> @@ -36,11 +36,10 @@
>>  import java.beans.Introspector;
>>  import java.beans.MethodDescriptor;
>>  import java.lang.reflect.Method;
>> -import sun.misc.BASE64Encoder;
>>public class Test6277246 {
>>  public static void main(String[] args) throws IntrospectionException {
>> -Class type = BASE64Encoder.class;
>> +Class type = sun.security.x509.X509CertInfo.class;
>>  System.setSecurityManager(new SecurityManager());
>>  BeanInfo info = Introspector.getBeanInfo(type);
>>  for (MethodDescriptor md : info.getMethodDescriptors()) {
>> @@ -48,7 +47,7 @@
>>  System.out.println(method);
>>String name = method.getDeclaringClass().getName();
>> -if (name.startsWith("sun.misc.")) {
>> +if (name.startsWith("sun.")) {
>>  throw new Error("found inaccessible method");
>>  }
>>  }
>> 
>> diff --git a/test/ProblemList.txt b/test/ProblemList.txt
>> --- a/test/ProblemList.txt
>> +++ b/test/ProblemList.txt
>> @@ -126,9 +126,6 @@
>>  java/beans/Introspector/8132566/OverridePropertyInfoTest.java generic-all
>>  java/beans/Introspector/8132566/OverrideUserDefPropertyInfoTest.java 
>> generic-all
>>  -# 8145589
>> -java/beans/Introspector/Test6277246.java  generic-all
>> -
>>  
>># jdk_lang
>> 
>> 
>> -Chris.
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8144479
> 

Re: Review Request 8144553: java/lang/StackWalker/StackWalkTest.java and MultiThreadStackWalk.java fail with stack overflows

[Daniel Fuchs]

On 17/12/15 17:07, Mandy Chung wrote:



On Dec 17, 2015, at 7:39 AM, Daniel Fuchs  wrote:

On 17/12/15 16:22, Mandy Chung wrote:

On Dec 17, 2015, at 6:02 AM, Daniel Fuchs  wrote:


Hi Mandy,

I believe it would be good to have some test that go over
the 1024 limit - as this has been useful to detect bugs
when we were actively prototyping the API.

So  maybe we should first try to reduce from 2000 to e.g. 1028?


Are you relating 1024 to the MaxJavaStackTraceDepth?  That is the max depth of 
builtin  backtrace.  StackWalker no longer has the maxDepth.


Yes. Is this truly gone? I thought it was still lurking :-)

StackStreamFactory.java
918:private static final int MAX_STACK_FRAMES = 1024;


Oh - OK then.

-- daniel






This is for StackTrace which is used to generate Thread::dumpStack and 
Thread::getStackTrace with a limit on the number of stack trace elements.

Mandy

Re: RFR (S) 8145428: Optimize StringUTF16 compress/copy methods for C1

[Aleksey Shipilev]

On 12/17/2015 04:54 PM, Ulf Zibis wrote:
> Am 17.12.2015 um 08:54 schrieb Aleksey Shipilev:
>> On 12/17/2015 02:34 AM, Ulf wrote:
>>> I'm wondering why moving the increment operation to an extra line wound
>>> enhance performance.
>> Because C1 is very straightforward, and code movement like that is a
>> poor man's instruction scheduling, that pads out the data dependency
>> between index update and indexed access.
> 
> Thanks.
> Shouldn't this simple optimization be addressed to javac?
> Otherwise programmers always risk some performance when using the
> post-increment idiom, which IMHO is good code style.

No, it should not be addressed in javac, as good optimizing compiler
would handle this without involving bytecode scheduling in the Java
compiler.

With C1, it is always the "low-cost" branch of cost-benefit game -- if
you can spend a few hours polishing the code to make it perform better
with C1, then you should do it. But, wasting man-years of effort to
please C1 is not almost wise resource investment. This is why I am
resistant to any sort of bikeshedding w.r.t. these particular changes --
we did them, they improved C1, let's move on.

Thanks,
-Aleksey

Re: RFR [9] 8145589: Test6277246.java fails to compile after JDK-8144479

[Chris Hegarty]

For ease of review, I moved the complete changes into a webrev.

  http://cr.openjdk.java.net/~chegar/8145589/webrev/

-Chris.

On 17 Dec 2015, at 15:08, Chris Hegarty  wrote:

> On 17 Dec 2015, at 14:54, Roger Riggs  wrote:
> 
>> Hi Chris,
>> 
>> Looks fine.
> 
> Thanks Roger. Just a little addition to this review.
> 
> While not strictly necessary, I’d like to clean up another, string,
> reference use in a reflective call. ( It is not causing a failure as
> the security manager will restrict access to sun.*, but it may 
> avoid confusion in the future.
> 
> diff --git a/test/java/beans/EventHandler/Test6277246.java 
> b/test/java/beans/EventHandler/Test6277246.java
> --- a/test/java/beans/EventHandler/Test6277246.java
> +++ b/test/java/beans/EventHandler/Test6277246.java
> @@ -39,7 +39,7 @@
> Class container = Class.forName("java.lang.Class");
> Class parameter = Class.forName("java.lang.String");
> Method method = container.getMethod("forName", parameter);
> -Object[] arglist = new Object[] {"sun.misc.BASE64Encoder"};
> +Object[] arglist = new Object[] 
> {"sun.security.x509.X509CertInfo"};
> EventHandler eh = new EventHandler(Test6277246.class, "forName", 
> "", "forName");
> Object object = eh.invoke(null, method, arglist);
> throw new Error((object != null) ? "test failure" : "test error”);
> 
> -Chris.
> 
>> Roger
>> 
>> 
>> On 12/17/2015 9:46 AM, Chris Hegarty wrote:
>>> The removal of BASE64Encoder, and a related types, in 8144479 [1]
>>> has triggered the failure of java/beans/Introspector/Test6277246.java.
>>> Another internal type should be used instead of sun.misc.BASE64Encoder.
>>> The sun.security.x509 package seems stable, and is being used in other
>>> areas, like langtools and jigsaw/jake to test accessibility.
>>> 
>>> 
>>> diff --git a/test/java/beans/Introspector/Test6277246.java 
>>> b/test/java/beans/Introspector/Test6277246.java
>>> --- a/test/java/beans/Introspector/Test6277246.java
>>> +++ b/test/java/beans/Introspector/Test6277246.java
>>> @@ -25,7 +25,7 @@
>>>  * @test
>>>  * @bug 6277246
>>>  * @summary Tests problem with java.beans use of reflection
>>> - * @modules java.base/sun.misc
>>> + * @modules java.base/sun.security.x509
>>>  *  java.desktop
>>>  * @run main/othervm Test6277246
>>>  * @author Jeff Nisewanger
>>> @@ -36,11 +36,10 @@
>>> import java.beans.Introspector;
>>> import java.beans.MethodDescriptor;
>>> import java.lang.reflect.Method;
>>> -import sun.misc.BASE64Encoder;
>>>   public class Test6277246 {
>>> public static void main(String[] args) throws IntrospectionException {
>>> -Class type = BASE64Encoder.class;
>>> +Class type = sun.security.x509.X509CertInfo.class;
>>> System.setSecurityManager(new SecurityManager());
>>> BeanInfo info = Introspector.getBeanInfo(type);
>>> for (MethodDescriptor md : info.getMethodDescriptors()) {
>>> @@ -48,7 +47,7 @@
>>> System.out.println(method);
>>>   String name = method.getDeclaringClass().getName();
>>> -if (name.startsWith("sun.misc.")) {
>>> +if (name.startsWith("sun.")) {
>>> throw new Error("found inaccessible method");
>>> }
>>> }
>>> 
>>> diff --git a/test/ProblemList.txt b/test/ProblemList.txt
>>> --- a/test/ProblemList.txt
>>> +++ b/test/ProblemList.txt
>>> @@ -126,9 +126,6 @@
>>> java/beans/Introspector/8132566/OverridePropertyInfoTest.java generic-all
>>> java/beans/Introspector/8132566/OverrideUserDefPropertyInfoTest.java 
>>> generic-all
>>> -# 8145589
>>> -java/beans/Introspector/Test6277246.java  generic-all
>>> -
>>> 
>>>   # jdk_lang
>>> 
>>> 
>>> -Chris.
>>> 
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8144479
>> 
> 

Re: Review Request 8144553: java/lang/StackWalker/StackWalkTest.java and MultiThreadStackWalk.java fail with stack overflows

[Daniel Fuchs]

On 17/12/15 16:22, Mandy Chung wrote:

On Dec 17, 2015, at 6:02 AM, Daniel Fuchs  wrote:
>
>Hi Mandy,
>
>I believe it would be good to have some test that go over
>the 1024 limit - as this has been useful to detect bugs
>when we were actively prototyping the API.
>
>So  maybe we should first try to reduce from 2000 to e.g. 1028?
>

Are you relating 1024 to the MaxJavaStackTraceDepth?  That is the max depth of 
builtin  backtrace.  StackWalker no longer has the maxDepth.


Yes. Is this truly gone? I thought it was still lurking :-)

StackStreamFactory.java
918:private static final int MAX_STACK_FRAMES = 1024;


best regards,

-- daniel

Re: RFR (S) 8145428: Optimize StringUTF16 compress/copy methods for C1

[Roger Riggs]

Hi Aleksey,

I should have worded it as a suggestion (and provided the words) so as 
avoided impeding progress.
It could have been as general as saying that an @implNote that String is 
uniquely performance sensitive,
that would apply to the whole file, not just a few expressions, and it 
should probably have been there

for 20 years.

You are particularly sensitive to performance, not every developer is 
and they assume
that the compiler does reasonable optimizations (it doesn't) and that 
what look
like equivalent expressions should work the same.  As the comments to 
your original change
elicited, the change is tuning the source code for a particular 
optimization (or lack of) in C1.


I agree that you should just push it and move on.

Roger


On 12/17/2015 11:37 AM, Aleksey Shipilev wrote:

Hi Roger,

Um, do you really want to spell out "thou shalt run thee benchmarks
while changing the String hotpath" in source code comments?

I disagree: documenting development processes in the source code is odd.
But equally odd is the suggestion that a common development practice
would *not* involve running benchmarks when changing the ubiquitous
codepath in the standard library, *or* studying the prior history for
the code under modification. Some knowledge really is common.

Can we pretty please get this done, and move on to other interesting things?

Thanks,
-Aleksey

On 12/17/2015 05:55 PM, Roger Riggs wrote:

Hi Alexsey,

The 'expected to run benchmarks' might the operative comment in the code.
'Common' knowledge sometimes isn't so common.

Roger


On 12/17/2015 2:54 AM, Aleksey Shipilev wrote:

On 12/17/2015 02:34 AM, Ulf wrote:

I'm wondering why moving the increment operation to an extra line wound
enhance performance.

Because C1 is very straightforward, and code movement like that is a
poor man's instruction scheduling, that pads out the data dependency
between index update and indexed access. I don't think it deserves a
comment -- it is expected one will run the benchmarks when changing that
code.

Thanks,
-Aleksey

Re: Review Request 8144553: java/lang/StackWalker/StackWalkTest.java and MultiThreadStackWalk.java fail with stack overflows

[Mandy Chung]

> On Dec 17, 2015, at 7:39 AM, Daniel Fuchs  wrote:
> 
> On 17/12/15 16:22, Mandy Chung wrote:
>>> On Dec 17, 2015, at 6:02 AM, Daniel Fuchs  wrote:
>>> >
>>> >Hi Mandy,
>>> >
>>> >I believe it would be good to have some test that go over
>>> >the 1024 limit - as this has been useful to detect bugs
>>> >when we were actively prototyping the API.
>>> >
>>> >So  maybe we should first try to reduce from 2000 to e.g. 1028?
>>> >
>> Are you relating 1024 to the MaxJavaStackTraceDepth?  That is the max depth 
>> of builtin  backtrace.  StackWalker no longer has the maxDepth.
> 
> Yes. Is this truly gone? I thought it was still lurking :-)
> 
> StackStreamFactory.java
> 918:private static final int MAX_STACK_FRAMES = 1024;
> 


This is for StackTrace which is used to generate Thread::dumpStack and 
Thread::getStackTrace with a limit on the number of stack trace elements.

Mandy

Re: RFR (XS) 8145539: (coll) AbstractMap.keySet and .values should not be volatile

[Doug Lea]

On 12/16/2015 05:53 PM, Aleksey Shipilev wrote:

Hi,

Since the dawn of OpenJDK, AbstractMap.keySet and .value were defined as
package-private volatile fields. Their only use is to cache keySet and
valueSet implementations from java.util collections.


I think these were declared volatile to be more clearly JMM compliant.
They are definitely OK without it, but you should probably add some
rationale in documentation, mentioning that all java.util.Map view class
implementations using these fields have no non-final fields (or any
fields at all except for outer-this).

-Doug

Re: Review Request 8144553: java/lang/StackWalker/StackWalkTest.java and MultiThreadStackWalk.java fail with stack overflows

[Mandy Chung]

> On Dec 17, 2015, at 6:02 AM, Daniel Fuchs  wrote:
> 
> Hi Mandy,
> 
> I believe it would be good to have some test that go over
> the 1024 limit - as this has been useful to detect bugs
> when we were actively prototyping the API.
> 
> So  maybe we should first try to reduce from 2000 to e.g. 1028?
> 

Are you relating 1024 to the MaxJavaStackTraceDepth?  That is the max depth of 
builtin  backtrace.  StackWalker no longer has the maxDepth.

Mandy

> best regards,
> 
> -- daniel
> 
> On 16/12/15 21:30, Mandy Chung wrote:
>> This is a test fix to reduce the stack depth to avoid StackOverFlow when 
>> running with -Xcomp on solaris sparc.
>> 
>> Webrev:
>>
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8144553/webrev.00/index.html
>> 
>> I verified the tests on solaris-sparcv9 machine with fastdebug VM -Xcomp -ea 
>> -esa.
>> 
>> Mandy
>> 
> 

Re: RFR [9] 8145589: Test6277246.java fails to compile after JDK-8144479

[Roger Riggs]

+1

On 12/17/2015 10:15 AM, Chris Hegarty wrote:

For ease of review, I moved the complete changes into a webrev.

   http://cr.openjdk.java.net/~chegar/8145589/webrev/

-Chris.

On 17 Dec 2015, at 15:08, Chris Hegarty  wrote:


On 17 Dec 2015, at 14:54, Roger Riggs  wrote:


Hi Chris,

Looks fine.

Thanks Roger. Just a little addition to this review.

While not strictly necessary, I’d like to clean up another, string,
reference use in a reflective call. ( It is not causing a failure as
the security manager will restrict access to sun.*, but it may
avoid confusion in the future.

diff --git a/test/java/beans/EventHandler/Test6277246.java 
b/test/java/beans/EventHandler/Test6277246.java
--- a/test/java/beans/EventHandler/Test6277246.java
+++ b/test/java/beans/EventHandler/Test6277246.java
@@ -39,7 +39,7 @@
 Class container = Class.forName("java.lang.Class");
 Class parameter = Class.forName("java.lang.String");
 Method method = container.getMethod("forName", parameter);
-Object[] arglist = new Object[] {"sun.misc.BASE64Encoder"};
+Object[] arglist = new Object[] {"sun.security.x509.X509CertInfo"};
 EventHandler eh = new EventHandler(Test6277246.class, "forName", "", 
"forName");
 Object object = eh.invoke(null, method, arglist);
 throw new Error((object != null) ? "test failure" : "test error”);

-Chris.


Roger


On 12/17/2015 9:46 AM, Chris Hegarty wrote:

The removal of BASE64Encoder, and a related types, in 8144479 [1]
has triggered the failure of java/beans/Introspector/Test6277246.java.
Another internal type should be used instead of sun.misc.BASE64Encoder.
The sun.security.x509 package seems stable, and is being used in other
areas, like langtools and jigsaw/jake to test accessibility.


diff --git a/test/java/beans/Introspector/Test6277246.java 
b/test/java/beans/Introspector/Test6277246.java
--- a/test/java/beans/Introspector/Test6277246.java
+++ b/test/java/beans/Introspector/Test6277246.java
@@ -25,7 +25,7 @@
  * @test
  * @bug 6277246
  * @summary Tests problem with java.beans use of reflection
- * @modules java.base/sun.misc
+ * @modules java.base/sun.security.x509
  *  java.desktop
  * @run main/othervm Test6277246
  * @author Jeff Nisewanger
@@ -36,11 +36,10 @@
import java.beans.Introspector;
import java.beans.MethodDescriptor;
import java.lang.reflect.Method;
-import sun.misc.BASE64Encoder;
   public class Test6277246 {
 public static void main(String[] args) throws IntrospectionException {
-Class type = BASE64Encoder.class;
+Class type = sun.security.x509.X509CertInfo.class;
 System.setSecurityManager(new SecurityManager());
 BeanInfo info = Introspector.getBeanInfo(type);
 for (MethodDescriptor md : info.getMethodDescriptors()) {
@@ -48,7 +47,7 @@
 System.out.println(method);
   String name = method.getDeclaringClass().getName();
-if (name.startsWith("sun.misc.")) {
+if (name.startsWith("sun.")) {
 throw new Error("found inaccessible method");
 }
 }

diff --git a/test/ProblemList.txt b/test/ProblemList.txt
--- a/test/ProblemList.txt
+++ b/test/ProblemList.txt
@@ -126,9 +126,6 @@
java/beans/Introspector/8132566/OverridePropertyInfoTest.java generic-all
java/beans/Introspector/8132566/OverrideUserDefPropertyInfoTest.java generic-all
-# 8145589
-java/beans/Introspector/Test6277246.java  generic-all
-

   # jdk_lang


-Chris.

[1] https://bugs.openjdk.java.net/browse/JDK-8144479

Re: RFR (XS) 8145539: (coll) AbstractMap.keySet and .values should not be volatile

[Aleksey Shipilev]

On 12/17/2015 06:07 PM, Doug Lea wrote:
> On 12/16/2015 05:53 PM, Aleksey Shipilev wrote:
>> Since the dawn of OpenJDK, AbstractMap.keySet and .value were defined as
>> package-private volatile fields. Their only use is to cache keySet and
>> valueSet implementations from java.util collections.
> 
> I think these were declared volatile to be more clearly JMM compliant.
> They are definitely OK without it, but you should probably add some
> rationale in documentation, mentioning that all java.util.Map view class
> implementations using these fields have no non-final fields (or any
> fields at all except for outer-this).

Good idea, did so:
 http://cr.openjdk.java.net/~shade/8145539/webrev.04/

(There is also an @implSpec on keySet() and values() that say no
synchronization is performed)

Thanks,
-Aleksey

Re: RFR (S) 8145428: Optimize StringUTF16 compress/copy methods for C1

[Aleksey Shipilev]

Hi Roger,

Um, do you really want to spell out "thou shalt run thee benchmarks
while changing the String hotpath" in source code comments?

I disagree: documenting development processes in the source code is odd.
But equally odd is the suggestion that a common development practice
would *not* involve running benchmarks when changing the ubiquitous
codepath in the standard library, *or* studying the prior history for
the code under modification. Some knowledge really is common.

Can we pretty please get this done, and move on to other interesting things?

Thanks,
-Aleksey

On 12/17/2015 05:55 PM, Roger Riggs wrote:
> Hi Alexsey,
> 
> The 'expected to run benchmarks' might the operative comment in the code.
> 'Common' knowledge sometimes isn't so common.
> 
> Roger
> 
> 
> On 12/17/2015 2:54 AM, Aleksey Shipilev wrote:
>> On 12/17/2015 02:34 AM, Ulf wrote:
>>> I'm wondering why moving the increment operation to an extra line wound
>>> enhance performance.
>> Because C1 is very straightforward, and code movement like that is a
>> poor man's instruction scheduling, that pads out the data dependency
>> between index update and indexed access. I don't think it deserves a
>> comment -- it is expected one will run the benchmarks when changing that
>> code.
>>
>> Thanks,
>> -Aleksey
>>
> 

Re: RFR:8143413:add toEpochSecond methods for efficient access

[nadeesh tv]

Hi all,

Please see the updated webrev 
http://cr.openjdk.java.net/~ntv/8143413/webrev.03/


Thanks and Regards,
Nadeesh
On 12/4/2015 3:56 AM, Stephen Colebourne wrote:

In the interests of harmony and getting something in, I'll accept that.

I think LocalDate.EPOCH is probably better than LocalDate.EPOCHDAY

Stephen



On 3 December 2015 at 22:09, Roger Riggs  wrote:

Hi Sherman,

It makes sense to me to provide the missing time/date as an explicit
parameter
for toEpochSeconds instead of assuming some constant.

localDate.toEpochSeconds(LocalTime.MIDNIGHT, ZoneOffset.UTC);
localTime.toEpochSeconds(LocalDate.EPOCHDAY, ZoneOffset.UTC);
offsetTime.toEpochSeconds(LocalDate.EPOCHDAY);

We'll have to add the LocalDate.EPOCHDAY constant.

It makes it a bit heavier weight to use but can still be optimized and won't
create garbage.

Roger



On 12/01/2015 12:33 PM, Xueming Shen wrote:

On 12/1/15 6:36 AM, Stephen Colebourne wrote:

As Roger says, these new methods are about performance as well as
conversion.

While I fully acknowledge the time methods make an assumption, it is
not a crazy one, after all 1970-01-01 is just zero.

Key I think is it allows:
   long epochSecs = date.toEpochSeconds(offset) +
time.toEpochSeconds(offset);
to efficiently merge two objects without garbage.

So it's not about j.t.LD/LT <-> j.u.Date, but instead of the clean
approach

LocalDate date = ...
LocalDate time = ...
ZoneOffset offset = ...

==> long spochSecs = LocalDateTime.of(date, time).toEpochSeconds(offset);

we are adding APIs to provide a "fastpath" with the special assumption
that the LocalDate "date"
here is actually a "LocalDateTime" object ("date" + LocalTime.MIDNIGHT)
and the LocalTime "time"
again actually means a "LocalDateTime" (the "time" of 1970-01-01), to  let
the third party "libraries"
to fool the basic date/time abstract in java.time package,  simply to
avoid creating the garbage
middle man, localDateTime? it really does not sound right to me.

First, if someone needs to mix/link LocalDate, LocalTime and Offset to
epoch seconds in their
libraries, they really SHOULD think hard about the conversion and make it
right (it should not
be encouraged to use these classes LocalDate, LocalTime and Offset without
even understand
what these classes are). But if we really have to provide such fastpath,
personally I think it might
be better either to provide these "utility/convenient" methods in a
"utilities" class, or with an
explicit date/time parameters (instead of the false assumption) for the
missing date/time piece,
such as

localDate.toEpochSeconds(LocalTime.MIDNIGHT, offset);
localTime.toEpochSeconds(LocalDate.EPOCHDAY, offset);

Sherman



It also means that no-one has to think hard about the conversion, as
it is just there. It tends to be when people try to work this stuff
out for themselves that they get it wrong.

Stephen


On 1 December 2015 at 14:21, Roger Riggs  wrote:

Hi Sherman,

On 11/30/2015 6:09 PM, Xueming Shen wrote:

On 11/30/2015 01:26 PM, Stephen Colebourne wrote:

Converting LocalDate<-> java.util.Date is the question with the
largest number of votes on SO:


http://stackoverflow.com/questions/21242110/convert-java-util-date-to-java-time-localdate/21242111
The proposed method is designed to make the conversion easier. It also
plugs an obvious gap in the API.

While the LocalTime/OffsetTime methods are of lower importance, not
having them would result in inconsistency between the various classes.
We've already added factory methods to LocalTime for Java 9, these are
just the other half of the picture.


I'm not sure I understand the idea of "the proposed method is designed
to
make the conversion easier", as the SO is mainly about
j.u.Date->LocalDate,
not the other way around, from LocalDate -> j.u.Date.

I think the issue is about *other* libraries that manipulate time via
epochSeconds
not about j.u.Date conversions.  The concern was about performance and
creating garbage along the way.

Roger




As I said in the previous email, it might be "common" to use the
j.u.Date
to
abstract a "local date" and/or a "local time" (no other choice) before
java.time,
and now there is the need to provide a migration path from those "local
date/
time" to the j.t.LocalDate/Time. But convert backward from the new
date/time
type to the "old" j.u.Date should not be encouraged (guess this is also
the
consensus we agreed on back to jsr203).

What are the "factory methods" you are referring to here? JDK-8133079,
The
LocalDate/LocalTime.ofInstant()?
(btw, these two methods see missing the "since 1.9/9" tag)

It seems to me that the ofInstant(Instant, ZondId) is from a
"super-set"
of
date/time to a "sub-set", without any assumption of "default value", it
is
similar to the conversion from zdt->ldt->ld/lt, and I can see the
"small"
improvement

from|
Date input = new Date();
LocalDatedate
=input.toInstant().atZone(ZoneId.systemDefault()).toLocalDate();|

to

|LocalDatedate

Re: RFR (XS) 8145539: (coll) AbstractMap.keySet and .values should not be volatile

[Paul Sandoz]

> On 17 Dec 2015, at 16:54, Aleksey Shipilev  
> wrote:
> 
> On 12/17/2015 06:07 PM, Doug Lea wrote:
>> On 12/16/2015 05:53 PM, Aleksey Shipilev wrote:
>>> Since the dawn of OpenJDK, AbstractMap.keySet and .value were defined as
>>> package-private volatile fields. Their only use is to cache keySet and
>>> valueSet implementations from java.util collections.
>> 
>> I think these were declared volatile to be more clearly JMM compliant.
>> They are definitely OK without it, but you should probably add some
>> rationale in documentation, mentioning that all java.util.Map view class
>> implementations using these fields have no non-final fields (or any
>> fields at all except for outer-this).
> 
> Good idea, did so:
> http://cr.openjdk.java.net/~shade/8145539/webrev.04/
> 
> (There is also an @implSpec on keySet() and values() that say no
> synchronization is performed)
> 

+1

Paul.

Re: RFR 4823133: RandomAccessFile.length() is not thread-safe

[Martin Buchholz]

Looks good!

On Thu, Dec 17, 2015 at 12:14 AM, vyom  wrote:
> Hi Martin,
>
> thanks for review, i undated the
> webrev(http://cr.openjdk.java.net/~vtewari/4823133/webrev0.2/
> ) as per your
> comment after confirming that the corresponding "fd" if opened with
> "open64".
>
> Thanks,
> Vyom
>
>
>
> On Thursday 17 December 2015 12:38 AM, Martin Buchholz wrote:
>>
>> Calling naked fstat without _FILE_OFFSET_BITS=64 is a bug.  Don't you
>> need to call fstat64 if it's available?
>>
>> +jlong
>> +handleGetLength(FD fd)
>> +{
>> +struct stat sb;
>> +if (fstat(fd, ) == 0) {
>> +return sb.st_size;
>> +} else {
>> +return -1;
>> +}
>> +}
>>
>> On Wed, Dec 16, 2015 at 5:02 AM, vyom  wrote:
>>>
>>> Hi,
>>>
>>> Updated the webrev(http://cr.openjdk.java.net/~vtewari/4823133/webrev0.1/
>>> ) as per Martin
>>> review comment removed the _FILE_OFFSET_BITS from source.
>>>
>>> Thanks,
>>> Vyom
>>>
>>>
>>>
>>> On Tuesday 15 December 2015 10:55 PM, Martin Buchholz wrote:

 _FILE_OFFSET_BITS is generally an all-or-nothing thing, because it
 affects interoperability between translation units.  It would be good
 to convert all of the JDK build to use -D_FILE_OFFSET_BITS=64, but
 that would be a big job.  So traditionally the JDK has instead used
 the functions made available via _LARGEFILE64_SOURCE.  But that is
 also a JDK-wide job now, because every call to plain stat in the
 source code is broken on 32-bit systems with 64-bit inodes, which are
 becoming more common.

 I recommend the _FILE_OFFSET_BITS=64 strategy, but it's probably a job
 for build-dev, not core-libs-dev.




 On Tue, Dec 15, 2015 at 8:31 AM, Roger Riggs 
 wrote:
>
> Hi Yvom,
>
> Minor comments:
>
> src/java.base/share/native/libjava/RandomAccessFile.c:
>- "length fail" might be clearer as "GetLength failed"
>
> src/java.base/unix/native/libjava/io_util_md.c:
>
>- Please add a comment before the define of FILE_OFFSET_BITS to
> indicate
> where it is used and why it is there.
>- BTW, are there any unintended side effects?
>  Perhaps a different issue but perhaps 64 bit offsets should be
> used
> everywhere
>
> src/java.base/windows/native/libjava/io_util_md.c
>- Line 592: Using INVALID_HANDLE_VALUE is better than -1 and is used
> elsewhere in the file
>  BTW, Testing for invalid handle might be unnecessary since the
> call
> to
> GetFileSizeEx will fail
>  if it is invalid, yielding the same result.
>
> Roger
>
>
> On 12/10/2015 5:52 AM, vyom wrote:
>>
>> Hi All,
>>
>> Please review my changes for below bug.
>>
>> Bug: JDK-4823133 : RandomAccessFile.length() is not thread-safe
>>
>> Webrev:http://cr.openjdk.java.net/~vtewari/4823133/webrev0.0/
>> 
>>
>> This change ensure that  length() does not temporarily changes the
>> file
>> pointer and it will make sure that there is no race
>> condition in case of multi thread uses.
>>
>> Thanks,
>> Vyom
>>
>>
>>
>>
>

Re: RFR (S) 8145428: Optimize StringUTF16 compress/copy methods for C1

[Paul Sandoz]

> On 17 Dec 2015, at 17:54, Roger Riggs  wrote:
> 
> I agree that you should just push it and move on.
> 

+1

Paul.

Re: RFR: JDK-8115868: 32-bit JVM failed to start from a large network filesystem

[Martin Buchholz]

This is surely progress, but no one seems to own the problem of fixing
all the stat calls in the JDK.
Instead of failing at startup, we may get a more obscure failure later
at runtime, which not everyone will think is progress.

On Thu, Dec 17, 2015 at 5:32 AM, Kumar Srinivasan
 wrote:
>
> On 12/16/2015 11:01 AM, Martin Buchholz wrote:
>>
>> On Tue, Dec 15, 2015 at 2:12 PM, Bernd Eckenfels 
>> wrote:
>>>
>>> Hello,
>>>
>>> I always like it when access() is used instead of stat() magic.
>>>
>>> I noticed that the new ProgramExists in java_md_common.c does not
>>> anymore reject directories (which are typically executable). Not sure
>>> it this matters or is intentional, it is a change in semantic.
>>
>> Right.  This is a small loss of robustness.  Changing stat into access
>> here isn't really right because not stat has a superset of
>> functionality, and (like I keep saying) all the stat calls in the JDK
>> are potentially broken and need fixing.
>
>
> I agree this is a very small corner case, the caller (there is one call and
> will always
> test for the existence of a file). So I am going to go with this, unless I
> hear objections.
>
> Kumar
>
>

Re: RFR: 8145686: SimpleConsoleLogger and LogRecord should take advantage of StackWalker to skip classes implementing System.Logger

[Mandy Chung]

> On Dec 17, 2015, at 8:42 AM, Daniel Fuchs  wrote:
> 
> Hi,
> 
> Please find below a small enhancement for the method that infers
> the calling class name and method name in  SimpleConsoleLogger
> and LogRecord.
> 
> The idea is that since these methods now use the new StackWalker API
> they should take advantage of that in order to skip classes that
> implement System.Logger. This should make it possible for components
> that need it to easily wrap a System.Logger (and even a j.u.l.Logger).
> 
> 8145686: SimpleConsoleLogger and LogRecord should take advantage
> of StackWalker to skip classes implementing System.Logger
> https://bugs.openjdk.java.net/browse/JDK-8145686
> 
> webrev:
> http://cr.openjdk.java.net/~dfuchs/webrev_8145686/webrev.00/

What is the difference between skipLoggingFrame and isLoggerImplFrame?

Mandy

Re: RFR (S) 8145428: Optimize StringUTF16 compress/copy methods for C1

[Aleksey Shipilev]

On 12/17/2015 08:18 PM, Paul Sandoz wrote:
> 
>> On 17 Dec 2015, at 17:54, Roger Riggs  wrote:
>>
>> I agree that you should just push it and move on.
>>
> 
> +1

Thanks Claes, Sherman, Roger, and Paul! Pushed.

Cheers,
-Aleksey

Re: RFR: 8145686: SimpleConsoleLogger and LogRecord should take advantage of StackWalker to skip classes implementing System.Logger

[Daniel Fuchs]

On 12/17/15 7:02 PM, Mandy Chung wrote:



On Dec 17, 2015, at 8:42 AM, Daniel Fuchs  wrote:

Hi,

Please find below a small enhancement for the method that infers
the calling class name and method name in  SimpleConsoleLogger
and LogRecord.

The idea is that since these methods now use the new StackWalker API
they should take advantage of that in order to skip classes that
implement System.Logger. This should make it possible for components
that need it to easily wrap a System.Logger (and even a j.u.l.Logger).

8145686: SimpleConsoleLogger and LogRecord should take advantage
 of StackWalker to skip classes implementing System.Logger
https://bugs.openjdk.java.net/browse/JDK-8145686

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8145686/webrev.00/


What is the difference between skipLoggingFrame and isLoggerImplFrame?


isLoggingImplFrame stops when it finds the concrete logger 
implementation. It will unstack all the LogRecord, Handler, Filter

implementations etc... (which could be custom application classes)

skipLoggingFrame will skip everything above the first method found
on the concrete Logger implementation until it reaches something that
looks like a calling frame. In particular it should skip other
methods of the logger implementation (e.g. log() calls log(LogRecord),
default interface methods provided by the logging framework,
reflection/method handle frames etc...
In particular it should skip any Logger wrapper classes.

For instance, RMI has a logger that wraps a j.u.l.Logger so
ideally that should be skipped too...
JMX remote has a ClassLogger that should also be skipped.
etc...

-- daniel



Mandy

Re: RFR: 8071507: (ref) Clear phantom reference as soft and weak references do

[Kim Barrett]

On Dec 4, 2015, at 1:07 PM, Kim Barrett  wrote:
> 
> On Dec 3, 2015, at 6:04 PM, Mandy Chung  wrote:
>> 
>>> [Indeed, this whole section isn't strictly necessary; all of it can be
>>> inferred from information in other places.]
>> 
>> Agree.  This section is no longer necessary and maybe just remove it:
> 
> I wasn't actually intending to suggest removal.  It seems like there
> is useful overview information to be had here, rather than requiring
> readers to make not necessarily obvious inferences.  My impression is
> that readability is valued more highly than terseness in Java
> documentation.

Sorry for the long gap in the discussion.  Mandy and I have been
talking about what to do about the "Automatically-cleared references"
section that was the topic of some debate.  We've decided to eliminate
it, which led us to some additional modifications.

Here's the latest set of webrevs:
http://cr.openjdk.java.net/~kbarrett/8071507/jdk.06/
http://cr.openjdk.java.net/~kbarrett/8071507/hotspot.06/

They've been rebased to hs-rt tip, which required resolving a minor
merge conflict with a nearby change to logging in
referenceProcessor.cpp.  Other than that, there are only specification
wording changes.  Here's the incremental change from the previous
version:
http://cr.openjdk.java.net/~kbarrett/8071507/jdk.06.inc/

We are treating the discussions around changing PhantomReference to
forbid a null queue as out of scope for this change.  As Mandy said
earlier, she might propose a separate change for that in the future.

Re: RFR: 8071507: (ref) Clear phantom reference as soft and weak references do

[Mandy Chung]

> On Dec 17, 2015, at 6:05 PM, Tao Mao  wrote:
> 
> Hi Kim,
> 
> 34  *  Suppose the garbage collector determines at a certain point in time
>   35  * that an object is 
>   36  * phantom reachable.  At that time it will atomically clear
>   37  * all phantom references to that object and all phantom references to
>   38  * any other phantom-reachable objects from which that object is 
> reachable.
>   39  * At the same time or at some later time it will enqueue those 
> newly-cleared
>   40  * phantom references that are registered with reference queues.
> 
> XYZRefenrece concept is never being understood too well besides the authors 
> who spend time grinding words. WeakReference 
> (https://docs.oracle.com/javase/7/docs/api/java/lang/ref/WeakReference.html) 
> states this: "At that time it will atomically clear all weak references to 
> that object and all weak references to any other weakly-reachable objects 
> from which that object is reachable through a chain of strong and soft 
> references."
> 
> On line 38, can we also add "through a chain of strong, soft, and weak 
> references." to the above, which I think is the case?
> 

Yes it is and this is not strictly needed. For phantom reference case, this can 
be left “unqualified” as it’s basically a chain of any reference type.

> In addition, in src/java.base/share/classes/java/lang/ref/package-info.java, 
> can we add WeakHashMap 
> (https://docs.oracle.com/javase/7/docs/api/java/util/WeakHashMap.html) as a 
> concrete example of so-called canonicalized mappings to help readers 
> understand its usage? I always need to look up for what "canonicalized 
> mappings" really means and end up with "OK, it's WeakHashMap”.

In the Notification section, it does reference WeakHashMap (although it might 
not contain the exact description you are looking at).

Mandy

> Thanks.
> Tao Mao
> 
> 
> 
> On Thu, Dec 17, 2015 at 1:30 PM, Kim Barrett  wrote:
> On Dec 4, 2015, at 1:07 PM, Kim Barrett  wrote:
> >
> > On Dec 3, 2015, at 6:04 PM, Mandy Chung  wrote:
> >>
> >>> [Indeed, this whole section isn't strictly necessary; all of it can be
> >>> inferred from information in other places.]
> >>
> >> Agree.  This section is no longer necessary and maybe just remove it:
> >
> > I wasn't actually intending to suggest removal.  It seems like there
> > is useful overview information to be had here, rather than requiring
> > readers to make not necessarily obvious inferences.  My impression is
> > that readability is valued more highly than terseness in Java
> > documentation.
> 
> Sorry for the long gap in the discussion.  Mandy and I have been
> talking about what to do about the "Automatically-cleared references"
> section that was the topic of some debate.  We've decided to eliminate
> it, which led us to some additional modifications.
> 
> Here's the latest set of webrevs:
> http://cr.openjdk.java.net/~kbarrett/8071507/jdk.06/
> http://cr.openjdk.java.net/~kbarrett/8071507/hotspot.06/
> 
> They've been rebased to hs-rt tip, which required resolving a minor
> merge conflict with a nearby change to logging in
> referenceProcessor.cpp.  Other than that, there are only specification
> wording changes.  Here's the incremental change from the previous
> version:
> http://cr.openjdk.java.net/~kbarrett/8071507/jdk.06.inc/
> 
> We are treating the discussions around changing PhantomReference to
> forbid a null queue as out of scope for this change.  As Mandy said
> earlier, she might propose a separate change for that in the future.
> 
> 

Re: RFR (XS) 8145539: (coll) AbstractMap.keySet and .values should not be volatile

[Aleksey Shipilev]

On 12/17/2015 08:21 PM, Paul Sandoz wrote:
>> On 17 Dec 2015, at 16:54, Aleksey Shipilev  
>> wrote:
>> On 12/17/2015 06:07 PM, Doug Lea wrote:
>>> On 12/16/2015 05:53 PM, Aleksey Shipilev wrote:
 Since the dawn of OpenJDK, AbstractMap.keySet and .value were defined as
 package-private volatile fields. Their only use is to cache keySet and
 valueSet implementations from java.util collections.
>>>
>>> I think these were declared volatile to be more clearly JMM compliant.
>>> They are definitely OK without it, but you should probably add some
>>> rationale in documentation, mentioning that all java.util.Map view class
>>> implementations using these fields have no non-final fields (or any
>>> fields at all except for outer-this).
>>
>> Good idea, did so:
>> http://cr.openjdk.java.net/~shade/8145539/webrev.04/
>>
>> (There is also an @implSpec on keySet() and values() that say no
>> synchronization is performed)
>>
> 
> +1

Thanks for reviews, Claes, Peter, Doug and Paul!
With no other comments, I pushed the changeset.

Thanks,
-Aleksey

Re: RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to use alternative to finalization

[Peter Levart]

Hi Roger,

On 12/16/2015 08:47 PM, Roger Riggs wrote:

Hi Peter,

It was a bit more involved than I expected, mostly in the tests to 
make this change.


Is this what you expected?  (just the deltas, I'll merge the patches 
before pushing).


http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696-no-clear/

Thanks, Roger


Yes, that's what I meant. The method is not final any more, but that's 
OK for now as those classes are encapsulated. If this ever gets exposed 
as a public API, it could be made as public subclasses of 
CleanerImpl.Cleanable classes and those subclasses could make the 
method final.


I see no other issues left besides the reachability races that I talked 
about in previous messages. Do you know if Reference.reachabilityFence 
is being pushed before this API?


Regards, Peter





On 12/15/2015 6:01 PM, Peter Levart wrote:



On 12/15/2015 11:48 PM, Roger Riggs wrote:

Hi Peter,

That will break up clearing the ref when the Cleanable is explicitly 
cleaned.

Reference.clear() needs to be called from Cleanable.clean().


From PhantomCleanable (the superclass of PhantomCleanableRef):

 253 @Override
 254 public final void clean() {
 255 if (remove()) {
 256 super.clear();
 257 performCleanup();
 258 }
 259 }
 260
 261 /**
 262  * Unregister this PhantomCleanable and clear the reference.
 263  * Due to inherent concurrency, {@link 
#performCleanup()} may still be invoked.

 264  */
 265 @Override
 266 public final void clear() {
 267 if (remove()) {
 268 super.clear();
 269 }
 270 }


... clean() calls super.clear(), which is "invokespecial" (not a 
virtual dispatch).



Regards, Peter



it might be nice to block that but to do so we'd need to go back to 
separate objects
for the Reference and the Cleanable and we worked hard to get to a 
single object.


Roger


On 12/15/2015 5:38 PM, Peter Levart wrote:

Hi Roger,

Just one thing about implementation:

Since the type exposed to user is Cleaner.Cleanable that has only a 
single method clean(), it would be good if the implementation class 
(CleanerImpl.PhantomCleanableRef) overrode 
CleanerImpl.PhantomCleanable.clear() method and threw 
UnsupportedOperationException, otherwise users will be tempted to 
cast the returned Cleaner.Cleanable to Reference and invoke clear() 
method to de-register cleanup action without invoking it. This is 
the only remaining public Reference method that is not disabled 
this way.


Regards, Peter

On 12/09/2015 07:40 PM, Roger Riggs wrote:

Hi,

The example is revised to caution about inner classes and lambdas.

[1]http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/
[2]http://cr.openjdk.java.net/~rriggs/cleaner-doc/index.html

Thanks, Roger

On 12/9/2015 11:04 AM, Peter Levart wrote:

Hi Chris,

On 12/09/2015 04:03 PM, Chris Hegarty wrote:

Peter,

On 09/12/15 07:05, Peter Levart wrote:

Hi,

I think the only way to try to prevent such things is with a good
example in javadoc that "screams" of possible miss-usages.


public static class CleanerExample implements AutoCloseable {

 private static final Cleaner cleaner = ...; // 
preferably a

shared cleaner

 private final PrivateNativeResource pnr;

 private final Cleaner.Cleanable cleanable;

 public CleanerExample(args, ...) {

 // prepare captured state as local vars...
 PrivateNativeResource _pnr = ...;

 this.cleanable = cleaner.register(this, () -> {
 // DON'T capture any instance fields with 
lambda since

that would
 // capture 'this' and prevent it from becoming


I assume that the WARNING should include anonymous inner classes 
too

( which I expect are quite common, though less now with lambda ) ?

Is "leaking" 'this' in a constructor a potential issue with respect
to the visibility of pnr? As well as causing red-squiggly lines in
the IDE ;-)


'this' only leaks to the 'referent' field of PhantomReference 
where by definition is not accessible.


'this' can become phantom-reachable before CleanerExample 
constructor ends. But this is harmless, because the code that may 
execute at that time does not access the object any more, so the 
object may be safely collected.


Cleanup action can run at any time after registration even before 
CleanerExample constructor ends. But this is harmless too, 
because it only accesses PrivateNativeResource which is fully 
constructed before registration of cleanup action.


I see no issues apart from IDE(s) not seeing no issues.

Regards, Peter



-Chris.



phantom-reachable!!!
 _pnr.close();
 });

 this.pnr = _pnr;
 }

 public void close() {
 cleanable.clean();
 }


Regards, Peter

Re: RFR (XS) 8145539: (coll) AbstractMap.keySet and .values should not be volatile

[Peter Levart]

Hi Aleksey,

Wouldn't that change make a possible outcome of keySet() returning null 
in case it was invoked concurrently? Wouldn't you have to use a local 
variable to prevent that?


Regards, Peter

On 12/16/2015 11:53 PM, Aleksey Shipilev wrote:

Hi,

Since the dawn of OpenJDK, AbstractMap.keySet and .value were defined as
package-private volatile fields. Their only use is to cache keySet and
valueSet implementations from java.util collections.

However, all relevant java.util collections are not having any declared
fields (an opaque reference to enclosing class is stored in final
field), and they delegate straight to the backing collection. Therefore,
any race on cache field is benign, and we can drop "volatile" from the
fields:
   https://bugs.openjdk.java.net/browse/JDK-8145539
   http://cr.openjdk.java.net/~shade/8145539/webrev.02/

This improves performance for keySet()/values() on AbstractMap
implementations, because we don't emit barriers (volatile write in x86
case). This does not affect AbstractMap subclasses allocation
performance, because the volatile field values were default since
JDK-8035284.

See:
  http://cr.openjdk.java.net/~shade/8145539/HashMapBench.java

Testing: microbenchmarks, java/util jtreg on Linux x86_64

Thanks,
-Aleksey

Re: Review request for JDK-8145545 : Typos in Javadoc of XmlAdapter

[Seán Coffey]

Looks fine.

Regards,
Sean.

On 17/12/2015 08:55, Abhijit Roy wrote:

Hi all,

  Please review a fix for Bug  - 
https://bugs.openjdk.java.net/browse/JDK-8145545

Bug - Typos in Javadoc of XmlAdapter

Webrev - http://cr.openjdk.java.net/~ntv/ababroy/8145545/webrev.00/

  I have just rectified and modified those error. And moving forward it for 
review.

  

  


Regards,

Abhijit.

Re: RFR: 8145686: SimpleConsoleLogger and LogRecord should take advantage of StackWalker to skip classes implementing System.Logger

[Mandy Chung]

> On Dec 17, 2015, at 11:23 AM, Daniel Fuchs  wrote:
> 
> On 12/17/15 7:02 PM, Mandy Chung wrote:
>> 
>>> On Dec 17, 2015, at 8:42 AM, Daniel Fuchs  wrote:
>>> 
>>> Hi,
>>> 
>>> Please find below a small enhancement for the method that infers
>>> the calling class name and method name in  SimpleConsoleLogger
>>> and LogRecord.
>>> 
>>> The idea is that since these methods now use the new StackWalker API
>>> they should take advantage of that in order to skip classes that
>>> implement System.Logger. This should make it possible for components
>>> that need it to easily wrap a System.Logger (and even a j.u.l.Logger).
>>> 
>>> 8145686: SimpleConsoleLogger and LogRecord should take advantage
>>> of StackWalker to skip classes implementing System.Logger
>>> https://bugs.openjdk.java.net/browse/JDK-8145686
>>> 
>>> webrev:
>>> http://cr.openjdk.java.net/~dfuchs/webrev_8145686/webrev.00/
>> 
>> What is the difference between skipLoggingFrame and isLoggerImplFrame?
> 
> isLoggingImplFrame stops when it finds the concrete logger implementation. It 
> will unstack all the LogRecord, Handler, Filter
> implementations etc... (which could be custom application classes)
> 
> skipLoggingFrame will skip everything above the first method found
> on the concrete Logger implementation until it reaches something that
> looks like a calling frame. In particular it should skip other
> methods of the logger implementation (e.g. log() calls log(LogRecord),
> default interface methods provided by the logging framework,
> reflection/method handle frames etc...
> In particular it should skip any Logger wrapper classes.
> 

It’d be helpful to add some comment in the skipLoggingFrame method.  Maybe even 
better to change this method to isFilteredFrame??  Reflection frames and lambda 
forms should already be skipped by StackWalker.  The remaining to skip are 
method handles, doPrivileged, other logging internal (do you have the case that 
the logging internal are called via reflection and is that why they are 
included here)?  Looks like it’s good to check if this list should be cleaned 
up.

Should Formatting::skipLoggingFrame to return true if 
System.Logger.class.isAssignableFrom(t.getDeclaringClass()) so that that check 
doesn’t need to be duplicated in both SimpleConsoleLogger and LogRecord?

 422 if (cname.startsWith("java.lang.System$Logger"))   
return true;

This line can be removed.

> For instance, RMI has a logger that wraps a j.u.l.Logger so
> ideally that should be skipped too...
> JMX remote has a ClassLogger that should also be skipped.
> etc…


That might be the reason why skipLoggingFrame filters sun.rmi.runtime.Log?  Is 
this necessary?  This white list needs to be maintained manually which is 
fragile.  


186 @Override public StackWalker run() {

- Nit: separate @Override into a separate line

Mandy

RFR (JAXP): 8144967: javax.xml.transform.Source and org.xml.sax.InputSource can be empty

[huizhe wang]

Hi,

Adding a convenient method isEmpty to javax.xml.transform.Source and 
org.xml.sax.InputSource.


JBS: https://bugs.openjdk.java.net/browse/JDK-8144967
webrev: http://cr.openjdk.java.net/~joehw/jdk9/8144967/webrev/

Thanks,
Joe