Re: RFR: jsr166 jdk integration 2018-05

[Stuart Marks]

I owe all of you replies on this, but I don't have time to continue the 
discussion, and I suspect you all want to get on with things.


Let me have a quick chat with Paul tomorrow and then I'll propose a path 
forward.

s'marks

Re: [11] RFR: 8202088: Japanese new era implementation

[Stephen Colebourne]

The java.time change seems OK to me
Stephen

On 17 May 2018 at 21:31, Naoto Sato  wrote:
> Hi,
>
> Please review the changes to the subject issue:
>
> https://bugs.openjdk.java.net/browse/JDK-8202088
>
> The proposed change is located at:
>
> http://cr.openjdk.java.net/~naoto/8202088/webrev.01/
>
> This is the implementation part of the upcoming Japanese new era, starting
> from May 1st, 2019. Current anticipation is that the new name won't be known
> till one month prior to the beginning of the era. So it's not possible to
> make changes to the JDK with the proper name. Instead, here we are going to
> implement the new era with a place holder name which will be immediately
> replaced with the proper name once it's known. The CSR is currently under
> review:
>
> https://bugs.openjdk.java.net/browse/JDK-8202336
>
> Naoto

Re: RFR: JDK-8200436 - String::isBlank

[Jim Laskey]

“isBlank" was primarily chosen because isBlank has been the name used in Apache 
StringUtils for the last fifteen years. The fact that Excel and Kotlin use 
isBlank as well might have influenced the choice. Not many other languages have 
the isBlank concept, usually requiring several method calls to gather the same 
effect. Secondarily, isBlank was chosen because of brevity, in the same vein as 
isEmpty.

The primary use case I see for the predicate is to filter out “blank” lines in 
streams of strings; eliminate non-informational data. As a predicate, isBlank 
seems very apropos. The name reflects its purpose and not focused on how it is 
implemented.

Cheers,

— Jim


> On May 16, 2018, at 1:43 AM, Jeremy Manson  wrote:
> 
> Blank can even mean U+2422, if it comes to that.
> 
> Jeremy
> 
> On Tue, May 15, 2018 at 12:58 PM Louis Wasserman 
> wrote:
> 
>> Does this method offer significant benefits over the more general approach
>> of String.codePoints().allMatch(Character::isWhitespace)?
>> 
>> " there is a notion of all characters in > > isBlank that you do not have
>> with isWhitespace."
>> I'm not sure I follow.  I do tend to interpret "is this string whitespace?"
>> as implying that it consists only of whitespace, whereas "blank" could mean
>> "" or maybe even "___" but not obviously "   ".
>> 
>> On Tue, May 15, 2018 at 11:19 AM Jonathan Bluett-Duncan <
>> jbluettdun...@gmail.com> wrote:
>> 
>>> How about String.isWhitespaceOrEmpty? I think that accurately describes
>> the
>>> method.
>>> 
>>> Cheers,
>>> Jonathan
>>> 
>>> On 15 May 2018 at 19:15, Jeremy Manson  wrote:
>>> 
 Seems to me that consistency matters here: for Character to call it
 whitespace and for String to call it blank is a little weird.
 
 Is there a well-understood definition of "blank string", or did this
>> just
 seem like a good name choice?
 
 Jeremy
 
 On Mon, May 14, 2018 at 11:14 PM Remi Forax  wrote:
 
> Hi Louis,
> I prefer isBlank to isWhitespace, there is a notion of all characters
>>> in
> isBlank that you do not have with isWhitespace.
> 
> Rémi
> 
> - Mail original -
>> De: "Louis Wasserman" 
>> À: "Xueming Shen" 
>> Cc: "core-libs-dev" 
>> Envoyé: Lundi 14 Mai 2018 22:15:50
>> Objet: Re: RFR: JDK-8200436 - String::isBlank
> 
>> It's not clear to me that "isBlank" is a good name for this method.
>> "isWhitespace" might be more appropriate, perhaps.
>> 
>> On Mon, May 14, 2018 at 12:48 PM Xueming Shen <
>>> xueming.s...@oracle.com
> 
>> wrote:
>> 
>>> +1
>>> 
>>> On 5/14/18, 8:25 AM, Jim Laskey wrote:
 New string instance method that returns true if the string is
>>> empty
 or
>>> contains only white space, where white space is defined as any
 codepoint
>>> returns true when passed to Character::isWhitespace.
 
 webrev:
>> http://cr.openjdk.java.net/~jlaskey/8200436/webrev/index.
 html
 jbs: https://bugs.openjdk.java.net/browse/JDK-8200436
 csr: https://bugs.openjdk.java.net/browse/JDK-8200437
 
>>> 
> 
 
>>> 
>> 

Re: RFR 8203279 : Faster calculation of power of two

[Ivan Gerasimov]

Thank you Claes for your review and the detailed analysis!


On 5/17/18 4:07 AM, Claes Redestad wrote:

Shouldn't this be called "Faster rounding up to nearest power of two"?


Yes, it's more accurate.

Patch looks OK to me, but I'd like to see numbers with the 
numberOfLeadingZeros intrinsic
disabled so that we ensure we're not incurring an unreasonable penalty 
on platforms who don't

have an intrinsic for this.

Running your benchmark with the intrinsic disabled[1] on my machine I 
see a 25-30% penalty
with testNew relative to testOld, which is perhaps a bit toomuch for 
comfort..


So I took a look at profiles for numberOfLeadingZeros with the 
intrinsic disabled and realized

it might be possible to improve:

diff -r de35abdfe5da src/java.base/share/classes/java/lang/Integer.java
--- a/src/java.base/share/classes/java/lang/Integer.java Mon May 14 
16:21:08 2018 +0200
+++ b/src/java.base/share/classes/java/lang/Integer.java Thu May 17 
12:44:53 2018 +0200

@@ -1621,12 +1621,12 @@
 // HD, Figure 5-6
 if (i <= 0)
 return i == 0 ? 32 : 0;
-int n = 1;
-if (i >>> 16 == 0) { n += 16; i <<= 16; }
-if (i >>> 24 == 0) { n +=  8; i <<=  8; }
-if (i >>> 28 == 0) { n +=  4; i <<=  4; }
-if (i >>> 30 == 0) { n +=  2; i <<=  2; }
-n -= i >>> 31;
+int n = 0;
+if (  i < 1 << 16) { n += 16; i <<= 16; }
+if (i >= 0 && i < 1 << 24) { n +=  8; i <<=  8; }
+if (i >= 0 && i < 1 << 28) { n +=  4; i <<=  4; }
+if (i >= 0 && i < 1 << 30) { n +=  2; i <<=  2; }
+if (i >= 0) n++;
 return n;
 }

Adding a case that uses this version to your benchmark[2] and the new 
version is only about
10% slower than the baseline, with the added benefit that other uses 
of numberOfLeadingZeros
might see a speed-upif there's no intrinsic (runs with intrinsic 
disabled[1]):



Interesting.
It can probably be done with fewer comparisons, if the direction of all 
the shifts is inverted:


The following variant showed slightly better performance on my machine:

static final int numberOfLeadingZeros(int i) {
if (i <= 0)
return i == 0 ? 32 : 0;
int n = 31;
if (i >= 1 << 16) { n -= 16; i >>>= 16; }
if (i >= 1 <<  8) { n -=  8; i >>>=  8; }
if (i >= 1 <<  4) { n -=  4; i >>>=  4; }
if (i >= 1 <<  2) { n -=  2; i >>>=  2; }
return n - (i >>> 1);
}

I agree that improving Java implementation of numberOfLeadingZeros() can 
be done as a separate RFE.


With kind regards,
Ivan



Benchmark  (arg)  Mode  Cnt   Score   Error Units
TableFor.testNew   1  avgt6  28.343 ± 0.534  ns/op
TableFor.testNew  42  avgt6  26.458 ± 0.064  ns/op
TableFor.testNew2  1  avgt6  23.969 ± 0.201  ns/op
TableFor.testNew2 42  avgt6  23.934 ± 0.107  ns/op
TableFor.testOld   1  avgt6  21.615 ± 0.803  ns/op
TableFor.testOld  42  avgt6  21.418 ± 0.106  ns/op

So I think with the above patch to Integer.numberOfLeadingZeros we can 
get the benefit for
our supported platforms while staying roughly performance neutral on 
platforms without

an intrinsic. Not strictly necessary to fold it into this RFE, of course.

Thanks!

/Claes

[1] -XX:+UnlockDiagnosticVMOptions 
-XX:DisableIntrinsic=_numberOfLeadingZeros_i

[2] http://cr.openjdk.java.net/~redestad/scratch/TableFor.java

On 2018-05-17 05:48, David Holmes wrote:

Do you think it's good to go?


I think I'd rather defer to a more performance oriented reviewer - 
paging Claes! ;-)


David
- 




--
With kind regards,
Ivan Gerasimov

Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit

[Peter Levart]

Hi Tony,

If we anticipate only small number of JdkThreadLocal(s) (there will be 
only one for the start) then this is a plausible solution. Then perhaps 
this limitation should be written somewhere in the JdkThreadLocal 
javadoc so that one day somebody will not be tempted to use 
JdkThreadLocal(s) en masse. Let's say there will be a few more 
JdkThreadLocal(s) in the future. Are we willing to pay for a few lookups 
into a ThreadLocalMap at each and every thread's exit even though such 
thread did not register a mapping for any JdkThreadLocal? Is an 
additional reference field in each and every ThreadLocalMap (one per 
Thread that uses thread locals) a bigger price to pay? I don't know. 
Will let others comment on this.


Otherwise the code looks good. Just a couple of observations:

Since private static method JdkThreadLocal.add is only called from 
JdkThreadLocal constructor with just constructed instance (this), 
there's no possibility for it to be called twice or more times with the 
same instance. The check for duplicates could go away then, right?


You keep an array of Entry objects which are just wrappers for 
JdkThreadLocal objects. Are you already planning for Entry to become a 
WeakReference? Otherwise you could just keep JdkThreadLocal objects in 
the array directly.


Regards, Peter

On 05/17/18 20:25, Tony Printezis wrote:

Hi all,

I have a new version of the code for your consideration:

http://cr.openjdk.java.net/~tonyp/8202788/webrev.1/ 



I basically combined our two approaches. The usage is as Alan had 
proposed it: Users have to use JdkThreadLocal (which is only available 
within java.base) and override threadTerminated(). However, I keep 
track of JdkThreadLocal instances globally (as I did before) and not 
per-thread. This way we don’t need to add any unnecessary complexity 
to ThreadLocalMap.


Currently, I don’t allow entries to be purged if the JdkThreadLocal 
instance becomes otherwise unreachable. I can easily add that 
functionality if needed (I can use WeakReferences for that). However, 
for the uses we’re considering, is it really necessary?


Thoughts?

Tony


—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com 




On May 14, 2018 at 12:40:28 PM, Tony Printezis (tprinte...@twitter.com 
) wrote:



Peter,

In my proposal, you can register the exit hook in the ThreadLocal 
c’tor, so it’s almost as nice as Alan’s in that respect (and it 
doesn't require an extra field per ThreadLocal plus two extra fields 
per JdkEntry). :-)


But, I do like the addition of the JdkEntry list to avoid 
unnecessarily iterating over all the map entries (which was my main 
concern with Alan’s original webrev). I’ll be totally happy with a 
version of this.


Tony


—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com 




On May 12, 2018 at 6:44:08 AM, Peter Levart (peter.lev...@gmail.com 
) wrote:



Hi,

On 05/11/18 16:13, Alan Bateman wrote:

On 08/05/2018 16:07, Tony Printezis wrote:

Hi all,

Following the discussion on this a few weeks ago, here’s the first 
version

of the change:

http://cr.openjdk.java.net/~tonyp/8202788/webrev.0/

I think the consensus was that it’d be easier if the exit hooks 
were only
available within java.base. Is it enough that I added the 
functionality to
the jdk.internal.misc package? (And is jdk.internal.misc the best 
place for

this?)

I’ll also add a test for the new functionality. But let’s first 
come up

with an approach that everyone is happy with. :-)

Peter's approach in early April was clean (and we should come to 
the getIfPresent discussion) but it adds a field to Thread for the 
callback list. If I read your approach correctly, you are avoiding 
that by maintaining an array of hooks in ThreadLocalExitHooks.


Another approach to try is a java.base-internal ThreadLocal that 
defines a method to be invoked when a thread terminates. Something 
like the following:

http://cr.openjdk.java.net/~alanb/8202788/webrev/index.html

-Alan


From the API perspective, Alan's suggestion is the most attractive 
one as it puts the method to where it belongs - into the ThreadLocal 
instance. But the implementation could be improved a bit. I don't 
like the necessity to iterate over all ThreadLocal(s) to filter out 
JdkThreadLocal(s). There might be a situation where there's plenty 
of ThreadLocal(s) registered per exiting thread which would produce 
a spike in CPU processing at thread exit.


The way to avoid this would be to maintain a separate linked list of 
entries that contains just those with JdkThreadLocal(s). Like in 
this modification of Alan's patch, for example:


http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.01/

(Only ThreadLocal class is modified from Alan's patch)

What do you think?


Regards, Peter

[11] RFR: 8202088: Japanese new era implementation

[Naoto Sato]

Hi,

Please review the changes to the subject issue:

https://bugs.openjdk.java.net/browse/JDK-8202088

The proposed change is located at:

http://cr.openjdk.java.net/~naoto/8202088/webrev.01/

This is the implementation part of the upcoming Japanese new era, 
starting from May 1st, 2019. Current anticipation is that the new name 
won't be known till one month prior to the beginning of the era. So it's 
not possible to make changes to the JDK with the proper name. Instead, 
here we are going to implement the new era with a place holder name 
which will be immediately replaced with the proper name once it's known. 
The CSR is currently under review:


https://bugs.openjdk.java.net/browse/JDK-8202336

Naoto

Re: RFR: Small cleanups in java.lang.ref

[mark . reinhold]

2018/5/16 18:31:15 -0700, marti...@google.com:
> I've been confused by NULL vs null for years.

That’s because `NULL` in ReferenceQueue.java refers to the singleton
object `ReferenceQueue.NULL`, not the null value.  See the long comment
at the top of Reference.java for an explanation.

To improve this you could change mentions of `NULL` in the comments to
`ReferenceQueue.NULL`, but changing them to `null` would be incorrect.

> 8203327: Small cleanups in java.lang.ref
> http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
> https://bugs.openjdk.java.net/browse/JDK-8203327

The other changes look fine.

- Mark

Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit

[Tony Printezis]

Hi all,

I have a new version of the code for your consideration:

http://cr.openjdk.java.net/~tonyp/8202788/webrev.1/

I basically combined our two approaches. The usage is as Alan had proposed
it: Users have to use JdkThreadLocal (which is only available within
java.base) and override threadTerminated(). However, I keep track of
JdkThreadLocal instances globally (as I did before) and not per-thread.
This way we don’t need to add any unnecessary complexity to ThreadLocalMap.

Currently, I don’t allow entries to be purged if the JdkThreadLocal
instance becomes otherwise unreachable. I can easily add that functionality
if needed (I can use WeakReferences for that). However, for the uses we’re
considering, is it really necessary?

Thoughts?

Tony


—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com


On May 14, 2018 at 12:40:28 PM, Tony Printezis (tprinte...@twitter.com)
wrote:

Peter,

In my proposal, you can register the exit hook in the ThreadLocal c’tor, so
it’s almost as nice as Alan’s in that respect (and it doesn't require an
extra field per ThreadLocal plus two extra fields per JdkEntry). :-)

But, I do like the addition of the JdkEntry list to avoid unnecessarily
iterating over all the map entries (which was my main concern with Alan’s
original webrev). I’ll be totally happy with a version of this.

Tony


—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com


On May 12, 2018 at 6:44:08 AM, Peter Levart (peter.lev...@gmail.com) wrote:

Hi,

On 05/11/18 16:13, Alan Bateman wrote:

On 08/05/2018 16:07, Tony Printezis wrote:

Hi all,

Following the discussion on this a few weeks ago, here’s the first version
of the change:

http://cr.openjdk.java.net/~tonyp/8202788/webrev.0/

I think the consensus was that it’d be easier if the exit hooks were only
available within java.base. Is it enough that I added the functionality to
the jdk.internal.misc package? (And is jdk.internal.misc the best place for
this?)

I’ll also add a test for the new functionality. But let’s first come up
with an approach that everyone is happy with. :-)

Peter's approach in early April was clean (and we should come to the
getIfPresent discussion) but it adds a field to Thread for the callback
list. If I read your approach correctly, you are avoiding that by
maintaining an array of hooks in ThreadLocalExitHooks.

Another approach to try is a java.base-internal ThreadLocal that defines a
method to be invoked when a thread terminates. Something like the
following:
   http://cr.openjdk.java.net/~alanb/8202788/webrev/index.html

-Alan


>From the API perspective, Alan's suggestion is the most attractive one as
it puts the method to where it belongs - into the ThreadLocal instance. But
the implementation could be improved a bit. I don't like the necessity to
iterate over all ThreadLocal(s) to filter out JdkThreadLocal(s). There
might be a situation where there's plenty of ThreadLocal(s) registered per
exiting thread which would produce a spike in CPU processing at thread exit.

The way to avoid this would be to maintain a separate linked list of
entries that contains just those with JdkThreadLocal(s). Like in this
modification of Alan's patch, for example:

http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.01/

(Only ThreadLocal class is modified from Alan's patch)

What do you think?


Regards, Peter

Re: RFR 8203279 : Faster calculation of power of two

[Doug Lea]

On 05/17/2018 07:07 AM, Claes Redestad wrote:
> Shouldn't this be called "Faster rounding up to nearest power of two"?
> 
> Patch looks OK to me, but I'd like to see numbers with the
> numberOfLeadingZeros intrinsic
> disabled so that we ensure we're not incurring an unreasonable penalty
> on platforms who don't
> have an intrinsic for this.
> 
> Running your benchmark with the intrinsic disabled[1] on my machine I
> see a 25-30% penalty
> with testNew relative to testOld, which is perhaps a bit toomuch for
> comfort..

This was the original reason for using the existing code -- hardware
support was uncommon. I like your idea of tweaking non-intrinsic
numberOfLeadingZeros to reduce penalty on the now-uncommon platforms
without support.

-Doug


> 
> So I took a look at profiles for numberOfLeadingZeros with the intrinsic
> disabled and realized
> it might be possible to improve:
> 
> diff -r de35abdfe5da src/java.base/share/classes/java/lang/Integer.java
> --- a/src/java.base/share/classes/java/lang/Integer.java    Mon May
> 14 16:21:08 2018 +0200
> +++ b/src/java.base/share/classes/java/lang/Integer.java    Thu May
> 17 12:44:53 2018 +0200
> @@ -1621,12 +1621,12 @@
>  // HD, Figure 5-6
>  if (i <= 0)
>  return i == 0 ? 32 : 0;
> -    int n = 1;
> -    if (i >>> 16 == 0) { n += 16; i <<= 16; }
> -    if (i >>> 24 == 0) { n +=  8; i <<=  8; }
> -    if (i >>> 28 == 0) { n +=  4; i <<=  4; }
> -    if (i >>> 30 == 0) { n +=  2; i <<=  2; }
> -    n -= i >>> 31;
> +    int n = 0;
> +    if (  i < 1 << 16) { n += 16; i <<= 16; }
> +    if (i >= 0 && i < 1 << 24) { n +=  8; i <<=  8; }
> +    if (i >= 0 && i < 1 << 28) { n +=  4; i <<=  4; }
> +    if (i >= 0 && i < 1 << 30) { n +=  2; i <<=  2; }
> +    if (i >= 0) n++;
>  return n;
>  }
> 
> Adding a case that uses this version to your benchmark[2] and the new
> version is only about
> 10% slower than the baseline, with the added benefit that other uses of
> numberOfLeadingZeros
> might see a speed-upif there's no intrinsic (runs with intrinsic
> disabled[1]):
> 
> Benchmark  (arg)  Mode  Cnt   Score   Error Units
> TableFor.testNew   1  avgt    6  28.343 ± 0.534  ns/op
> TableFor.testNew  42  avgt    6  26.458 ± 0.064  ns/op
> TableFor.testNew2  1  avgt    6  23.969 ± 0.201  ns/op
> TableFor.testNew2 42  avgt    6  23.934 ± 0.107  ns/op
> TableFor.testOld       1  avgt    6  21.615 ± 0.803  ns/op
> TableFor.testOld      42  avgt    6  21.418 ± 0.106  ns/op
> 
> So I think with the above patch to Integer.numberOfLeadingZeros we can
> get the benefit for
> our supported platforms while staying roughly performance neutral on
> platforms without
> an intrinsic. Not strictly necessary to fold it into this RFE, of course.
> 
> Thanks!
> 
> /Claes
> 
> [1] -XX:+UnlockDiagnosticVMOptions
> -XX:DisableIntrinsic=_numberOfLeadingZeros_i
> [2] http://cr.openjdk.java.net/~redestad/scratch/TableFor.java
> 
> On 2018-05-17 05:48, David Holmes wrote:
>>> Do you think it's good to go?
>>
>> I think I'd rather defer to a more performance oriented reviewer -
>> paging Claes! ;-)
>>
>> David
>> - 
> 

Re: RFR 8203279 : Faster calculation of power of two

[Paul Sandoz]

> On May 17, 2018, at 4:07 AM, Claes Redestad  wrote:
> 
> Shouldn't this be called "Faster rounding up to nearest power of two"?
> 
> Patch looks OK to me, but I'd like to see numbers with the 
> numberOfLeadingZeros intrinsic
> disabled so that we ensure we're not incurring an unreasonable penalty on 
> platforms who don't
> have an intrinsic for this.
> 

I did a quick check and i believe it is intrinsic for C2 on all hotspot cpu 
platform code, zero does not count! (did not check Graal but i suspect it 
supports it too). 

Extra points for comparing the two approaches with C1 :-) I don’t think that is 
necessary but i am curious.

Paul.
 
> Running your benchmark with the intrinsic disabled[1] on my machine I see a 
> 25-30% penalty
> with testNew relative to testOld, which is perhaps a bit toomuch for comfort..
> 
> So I took a look at profiles for numberOfLeadingZeros with the intrinsic 
> disabled and realized
> it might be possible to improve:
> 
> diff -r de35abdfe5da src/java.base/share/classes/java/lang/Integer.java
> --- a/src/java.base/share/classes/java/lang/Integer.javaMon May 14 
> 16:21:08 2018 +0200
> +++ b/src/java.base/share/classes/java/lang/Integer.javaThu May 17 
> 12:44:53 2018 +0200
> @@ -1621,12 +1621,12 @@
>  // HD, Figure 5-6
>  if (i <= 0)
>  return i == 0 ? 32 : 0;
> -int n = 1;
> -if (i >>> 16 == 0) { n += 16; i <<= 16; }
> -if (i >>> 24 == 0) { n +=  8; i <<=  8; }
> -if (i >>> 28 == 0) { n +=  4; i <<=  4; }
> -if (i >>> 30 == 0) { n +=  2; i <<=  2; }
> -n -= i >>> 31;
> +int n = 0;
> +if (  i < 1 << 16) { n += 16; i <<= 16; }
> +if (i >= 0 && i < 1 << 24) { n +=  8; i <<=  8; }
> +if (i >= 0 && i < 1 << 28) { n +=  4; i <<=  4; }
> +if (i >= 0 && i < 1 << 30) { n +=  2; i <<=  2; }
> +if (i >= 0) n++;
>  return n;
>  }
> 
> Adding a case that uses this version to your benchmark[2] and the new version 
> is only about
> 10% slower than the baseline, with the added benefit that other uses of 
> numberOfLeadingZeros
> might see a speed-upif there's no intrinsic (runs with intrinsic disabled[1]):
> 
> Benchmark  (arg)  Mode  Cnt   Score   Error Units
> TableFor.testNew   1  avgt6  28.343 ± 0.534  ns/op
> TableFor.testNew  42  avgt6  26.458 ± 0.064  ns/op
> TableFor.testNew2  1  avgt6  23.969 ± 0.201  ns/op
> TableFor.testNew2 42  avgt6  23.934 ± 0.107  ns/op
> TableFor.testOld   1  avgt6  21.615 ± 0.803  ns/op
> TableFor.testOld  42  avgt6  21.418 ± 0.106  ns/op
> 
> So I think with the above patch to Integer.numberOfLeadingZeros we can get 
> the benefit for
> our supported platforms while staying roughly performance neutral on 
> platforms without
> an intrinsic. Not strictly necessary to fold it into this RFE, of course.
> 
> Thanks!
> 
> /Claes
> 
> [1] -XX:+UnlockDiagnosticVMOptions 
> -XX:DisableIntrinsic=_numberOfLeadingZeros_i
> [2] http://cr.openjdk.java.net/~redestad/scratch/TableFor.java
> 
> On 2018-05-17 05:48, David Holmes wrote:
>>> Do you think it's good to go?
>> 
>> I think I'd rather defer to a more performance oriented reviewer - paging 
>> Claes! ;-)
>> 
>> David
>> - 
> 

Re: RFR: Rename EFS in java.util.zip internals to something meaningful

[Martin Buchholz]

Yup, I missed FLAG_EFS.

diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipConstants.java
b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipConstants.java
--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipConstants.java
+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipConstants.java
@@ -47,7 +47,7 @@ class ZipConstants {
  */
 static final int FLAG_ENCRYPTED  = 0x01;
 static final int FLAG_DATADESCR  = 0x08;// crc, size and csize in
dd
-static final int FLAG_EFS= 0x800;   // If this bit is set the
filename and
+static final int FLAG_USE_UTF8   = 0x800;   // If this bit is set the
filename and
 // comment fields for this
file must be
 // encoded using UTF-8.
 /*
diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
@@ -1374,7 +1374,7 @@ class ZipFileSystem extends FileSystem {
 // store size, compressed size, and crc-32 in LOC header
 e.flag = 0;
 if (zc.isUTF8())
-e.flag |= FLAG_EFS;
+e.flag |= FLAG_USE_UTF8;
 OutputStream os;
 if (useTempFile) {
 e.file = getTempPathForEntry(null);


On Wed, May 16, 2018 at 11:13 PM, Xueming Shen 
wrote:

> On 5/16/18, 6:28 PM, Martin Buchholz wrote:
>
>> Hi Xueming, I'd like you to do a code review
>>
>> 8203328: Rename EFS in java.util.zip internals to something meaningful
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/zip-EFS/ <
>> http://cr.openjdk.java.net/%7Emartin/webrevs/jdk/zip-EFS/>
>> https://bugs.openjdk.java.net/browse/JDK-8203328
>>
>> looks good. thanks for fixing the misleading name i put in 10 years ago
> :-)
> btw, you might as well want to fix the same constant in  zipfs at
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipConstands.java/FLAG_EFS
>

Re: RFR 8203279 : Faster calculation of power of two

[David Holmes]

On 17/05/2018 9:24 PM, Claes Redestad wrote:

On 2018-05-17 13:15, David Holmes wrote:

diff -r de35abdfe5da src/java.base/share/classes/java/lang/Integer.java
--- a/src/java.base/share/classes/java/lang/Integer.java Mon May 14 
16:21:08 2018 +0200
+++ b/src/java.base/share/classes/java/lang/Integer.java Thu May 17 
12:44:53 2018 +0200

@@ -1621,12 +1621,12 @@
  // HD, Figure 5-6


The code no longer maps to that from HD so the comment would need fixing.


Seems that's a pre-existing issue, but yes.


The comment is correct for the first edition of the book. It's 5-11 in 
the second edition. :)


David


/Claes

Re: RFR 8203279 : Faster calculation of power of two

[Claes Redestad]

On 2018-05-17 13:15, David Holmes wrote:

diff -r de35abdfe5da src/java.base/share/classes/java/lang/Integer.java
--- a/src/java.base/share/classes/java/lang/Integer.java Mon May 14 
16:21:08 2018 +0200
+++ b/src/java.base/share/classes/java/lang/Integer.java Thu May 17 
12:44:53 2018 +0200

@@ -1621,12 +1621,12 @@
  // HD, Figure 5-6


The code no longer maps to that from HD so the comment would need fixing.


Seems that's a pre-existing issue, but yes.

/Claes

Re: RFR 8203279 : Faster calculation of power of two

[David Holmes]

On 17/05/2018 9:07 PM, Claes Redestad wrote:

Shouldn't this be called "Faster rounding up to nearest power of two"?

Patch looks OK to me, but I'd like to see numbers with the 
numberOfLeadingZeros intrinsic
disabled so that we ensure we're not incurring an unreasonable penalty 
on platforms who don't

have an intrinsic for this.

Running your benchmark with the intrinsic disabled[1] on my machine I 
see a 25-30% penalty
with testNew relative to testOld, which is perhaps a bit toomuch for 
comfort..


So I took a look at profiles for numberOfLeadingZeros with the intrinsic 
disabled and realized

it might be possible to improve:

diff -r de35abdfe5da src/java.base/share/classes/java/lang/Integer.java
--- a/src/java.base/share/classes/java/lang/Integer.java    Mon May 
14 16:21:08 2018 +0200
+++ b/src/java.base/share/classes/java/lang/Integer.java    Thu May 
17 12:44:53 2018 +0200

@@ -1621,12 +1621,12 @@
  // HD, Figure 5-6


The code no longer maps to that from HD so the comment would need fixing.

David
-


  if (i <= 0)
  return i == 0 ? 32 : 0;
-    int n = 1;
-    if (i >>> 16 == 0) { n += 16; i <<= 16; }
-    if (i >>> 24 == 0) { n +=  8; i <<=  8; }
-    if (i >>> 28 == 0) { n +=  4; i <<=  4; }
-    if (i >>> 30 == 0) { n +=  2; i <<=  2; }
-    n -= i >>> 31;
+    int n = 0;
+    if (  i < 1 << 16) { n += 16; i <<= 16; }
+    if (i >= 0 && i < 1 << 24) { n +=  8; i <<=  8; }
+    if (i >= 0 && i < 1 << 28) { n +=  4; i <<=  4; }
+    if (i >= 0 && i < 1 << 30) { n +=  2; i <<=  2; }
+    if (i >= 0) n++;
  return n;
  }

Adding a case that uses this version to your benchmark[2] and the new 
version is only about
10% slower than the baseline, with the added benefit that other uses of 
numberOfLeadingZeros
might see a speed-upif there's no intrinsic (runs with intrinsic 
disabled[1]):


Benchmark  (arg)  Mode  Cnt   Score   Error Units
TableFor.testNew   1  avgt    6  28.343 ± 0.534  ns/op
TableFor.testNew  42  avgt    6  26.458 ± 0.064  ns/op
TableFor.testNew2  1  avgt    6  23.969 ± 0.201  ns/op
TableFor.testNew2 42  avgt    6  23.934 ± 0.107  ns/op
TableFor.testOld       1  avgt    6  21.615 ± 0.803  ns/op
TableFor.testOld      42  avgt    6  21.418 ± 0.106  ns/op

So I think with the above patch to Integer.numberOfLeadingZeros we can 
get the benefit for
our supported platforms while staying roughly performance neutral on 
platforms without

an intrinsic. Not strictly necessary to fold it into this RFE, of course.

Thanks!

/Claes

[1] -XX:+UnlockDiagnosticVMOptions 
-XX:DisableIntrinsic=_numberOfLeadingZeros_i

[2] http://cr.openjdk.java.net/~redestad/scratch/TableFor.java

On 2018-05-17 05:48, David Holmes wrote:

Do you think it's good to go?


I think I'd rather defer to a more performance oriented reviewer - 
paging Claes! ;-)


David
- 

Re: RFR 8203279 : Faster calculation of power of two

[Claes Redestad]

Shouldn't this be called "Faster rounding up to nearest power of two"?

Patch looks OK to me, but I'd like to see numbers with the 
numberOfLeadingZeros intrinsic
disabled so that we ensure we're not incurring an unreasonable penalty 
on platforms who don't

have an intrinsic for this.

Running your benchmark with the intrinsic disabled[1] on my machine I 
see a 25-30% penalty
with testNew relative to testOld, which is perhaps a bit toomuch for 
comfort..


So I took a look at profiles for numberOfLeadingZeros with the intrinsic 
disabled and realized

it might be possible to improve:

diff -r de35abdfe5da src/java.base/share/classes/java/lang/Integer.java
--- a/src/java.base/share/classes/java/lang/Integer.java    Mon May 
14 16:21:08 2018 +0200
+++ b/src/java.base/share/classes/java/lang/Integer.java    Thu May 
17 12:44:53 2018 +0200

@@ -1621,12 +1621,12 @@
 // HD, Figure 5-6
 if (i <= 0)
 return i == 0 ? 32 : 0;
-    int n = 1;
-    if (i >>> 16 == 0) { n += 16; i <<= 16; }
-    if (i >>> 24 == 0) { n +=  8; i <<=  8; }
-    if (i >>> 28 == 0) { n +=  4; i <<=  4; }
-    if (i >>> 30 == 0) { n +=  2; i <<=  2; }
-    n -= i >>> 31;
+    int n = 0;
+    if (  i < 1 << 16) { n += 16; i <<= 16; }
+    if (i >= 0 && i < 1 << 24) { n +=  8; i <<=  8; }
+    if (i >= 0 && i < 1 << 28) { n +=  4; i <<=  4; }
+    if (i >= 0 && i < 1 << 30) { n +=  2; i <<=  2; }
+    if (i >= 0) n++;
 return n;
 }

Adding a case that uses this version to your benchmark[2] and the new 
version is only about
10% slower than the baseline, with the added benefit that other uses of 
numberOfLeadingZeros
might see a speed-upif there's no intrinsic (runs with intrinsic 
disabled[1]):


Benchmark  (arg)  Mode  Cnt   Score   Error Units
TableFor.testNew   1  avgt    6  28.343 ± 0.534  ns/op
TableFor.testNew  42  avgt    6  26.458 ± 0.064  ns/op
TableFor.testNew2  1  avgt    6  23.969 ± 0.201  ns/op
TableFor.testNew2 42  avgt    6  23.934 ± 0.107  ns/op
TableFor.testOld       1  avgt    6  21.615 ± 0.803  ns/op
TableFor.testOld      42  avgt    6  21.418 ± 0.106  ns/op

So I think with the above patch to Integer.numberOfLeadingZeros we can 
get the benefit for
our supported platforms while staying roughly performance neutral on 
platforms without

an intrinsic. Not strictly necessary to fold it into this RFE, of course.

Thanks!

/Claes

[1] -XX:+UnlockDiagnosticVMOptions 
-XX:DisableIntrinsic=_numberOfLeadingZeros_i

[2] http://cr.openjdk.java.net/~redestad/scratch/TableFor.java

On 2018-05-17 05:48, David Holmes wrote:

Do you think it's good to go?


I think I'd rather defer to a more performance oriented reviewer - 
paging Claes! ;-)


David
- 

Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[David Holmes]

Clarification ...

On 17/05/2018 6:37 PM, David Holmes wrote:

Hi Remi,

On 17/05/2018 6:16 PM, Remi Forax wrote:

Hi all,

- Mail original -

De: "Alan Bateman" 
À: "David Holmes" , "core-libs-dev" 


Envoyé: Mardi 15 Mai 2018 15:53:44
Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: 
Nest-Based Access Control



On 15/05/2018 01:52, David Holmes wrote:

This review is being spread across four groups: langtools, core-libs,
hotspot and serviceability. This is the specific review thread for
core-libs - webrev:

http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/


[...]


Maybe a question for Kumar but are we planning to pull in any ASM
updates for JDK 11? NestMembers extends Attribute looks okay, I'm less
sure about the change to ClassReader as I don't know if there is
somewhere else in ASM that has the list of attributes to always parse.


With my ASM hat,
the current master of ASM (the release of ASM 6.2 is scheduled for the 
next week-end) already supports nestmates (and constant dynamic and 
preview feature) so i suppose that at some point in the future Kumar 
will merge it to the JDK.


Unfortunately Kumar is no longer with us.


By which I simply mean he is no longer at Oracle.

David
-

We have recently changed the way we implement features in ASM, instead 
of having features lingering in different branches, we now integrate 
them directly in the master under an experimental flag 
(ASM7_EXPERIMENTAL), which means for the JDK that it is no longer 
necessary to wait until the release of ASM 7 because it can use the 
experimental support of ASM 6.2.
(note that experimental doesn't mean full of bugs, or half baked or 
anything like this, it means that the feature is not yet integrated in 
a released JDK).


I've taking a look to the code in this patch, i've two comments,
- in Attributes, it seems that the code store the bytecode slice 
corresponding to the attribute only to use its length as argument of 
the ByteVector which is like an ArrayList of byte, it grows 
automatically so the initial capacity is a perf optimization. Perhaps 
the byte array is used somewhere else ?
- patching the ClassReader.accept is really a quick hack because the 
method accept with 3 arguments is not patched so if this method is 
called somewhere in the JDK it will behave as it should.


I'll take a look at this. To be honest I don't even remember who 
provided those changes ... I thought you had provided feedback at some 
point in the past :) There's a valhalla-dev email with a link that's no 
longer valid:


https://gitlab.ow2.org/asm/asm/tree/NEST_MATES

Thanks,
David


[...]



-Alan.


Rémi

Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[forax]

- Mail original -
> De: "David Holmes" 
> À: "Remi Forax" , "Alan Bateman" 
> Cc: "core-libs-dev" 
> Envoyé: Jeudi 17 Mai 2018 10:37:46
> Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: 
> Nest-Based Access Control

> Hi Remi,

Hi David

> 
> On 17/05/2018 6:16 PM, Remi Forax wrote:
>> Hi all,
>> 
>> - Mail original -
>>> De: "Alan Bateman" 
>>> À: "David Holmes" , "core-libs-dev"
>>> 
>>> Envoyé: Mardi 15 Mai 2018 15:53:44
>>> Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: 
>>> Nest-Based
>>> Access Control
>> 
>>> On 15/05/2018 01:52, David Holmes wrote:
 This review is being spread across four groups: langtools, core-libs,
 hotspot and serviceability. This is the specific review thread for
 core-libs - webrev:

 http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
>> 
>> [...]
>> 
>>> Maybe a question for Kumar but are we planning to pull in any ASM
>>> updates for JDK 11? NestMembers extends Attribute looks okay, I'm less
>>> sure about the change to ClassReader as I don't know if there is
>>> somewhere else in ASM that has the list of attributes to always parse.
>> 
>> With my ASM hat,
>> the current master of ASM (the release of ASM 6.2 is scheduled for the next
>> week-end) already supports nestmates (and constant dynamic and preview 
>> feature)
>> so i suppose that at some point in the future Kumar will merge it to the JDK.
> 
> Unfortunately Kumar is no longer with us.

I will miss him.

> 
>> We have recently changed the way we implement features in ASM, instead of 
>> having
>> features lingering in different branches, we now integrate them directly in 
>> the
>> master under an experimental flag (ASM7_EXPERIMENTAL), which means for the 
>> JDK
>> that it is no longer necessary to wait until the release of ASM 7 because it
>> can use the experimental support of ASM 6.2.
>> (note that experimental doesn't mean full of bugs, or half baked or anything
>> like this, it means that the feature is not yet integrated in a released 
>> JDK).
>> 
>> I've taking a look to the code in this patch, i've two comments,
>> - in Attributes, it seems that the code store the bytecode slice 
>> corresponding
>> to the attribute only to use its length as argument of the ByteVector which 
>> is
>> like an ArrayList of byte, it grows automatically so the initial capacity is 
>> a
>> perf optimization. Perhaps the byte array is used somewhere else ?
>> - patching the ClassReader.accept is really a quick hack because the method
>> accept with 3 arguments is not patched so if this method is called somewhere 
>> in
>> the JDK it will behave as it should.
> 
> I'll take a look at this. To be honest I don't even remember who
> provided those changes ... I thought you had provided feedback at some
> point in the past :) 

yes, i may have, i do not remember.

> There's a valhalla-dev email with a link that's no
> longer valid:
> 
> https://gitlab.ow2.org/asm/asm/tree/NEST_MATES

as i said above, the support has been integrated in the master of ASM,
see 
https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/asm/ClassVisitor.java#L156
and 
https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/asm/ClassVisitor.java#L246
 
the current code is fine, just not optimal, but given that when the JDK will 
use ASM 6.2, this code will have to be removed, i do not think you should spend 
some time one this.

> 
> Thanks,
> David

regards,
Rémi

Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[David Holmes]

Paul, Alan,

Just a quick thank you for taking a look at this so soon. I will respond 
to both of you as soon as practical.


Thanks,
David

On 17/05/2018 4:00 AM, Paul Sandoz wrote:

HI,

Nice thorough work on this, surprisingly tricky in some areas esp. MHs.

Class
—

3857  * If there is any error accessing the nest host, or the nest host 
is
3858  * in any way invalid, then {@code this} is returned.

I am curious under what conditions this can arise. As a caller this makes me 
nervous :-) Are there conditions that are worth calling it. This is related to 
Alan’s comment about primitive or array classes. Its clearer when looking at 
the implementation: primitive/array classes, linkage error, and the more 
general nest membership validation.


3883  * @throws SecurityException
3884  * If the returned class is not the current class, and
3885  * if a security manager, s, is present and the caller's
3886  * class loader is not the same as or an ancestor of the class
3887  * loader for the returned class and invocation of {@link
3888  * SecurityManager#checkPackageAccess s.checkPackageAccess()}
3889  * denies access to the package of the returned class
3890  * @since 11
3891  * @jvms 4.7.28 and 4.7.29 NestHost and NestMembers attributes
3892  */
3893 @CallerSensitive
3894 public Class getNestHost() {

Maybe i need more coffee, but I am struggling to see in the implementation the 
checks for the case of "and the caller’s class loader is not the same as or an 
ancestor of the class loader for the returned class”. Is it implied that all classes 
in the nest have to be loaded from the same or from a common ancestor class loader? 
so you only need to check one class in the nest against the calling class.


3984 public Class[] getNestMembers() {

I still think not removing dups is a mistake as it could be a source of subtle 
bugs. But i doubt at this point i can persuade you or others to change it :-)


3989 // Can't actually enable this due to bootstrapping issues
3990 // assert(members.length != 1 || members[0] == this); // expected 
invariant from VM

That's interesting and frustrating!


Reflection.java
—

  146 // Check for nestmate access if member is private
  147 if (Modifier.isPrivate(modifiers)) {
  148   // assert: isSubclassof(targetClass, memberClass)
  149   // Note: targetClass may be outside the nest, but that is okay
  150   //   as long as memberClass is in the nest.
  151   boolean nestmates = areNestMates(currentClass, memberClass);
  152   if (nestmates) {
  153 return true;
  154   }
  155 }

Trivially, you don’t need the local variable “nestmates”.


VerifyAccess.java
—

  134 case PRIVATE:
  135 // Rules for privates follows access rules for nestmates.
  136 boolean canAccess = ((allowedModes & PRIVATE) != 0 &&
  137  Reflection.areNestMates(defc, 
lookupClass));
  138 // FIX ME: Sanity check refc == defc. Either remove or 
convert to
  139 // plain assert before integration.
  140 myassert((canAccess && refc == defc) || !canAccess);
  141 return canAccess;
  142 default:
  143 throw new IllegalArgumentException("bad modifiers: 
"+Modifier.toString(mods));
  144 }
  145 }
  146 static void myassert(boolean cond) {
  147 if (!cond) throw new Error("Assertion failed");
  148 }

Do you plan to chase up the FIX ME now or later?


I agree with Alan about the current location of the reflection API tests. If 
these tests don’t need to be hammered with various and exotic HotSpot flags i 
think they are better placed to be under test/jdk/java/lang/reflect.


Thanks,
Paul.




On May 14, 2018, at 5:52 PM, David Holmes  wrote:

This review is being spread across four groups: langtools, core-libs, hotspot 
and serviceability. This is the specific review thread for core-libs - webrev:

http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/

See below for full details - including annotated full webrev guiding the review.

The intent is to have JEP-181 targeted and integrated by the end of this month.

Thanks,
David
-

The nestmates project (JEP-181) introduces new classfile attributes to identify 
classes and interfaces in the same nest, so that the VM can perform access 
control based on those attributes and so allow direct private access between 
nestmates without requiring javac to generate synthetic accessor methods. These 
access control changes also extend to core reflection and the 
MethodHandle.Lookup contexts.

Direct private calls between nestmates requires a more general calling context 
than is permitted by invokespecial, and so the JVMS is updated to allow, and 
javac updated to 

Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[David Holmes]

Hi Remi,

On 17/05/2018 6:16 PM, Remi Forax wrote:

Hi all,

- Mail original -

De: "Alan Bateman" 
À: "David Holmes" , "core-libs-dev" 

Envoyé: Mardi 15 Mai 2018 15:53:44
Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based 
Access Control



On 15/05/2018 01:52, David Holmes wrote:

This review is being spread across four groups: langtools, core-libs,
hotspot and serviceability. This is the specific review thread for
core-libs - webrev:

http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/


[...]


Maybe a question for Kumar but are we planning to pull in any ASM
updates for JDK 11? NestMembers extends Attribute looks okay, I'm less
sure about the change to ClassReader as I don't know if there is
somewhere else in ASM that has the list of attributes to always parse.


With my ASM hat,
the current master of ASM (the release of ASM 6.2 is scheduled for the next 
week-end) already supports nestmates (and constant dynamic and preview feature) 
so i suppose that at some point in the future Kumar will merge it to the JDK.


Unfortunately Kumar is no longer with us.


We have recently changed the way we implement features in ASM, instead of 
having features lingering in different branches, we now integrate them directly 
in the master under an experimental flag (ASM7_EXPERIMENTAL), which means for 
the JDK that it is no longer necessary to wait until the release of ASM 7 
because it can use the experimental support of ASM 6.2.
(note that experimental doesn't mean full of bugs, or half baked or anything 
like this, it means that the feature is not yet integrated in a released JDK).

I've taking a look to the code in this patch, i've two comments,
- in Attributes, it seems that the code store the bytecode slice corresponding 
to the attribute only to use its length as argument of the ByteVector which is 
like an ArrayList of byte, it grows automatically so the initial capacity is a 
perf optimization. Perhaps the byte array is used somewhere else ?
- patching the ClassReader.accept is really a quick hack because the method 
accept with 3 arguments is not patched so if this method is called somewhere in 
the JDK it will behave as it should.


I'll take a look at this. To be honest I don't even remember who 
provided those changes ... I thought you had provided feedback at some 
point in the past :) There's a valhalla-dev email with a link that's no 
longer valid:


https://gitlab.ow2.org/asm/asm/tree/NEST_MATES

Thanks,
David


[...]



-Alan.


Rémi

Re: Bug Request: Please remove out-of-date files from bug

[Adam Farley8]

Hi David,

Good catch. Copying to the Hotspot list.

Best Regards

Adam Farley 
OpenJDK Team 
Runtimes 
IBM 




From:   David Holmes 
To: Adam Farley8 , core-libs-dev 

Date:   16/05/2018 22:32
Subject:Re: Bug Request: Please remove out-of-date files from bug



Done.

Though you sent this to the wrong mailing list for a hotspot bug.

David

On 17/05/2018 1:36 AM, Adam Farley8 wrote:
> Hi All,
> 
> In bug JDK-8190187, hotspot_hg_diff and jdk_hg_diff are out-of-date.
> 
> Please can someone delete these files?
> 
> Best Regards
> 
> Adam Farley
> OpenJDK Team
> Runtimes
> IBM
> 
> 
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number
> 741598.
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 
3AU
> 





Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[Remi Forax]

Hi all,

- Mail original -
> De: "Alan Bateman" 
> À: "David Holmes" , "core-libs-dev" 
> 
> Envoyé: Mardi 15 Mai 2018 15:53:44
> Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: 
> Nest-Based Access Control

> On 15/05/2018 01:52, David Holmes wrote:
>> This review is being spread across four groups: langtools, core-libs,
>> hotspot and serviceability. This is the specific review thread for
>> core-libs - webrev:
>>
>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/

[...]

> Maybe a question for Kumar but are we planning to pull in any ASM
> updates for JDK 11? NestMembers extends Attribute looks okay, I'm less
> sure about the change to ClassReader as I don't know if there is
> somewhere else in ASM that has the list of attributes to always parse.

With my ASM hat,
the current master of ASM (the release of ASM 6.2 is scheduled for the next 
week-end) already supports nestmates (and constant dynamic and preview feature) 
so i suppose that at some point in the future Kumar will merge it to the JDK.

We have recently changed the way we implement features in ASM, instead of 
having features lingering in different branches, we now integrate them directly 
in the master under an experimental flag (ASM7_EXPERIMENTAL), which means for 
the JDK that it is no longer necessary to wait until the release of ASM 7 
because it can use the experimental support of ASM 6.2.
(note that experimental doesn't mean full of bugs, or half baked or anything 
like this, it means that the feature is not yet integrated in a released JDK).

I've taking a look to the code in this patch, i've two comments,
- in Attributes, it seems that the code store the bytecode slice corresponding 
to the attribute only to use its length as argument of the ByteVector which is 
like an ArrayList of byte, it grows automatically so the initial capacity is a 
perf optimization. Perhaps the byte array is used somewhere else ? 
- patching the ClassReader.accept is really a quick hack because the method 
accept with 3 arguments is not patched so if this method is called somewhere in 
the JDK it will behave as it should.
  
[...]

> 
> -Alan.

Rémi

Re: RFR: Rename EFS in java.util.zip internals to something meaningful

[Xueming Shen]

On 5/16/18, 6:28 PM, Martin Buchholz wrote:

Hi Xueming, I'd like you to do a code review

8203328: Rename EFS in java.util.zip internals to something meaningful
http://cr.openjdk.java.net/~martin/webrevs/jdk/zip-EFS/ 


https://bugs.openjdk.java.net/browse/JDK-8203328


looks good. thanks for fixing the misleading name i put in 10 years ago :-)
btw, you might as well want to fix the same constant in  zipfs at
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipConstands.java/FLAG_EFS