RFR 8229958: Provider.getService() scalability issue for legacy algorithms and message digests

[Yang, Letu]

Hi,

Please review the fix of https://bugs.openjdk.java.net/browse/JDK-8229958 where 
I made the change to allow majority of calls don't have to acquire the locks 
when checking the availability of the Provider object. Similar effort had been 
made in fixing https://bugs.openjdk.java.net/browse/JDK-7092821 , but it only 
helped the calls for new encryption algorithms. Xin had helped me to upload the 
CR: https://cr.openjdk.java.net/~xliu/8229958/webrev/ .

I had run tier-1 tests, and also a JMH test to confirm its performance 
improvement. We have also had it running in a load test environment of an 
application for a couple of days, and the improvement was confirmed as well.

Best regards,
Letu

Re: RFR 8207814: (proxy) upgrade the proxy class generator

[Mandy Chung]

Hi Roger,

The new test case looks good.  Thanks for adding that.

I like the renamed ProxyGenerator_v49 better.  ProxyPerf.java needs to 
be updated to call the renamed class.


Other than that, the patch looks good.  I'll rely on the test runs to 
verify this patch.  No need for a new webrev.


I filed JDK-8229959 on Remi's idea on using constant dynamic.

thanks
Mandy

On 8/20/19 2:08 PM, Roger Riggs wrote:

Hi,

Updated the webrev to rename the old ProxyGenerator to reflect the 
bytecode version (v49)
and added a combo test for throwing exceptions in the handler 
(expected and unexpected).


http://cr.openjdk.java.net/~rriggs/webrev-upgrade-proxy-gen-8207814/

Thanks, Roger


On 8/19/19 6:13 PM, Mandy Chung wrote:
This is a good idea to explore.  We should keep this patch to focus 
on switching ASM.I will file a JBS issue for this suggestion.


Mandy

On 8/19/19 2:19 PM, Remi Forax wrote:
A follow up should to use constant dynamic (introduce in Java 11) to 
get the j.l.r.Method object instead of pre-calculating all of them 
in the static init block.
The idea is that a ldc on a constant dynamic with bootstrap method 
that takes a MethodHandle as parameter (as boostrap argument) can 
return the corresponding Method by using Lookup.revealDirect + 
reflectAs.

The bootstrap method can be added to j.l.i.ConstantBootstraps

This should consume less bytecode (the MethodHandle is directly 
encodable as a constant pool constant) and only creates the 
j.l.r.Method if the interface method is actually called.


Rémi

- Mail original -

De: "Roger Riggs" 
À: "core-libs-dev" 
Envoyé: Vendredi 16 Août 2019 21:15:30
Objet: RFR 8207814: (proxy) upgrade the proxy class generator
Please review an enhancement to replace the java.lang.reflect.Proxy
class file generation.
The new generator uses ASM and generates stackmaps. The implementation
follows
the same structure as before but has many differences as it leverages
ASM for generating the bytecode.
A Combo test is included and two JMH based benchmarks.

The ancient ProxyGenerator_15 implementation is temporarily retained
to allow comparisons of generated class files and performance.

Issue:
https://bugs.openjdk.java.net/browse/JDK-8207814

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-upgrade-proxy-gen-8207814/

(Upgrading bytecode generation is necessary for Valhalla but makes 
sense

for the main line.)

Thanks, Roger

Re: 8229845: Decrease memory consumption of BigInteger.toString()

[Brian Burkhalter]

Hi Ivan,

> On Aug 20, 2019, at 2:45 PM, Ivan Gerasimov  wrote:
> 
> Would it make sense to add an argument `digits` to smallToString (like the 
> same named argument of toString, the minimum number of digits to pad to), so 
> it could zero-pad the result itself?
> 
> This way we could avoid inserting zeros into the middle of a StringBuilder 
> after a call to smallToString.

Indeed I do not like the zero insertion either. I’ll investigate your idea.

Thanks,

Brian

> On 8/20/19 11:51 AM, Brian Burkhalter wrote:
>> Delta [1] and revised [2] patches:
>> 
>> [1] http://cr.openjdk.java.net/~bpb/8229845/webrev.00-01/ 
>> 
>> [2] http://cr.openjdk.java.net/~bpb/8229845/webrev.01/ 
>> 

Re: 8229845: Decrease memory consumption of BigInteger.toString()

[Brian Burkhalter]

> On Aug 20, 2019, at 2:58 PM, Claes Redestad  wrote:
> 
> On 2019-08-20 23:56, Claes Redestad wrote:
>> On 2019-08-20 10:05, Aleksey Shipilev wrote:
>>> 
>>>   *) This might be "static final", while we are at it:
>>> 
>>> 4109 private static String[] zeros = new String[64];
>> Addressed by http://cr.openjdk.java.net/~redestad/8228581/open.00/ 
>> 
> 
> Meant to link the review thread, not the webrev:
> 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-July/061520.html 
> 
Looks like we have a small collision. ;-)

Thanks,

Brian

Re: RFR: JDK-8139820: URLClassPath.FileLoader constructor redundantly checks protocol

[Claes Redestad]

On 2019-08-20 02:52, Evgeny Mandrikov wrote:


Thank you for review and sponsoring!
Updated patch - http://cr.openjdk.java.net/~godin/8139820/webrev.01/


Ran it through the first couple of test tiers and pushed.

/Claes

Re: 8229845: Decrease memory consumption of BigInteger.toString()

[Claes Redestad]

On 2019-08-20 23:56, Claes Redestad wrote:

On 2019-08-20 10:05, Aleksey Shipilev wrote:


  *) This might be "static final", while we are at it:

4109 private static String[] zeros = new String[64];


Addressed by http://cr.openjdk.java.net/~redestad/8228581/open.00/


Meant to link the review thread, not the webrev:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-July/061520.html

/Claes

Re: 8229845: Decrease memory consumption of BigInteger.toString()

[Claes Redestad]

On 2019-08-20 10:05, Aleksey Shipilev wrote:


  *) This might be "static final", while we are at it:

4109 private static String[] zeros = new String[64];


Addressed by http://cr.openjdk.java.net/~redestad/8228581/open.00/

/Claes

Re: 8229845: Decrease memory consumption of BigInteger.toString()

[Ivan Gerasimov]

Hi Brian!

Would it make sense to add an argument `digits` to smallToString (like 
the same named argument of toString, the minimum number of digits to pad 
to), so it could zero-pad the result itself?


This way we could avoid inserting zeros into the middle of a 
StringBuilder after a call to smallToString.


With kind regards,
Ivan

On 8/20/19 11:51 AM, Brian Burkhalter wrote:

Delta [1] and revised [2] patches:

[1] http://cr.openjdk.java.net/~bpb/8229845/webrev.00-01/
[2] http://cr.openjdk.java.net/~bpb/8229845/webrev.01/

On Aug 20, 2019, at 1:05 AM, Aleksey Shipilev > wrote:


On 8/19/19 10:08 PM, Brian Burkhalter wrote:

[2]http://cr.openjdk.java.net/~bpb/8229845/webrev.00/


Two drive-by comments:

*) It seems the "signum == 0" case in smallToString is regressing? 
Before, it just returned the

constant-pooled "0", now it goes via StringBuilder;


Yes, and I don’t see any way not to have it do so. Note that due to 
lines 3933-3936 in [2] this case will never be hit for values which 
have no more than 20 ints in their magnitude, i.e., those which do not 
recurse in toString(BigInteger,StringBuilder,int,int).



*) This might be "static final", while we are at it:

4109 private static String[] zeros = new String[64];


Above made final at L4111 in [2].

On Aug 20, 2019, at 10:28 AM, Ivan Gerasimov 
mailto:ivan.gerasi...@oracle.com>> wrote:


While we're here.

On 8/20/19 1:05 AM, Aleksey Shipilev wrote:

 *) This might be "static final", while we are at it:

4109 private static String[] zeros = new String[64];


It may make sense to create the only string as

private static final String zeros = "0".repeat(63);


Changed at L4111 in [2].


and then replace all

sb.append(zeros[x]) with sb.append(zeros, 0, x) and


L4006 in [2].


sb.insert(pos, zeros[x]) with sb.insert(pos, zeros, 0, x).


L4049 and L4054 in [2].

The difference would be in one extra bounds check, and as both 
append() and insert() are relatively expensive, this check would 
hardly be noticeable.


Thanks,

Brian


--
With kind regards,
Ivan Gerasimov

RE: RFR: 8224974: Implement JEP 352

[Viswanathan, Sandhya]

Hi Andrew/Alan,

It is wonderful to see this feature integrated. Thanks a lot for all your hard 
work.

Best Regards,
Sandhya

-Original Message-
From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf 
Of Andrew Dinn
Sent: Tuesday, August 20, 2019 2:36 AM
To: Alan Bateman 
Cc: Jonathan Halliday ; nio-...@openjdk.java.net; 
core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8224974: Implement JEP 352

On 19/08/2019 15:36, Alan Bateman wrote:
> I think webrev.12 looks good; I don't have any other comments.
Thanks, Alan. I just pushed the patch for the JEP implementation.
(Hallelujah!). Thanks for all your help.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR 8207814: (proxy) upgrade the proxy class generator

[Roger Riggs]

Hi,

Updated the webrev to rename the old ProxyGenerator to reflect the 
bytecode version (v49)
and added a combo test for throwing exceptions in the handler (expected 
and unexpected).


http://cr.openjdk.java.net/~rriggs/webrev-upgrade-proxy-gen-8207814/

Thanks, Roger


On 8/19/19 6:13 PM, Mandy Chung wrote:
This is a good idea to explore.  We should keep this patch to focus on 
switching ASM.I will file a JBS issue for this suggestion.


Mandy

On 8/19/19 2:19 PM, Remi Forax wrote:
A follow up should to use constant dynamic (introduce in Java 11) to 
get the j.l.r.Method object instead of pre-calculating all of them in 
the static init block.
The idea is that a ldc on a constant dynamic with bootstrap method 
that takes a MethodHandle as parameter (as boostrap argument) can 
return the corresponding Method by using Lookup.revealDirect + 
reflectAs.

The bootstrap method can be added to j.l.i.ConstantBootstraps

This should consume less bytecode (the MethodHandle is directly 
encodable as a constant pool constant) and only creates the 
j.l.r.Method if the interface method is actually called.


Rémi

- Mail original -

De: "Roger Riggs" 
À: "core-libs-dev" 
Envoyé: Vendredi 16 Août 2019 21:15:30
Objet: RFR 8207814: (proxy) upgrade the proxy class generator
Please review an enhancement to replace the java.lang.reflect.Proxy
class file generation.
The new generator uses ASM and generates stackmaps. The implementation
follows
the same structure as before but has many differences as it leverages
ASM for generating the bytecode.
A Combo test is included and two JMH based benchmarks.

The ancient ProxyGenerator_15 implementation is temporarily retained
to allow comparisons of generated class files and performance.

Issue:
   https://bugs.openjdk.java.net/browse/JDK-8207814

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-upgrade-proxy-gen-8207814/

(Upgrading bytecode generation is necessary for Valhalla but makes 
sense

for the main line.)

Thanks, Roger

Re: 8229845: Decrease memory consumption of BigInteger.toString()

[Brian Burkhalter]

Delta [1] and revised [2] patches:

[1] http://cr.openjdk.java.net/~bpb/8229845/webrev.00-01/ 

[2] http://cr.openjdk.java.net/~bpb/8229845/webrev.01/

> On Aug 20, 2019, at 1:05 AM, Aleksey Shipilev  wrote:
> 
> On 8/19/19 10:08 PM, Brian Burkhalter wrote:
>> [2] http://cr.openjdk.java.net/~bpb/8229845/webrev.00/ 
>> 
> 
> Two drive-by comments:
> 
> *) It seems the "signum == 0" case in smallToString is regressing? Before, it 
> just returned the
> constant-pooled "0", now it goes via StringBuilder;

Yes, and I don’t see any way not to have it do so. Note that due to lines 
3933-3936 in [2] this case will never be hit for values which have no more than 
20 ints in their magnitude, i.e., those which do not recurse in 
toString(BigInteger,StringBuilder,int,int).

> *) This might be "static final", while we are at it:
> 
> 4109 private static String[] zeros = new String[64];

Above made final at L4111 in [2].

> On Aug 20, 2019, at 10:28 AM, Ivan Gerasimov  
> wrote:
> 
> While we're here.
> 
> On 8/20/19 1:05 AM, Aleksey Shipilev wrote:
>>  *) This might be "static final", while we are at it:
>> 
>> 4109 private static String[] zeros = new String[64];
>> 
> It may make sense to create the only string as
> 
> private static final String zeros = "0".repeat(63);

Changed at L4111 in [2].

> and then replace all
> 
> sb.append(zeros[x]) with sb.append(zeros, 0, x) and

L4006 in [2].

> sb.insert(pos, zeros[x]) with sb.insert(pos, zeros, 0, x).

L4049 and L4054 in [2].

> The difference would be in one extra bounds check, and as both append() and 
> insert() are relatively expensive, this check would hardly be noticeable.


Thanks,

Brian

Re: RFR 8211360 : Change #if DEF to #if defined(DEF)

[Brian Burkhalter]

Hello Ivan,

Looks fine.

Brian

> On Aug 20, 2019, at 10:38 AM, Ivan Gerasimov  
> wrote:
> 
> It's a followup for JDK-8211146.
> 
> With that fix several C-preprocessor statements of form #elif __linux__ were 
> changed to more accurate #elif defined(__linux__).
> 
> grep found a few more occurrences of the same pattern, which also would 
> better be cleaned up.
> 
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8211360 
> 
> WEBREV: http://cr.openjdk.java.net/~igerasim/8211360/00/webrev/ 
> 
> 
> Mach5 control build/testing went fine on all platforms.
> Would you please help review?

RFR 8211360 : Change #if DEF to #if defined(DEF)

[Ivan Gerasimov]

Hello!

It's a followup for JDK-8211146.

With that fix several C-preprocessor statements of form #elif __linux__ 
were changed to more accurate #elif defined(__linux__).


grep found a few more occurrences of the same pattern, which also would 
better be cleaned up.


BUGURL: https://bugs.openjdk.java.net/browse/JDK-8211360
WEBREV: http://cr.openjdk.java.net/~igerasim/8211360/00/webrev/

Mach5 control build/testing went fine on all platforms.
Would you please help review?

--
With kind regards,
Ivan Gerasimov

Re: 8229845: Decrease memory consumption of BigInteger.toString()

[Ivan Gerasimov]

While we're here.

On 8/20/19 1:05 AM, Aleksey Shipilev wrote:

  *) This might be "static final", while we are at it:

4109 private static String[] zeros = new String[64];


It may make sense to create the only string as

private static final String zeros = "0".repeat(63);

and then replace all

sb.append(zeros[x]) with sb.append(zeros, 0, x) and

sb.insert(pos, zeros[x]) with sb.insert(pos, zeros, 0, x).

The difference would be in one extra bounds check, and as both append() 
and insert() are relatively expensive, this check would hardly be 
noticeable.


--

With kind regards,
Ivan Gerasimov

Re: RFR: JDK-8229871: Imporve performance of Method.copy() and leafCopy()

[Mandy Chung]

Hi Ogata,

The patch looks okay.  Nit: please add a space between if and (.

About volatile methodAccessor field, I checked the history.  Both 
fieldAccessor and methodAccessor were started as volatile and the 
fieldAccessor declaration was updated due to JDK-5044412.   As you 
observe, I think the methodAccessor field could be made non-volatile.  
OTOH that might impact when it's inflated to spin bytecode for this 
method invocation.  I don't know how importance to keep its volatile vs 
non-volatile in practice without doing benchmarking/real application 
testing.


Mandy

On 8/19/19 2:51 AM, Kazunori Ogata wrote:

Hi,

May I have review for "JDK-8229871: Imporve performance of Method.copy()
and leafCopy()"?

Method.copy() and leafCopy() creates a copy of a Method object with
sharing MethodAccessor object. Since the methodAccessor field is a
volatile variable, copying this field needs memory fence to ensure the
field is visible to all threads on the weak memory platforms such as POWER
and ARM.

When the methodAccessor of the root object is null (i.e., not initialized
yet), we do not need to copy the null value because this field of the
copied object has been initialized to null in the constructor. We can
reduce overhead of the memory fence only when the root's methodAccessor is
non-null. This change improved performance by 5.8% using a micro benchmark
that repeatedly invokes Class.getMethods().

Bug: https://bugs.openjdk.java.net/browse/JDK-8229871

Webrev: http://cr.openjdk.java.net/~ogatak/8229871/webrev.00/


By the way, why Method.methodAccessor is volatile, while
Field.fieldAccessor and Field.overrideFieldAccessor are not volatile?  I
know the use of volatile reduces probability of creating duplicated method
accessor, but the chance still exists.  I couldn't find the difference
between Method and Field classes to make Method.methodAccessor volatile.
If we can make it non-volatile, it is more preferable than a quick hack
above.


Regards,
Ogata

Re: flatMap still prevents short circuiting when using .iterator()

[Stephen Buergler]

Oops thanks.
So I think the issue is that StreamSpliterators.java has this line
bufferSink = ph.wrapSink(b::accept);
that creates a Sink that never returns true in cancellationRequested()
which is needed to get flatMap() to stop looping.

On Thu, Aug 15, 2019 at 7:43 PM David Holmes  wrote:
>
> Re-directing to core-libs-dev
>
> David
>
> On 15/08/2019 7:48 pm, Stephen Buergler wrote:
> > Not really sure where to send this.
> > flatMap was fixed so that it doesn't prevent short circuiting.
> > https://bugs.openjdk.java.net/browse/JDK-8075939
> > If you replace the test cases in the ticket with versions that use
> > .iterator().next() instead of .findFirst().get() then the problem
> > still happens.
> > I just tested this with openjdk:14 on docker hub.
> >

Re: RFR: 8229899: java.io.File.isInvalid() is racy

[Arthur Eubanks]

Thanks for the reviews!

On Tue, Aug 20, 2019 at 8:21 AM Martin Buchholz  wrote:

> Looks good to me.
>
> On Mon, Aug 19, 2019 at 4:15 PM Arthur Eubanks 
> wrote:
>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8229899
>> Webrev: http://cr.openjdk.java.net/~aeubanks/8229899/webrev.00/
>>
>> final boolean isInvalid() {
>> if (status == null) {
>> status = (this.path.indexOf('\u') < 0) ?
>> PathStatus.CHECKED
>>:
>> PathStatus.INVALID;
>> }
>> return status == PathStatus.INVALID;
>> }
>> If the reads to "status" are reordered and another thread writes to
>> "status", the return value can be wrong.
>> Reading "status" to a local variable fixes the issue.
>>
>

Re: RFR: 8229899: java.io.File.isInvalid() is racy

[Martin Buchholz]

Looks good to me.

On Mon, Aug 19, 2019 at 4:15 PM Arthur Eubanks  wrote:

> Bug: https://bugs.openjdk.java.net/browse/JDK-8229899
> Webrev: http://cr.openjdk.java.net/~aeubanks/8229899/webrev.00/
>
> final boolean isInvalid() {
> if (status == null) {
> status = (this.path.indexOf('\u') < 0) ? PathStatus.CHECKED
>:
> PathStatus.INVALID;
> }
> return status == PathStatus.INVALID;
> }
> If the reads to "status" are reordered and another thread writes to
> "status", the return value can be wrong.
> Reading "status" to a local variable fixes the issue.
>

Re: RFR: 8224974: Implement JEP 352

[Dmitry Chuyko]

On 8/20/19 11:02 AM, Andrew Dinn wrote:

On 19/08/2019 19:38, Dmitry Chuyko wrote:

Just a minor style thing in MapSyncFail test: can "true" and "false"
(the mode) be "READ_WRITE_SYNC" and "READ_ONLY_SYNC" instead?

Are you referring to the arguments passed on the command line? If so
then the answer is clearly yes.


Yes, exactly that. Trivial change like a string switch wouldn't bloat 
the code much.


Anyway, probably it was already a time to push changes :-)

Latest version of MapSyncFail covers exception path then writeback is 
not enabled so I think we don't need additional work here either.


Thanks for a good job!

-Dmitry



..

regards,


Andrew Dinn
---

Re: RFR: 8229899: java.io.File.isInvalid() is racy

[Alan Bateman]

On 20/08/2019 00:14, Arthur Eubanks wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8229899
Webrev: http://cr.openjdk.java.net/~aeubanks/8229899/webrev.00/

 final boolean isInvalid() {
 if (status == null) {
 status = (this.path.indexOf('\u') < 0) ? PathStatus.CHECKED
:
PathStatus.INVALID;
 }
 return status == PathStatus.INVALID;
 }
If the reads to "status" are reordered and another thread writes to
"status", the return value can be wrong.
Reading "status" to a local variable fixes the issue.
An oversight in JDK-8003992 that we missed during the review at the 
time. The change looks good.


-Alan.

Re: RFR: 8224974: Implement JEP 352

[Andrew Dinn]

On 19/08/2019 15:36, Alan Bateman wrote:
> I think webrev.12 looks good; I don't have any other comments.
Thanks, Alan. I just pushed the patch for the JEP implementation.
(Hallelujah!). Thanks for all your help.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: 8229845: Decrease memory consumption of BigInteger.toString()

[Aleksey Shipilev]

On 8/19/19 10:08 PM, Brian Burkhalter wrote:
> [2] http://cr.openjdk.java.net/~bpb/8229845/webrev.00/

Two drive-by comments:

 *) It seems the "signum == 0" case in smallToString is regressing? Before, it 
just returned the
constant-pooled "0", now it goes via StringBuilder;

 *) This might be "static final", while we are at it:

4109 private static String[] zeros = new String[64];


-- 
Thanks,
-Aleksey

Re: RFR: 8224974: Implement JEP 352

[Andrew Dinn]

On 19/08/2019 19:38, Dmitry Chuyko wrote:
> Just a minor style thing in MapSyncFail test: can "true" and "false"
> (the mode) be "READ_WRITE_SYNC" and "READ_ONLY_SYNC" instead?
Are you referring to the arguments passed on the command line? If so
then the answer is clearly yes.

However, I'm not sure why you consider this change important and/or an
improvement? Do you feel that the code which processes the argument to
compute the Mode setting is obscure?

Or perhaps you are referring to something other than the command line
arguments?

I /do/ need to fix the typo in the test where is says

  "expected true of false as an argument"

Obviously that should say "true *or* false".

regards,


Andrew Dinn
---

Re: RFR: 8229899: java.io.File.isInvalid() is racy

[Aleksey Shipilev]

On 8/20/19 1:14 AM, Arthur Eubanks wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8229899
> Webrev: http://cr.openjdk.java.net/~aeubanks/8229899/webrev.00/

Right, the new code turns data race benign. In older code, the first read of 
"status" could have
returned "!null", while the second read could have returned "null", breaking 
the check. Dusty little
corner in Java memory model, that.

Looks good.

-- 
Thanks,
-Aleksey