RFR: jsr166 jdk9 integration wave 6
Easy changes to review, up to April Fools day, in part to make room for later unfinished more exciting changes. http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
Hi Martin, for http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/... aka introducing a new constructor seems to be a regression to me, the less overloads we have the better i understand the code. Rémi ----- Mail original -----
De: "Martin Buchholz" <martinrb@google.com> À: "core-libs-dev" <core-libs-dev@openjdk.java.net>, "Doug Lea" <dl@cs.oswego.edu>, "Paul Sandoz" <paul.sandoz@oracle.com>, "Chris Hegarty" <chris.hegarty@oracle.com>, "David Holmes" <David.Holmes@oracle.com> Envoyé: Dimanche 3 Avril 2016 20:29:57 Objet: RFR: jsr166 jdk9 integration wave 6
Easy changes to review, up to April Fools day, in part to make room for later unfinished more exciting changes.
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
Thanks for the review! On Sun, Apr 3, 2016 at 12:34 PM, Remi Forax <forax@univ-mlv.fr> wrote:
Hi Martin, for http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/...
aka introducing a new constructor seems to be a regression to me, the less overloads we have the better i understand the code.
For "telescoping constructors" and a parameter that's almost always null, I disagree. There's also the fear that the VM won't optimize away useless volatile write to next.
ParkLoops test is fine too, i suppose it's related to the recent change in the code of TimeUnit to test with different units.
No, it fixes a mistake made when introducing timeout factor scaling. The webrev has a link to the bug report https://bugs.openjdk.java.net/browse/JDK-8151501
----- Mail original -----
De: "Martin Buchholz" <martinrb@google.com> À: "Remi Forax" <forax@univ-mlv.fr> Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net>, "Doug Lea" <dl@cs.oswego.edu>, "Paul Sandoz" <paul.sandoz@oracle.com>, "Chris Hegarty" <chris.hegarty@oracle.com>, "David Holmes" <David.Holmes@oracle.com> Envoyé: Dimanche 3 Avril 2016 23:08:50 Objet: Re: RFR: jsr166 jdk9 integration wave 6
Thanks for the review!
On Sun, Apr 3, 2016 at 12:34 PM, Remi Forax <forax@univ-mlv.fr> wrote:
Hi Martin, for http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/...
aka introducing a new constructor seems to be a regression to me, the less overloads we have the better i understand the code.
For "telescoping constructors" and a parameter that's almost always null, I disagree.
if the parameter is often null, maybe the constructor with 4 parameters is useless, and next should be set explicitly in the few cases it's needed.
There's also the fear that the VM won't optimize away useless volatile write to next.
I've seen the bug [1] reported by Aleksey about Hotspot not being able to remove volatile writes with null in constructor. Here the code is different because val is also a volatile field, and written just before next, so the VM should be able to coalesce two volatile writes, if the VM is not able to do that, it's another bug. Nevertheless, we should only optimize to for the usual cases, so we should only have one constructor that doesn't set next. [...] [1] https://bugs.openjdk.java.net/browse/JDK-8145948 Rémi
On 04/03/2016 06:17 PM, forax@univ-mlv.fr wrote: ----
De: "Martin Buchholz" <martinrb@google.com>
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/...
aka introducing a new constructor seems to be a regression to me, the less overloads we have the better i understand the code.
For "telescoping constructors" and a parameter that's almost always null, I disagree.
if the parameter is often null, maybe the constructor with 4 parameters is useless, and next should be set explicitly in the few cases it's needed.
There's also the fear that the VM won't optimize away useless volatile write to next.
Right. The issue is whether to be explicit about lack of need of a fence, or to hope that the compiler figures it out. Especially in a component as commonly used as CHM, being explicit seems like the right choice. -Doug
Hi Martin, Dequeue doc change is fine, ParkLoops test is fine too, i suppose it's related to the recent change in the code of TimeUnit to test with different units. For the CompletableFuture change, i don't understand the consequence of the patch so i declare myself incompetent on that subject :) regards, Rémi ----- Mail original -----
De: "Martin Buchholz" <martinrb@google.com> À: "core-libs-dev" <core-libs-dev@openjdk.java.net>, "Doug Lea" <dl@cs.oswego.edu>, "Paul Sandoz" <paul.sandoz@oracle.com>, "Chris Hegarty" <chris.hegarty@oracle.com>, "David Holmes" <David.Holmes@oracle.com> Envoyé: Dimanche 3 Avril 2016 20:29:57 Objet: RFR: jsr166 jdk9 integration wave 6
Easy changes to review, up to April Fools day, in part to make room for later unfinished more exciting changes.
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
Hi Martin, miscellaneous — You reverted Aleksey’s change s/putOrderedObject/putObjectRelease. The *Ordered* methods are now removed from the “real” jdk.internal.misc.Unsafe. Paul.
On 3 Apr 2016, at 20:29, Martin Buchholz <martinrb@google.com> wrote:
Easy changes to review, up to April Fools day, in part to make room for later unfinished more exciting changes.
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
We missed Aleksey's changes, which broke our integration. Historically, hotspot was more independent from jdk, and there were no "flag days" that required both to be modified together. In jsr166 CVS we still consistently use sun.misc.Unsafe, because it's ... ummm ... more portable. Our openjdk integration script changes that to jdk.internal.misc, but needs modification. Most of these will sadly soon change again, due to varhandlification. Everything regenerated, now with: # Replaces sun.misc.Unsafe with jdk9's preferred jdk.internal.misc find src/main -name '*.java' \ | xargs perl -pi -0777 \ -e 's~\bsun\.misc\.Unsafe\b~jdk.internal.misc.Unsafe~g; s~\bputOrdered([A-Za-z]+)\b~put${1}Release~g' On Wed, Apr 6, 2016 at 5:54 AM, Paul Sandoz <paul.sandoz@oracle.com> wrote:
Hi Martin,
miscellaneous —
You reverted Aleksey’s change s/putOrderedObject/putObjectRelease.
The *Ordered* methods are now removed from the “real” jdk.internal.misc.Unsafe.
Paul.
On 3 Apr 2016, at 20:29, Martin Buchholz <martinrb@google.com> wrote:
Easy changes to review, up to April Fools day, in part to make room for later unfinished more exciting changes.
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
On 6 Apr 2016, at 22:17, Martin Buchholz <martinrb@google.com> wrote:
We missed Aleksey's changes, which broke our integration.
Looks good. Regarding CompletableFutureTest.testManyDependents i presume that kind of test was not failing before the modifications to CompletableFuture? I am struggling to square the CF updates to the test. AFAICT the cleaning of a CF stack is now less aggressive. A dependent’s stack stack will now only be cleared if it has not completed (rather than if also the computation is nested). Thus in theory that test should run more efficiently? Paul.
Historically, hotspot was more independent from jdk, and there were no "flag days" that required both to be modified together.
In jsr166 CVS we still consistently use sun.misc.Unsafe, because it's ... ummm ... more portable. Our openjdk integration script changes that to jdk.internal.misc, but needs modification. Most of these will sadly soon change again, due to varhandlification.
Everything regenerated, now with:
# Replaces sun.misc.Unsafe with jdk9's preferred jdk.internal.misc find src/main -name '*.java' \ | xargs perl -pi -0777 \ -e 's~\bsun\.misc\.Unsafe\b~jdk.internal.misc.Unsafe~g; s~\bputOrdered([A-Za-z]+)\b~put${1}Release~g'
On Wed, Apr 6, 2016 at 5:54 AM, Paul Sandoz <paul.sandoz@oracle.com> wrote:
Hi Martin,
miscellaneous —
You reverted Aleksey’s change s/putOrderedObject/putObjectRelease.
The *Ordered* methods are now removed from the “real” jdk.internal.misc.Unsafe.
Paul.
On 3 Apr 2016, at 20:29, Martin Buchholz <martinrb@google.com> wrote:
Easy changes to review, up to April Fools day, in part to make room for later unfinished more exciting changes.
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
On Thu, Apr 7, 2016 at 6:17 AM, Paul Sandoz <paul.sandoz@oracle.com> wrote:
On 6 Apr 2016, at 22:17, Martin Buchholz <martinrb@google.com> wrote:
Looks good.
Regarding CompletableFutureTest.testManyDependents i presume that kind of test was not failing before the modifications to CompletableFuture?
Right - technically it was not failing before, but the O(N^2) performance in case of regression would be noticed quickly. And it seems like a generally useful test.
I am struggling to square the CF updates to the test. AFAICT the cleaning of a CF stack is now less aggressive. A dependent’s stack stack will now only be cleared if it has not completed (rather than if also the computation is nested). Thus in theory that test should run more efficiently?
It's not useful to clean the stack of a future that has completed because all of its dependents will be triggered anyways, so there's no risk of garbage accumulation. But jsr166 CVS already has followon changes in this area that are not part of this integration.
On 7 Apr 2016, at 16:55, Martin Buchholz <martinrb@google.com> wrote:
On Thu, Apr 7, 2016 at 6:17 AM, Paul Sandoz <paul.sandoz@oracle.com> wrote:
On 6 Apr 2016, at 22:17, Martin Buchholz <martinrb@google.com> wrote:
Looks good.
Regarding CompletableFutureTest.testManyDependents i presume that kind of test was not failing before the modifications to CompletableFuture?
Right - technically it was not failing before, but the O(N^2) performance in case of regression would be noticed quickly. And it seems like a generally useful test.
Yes, no disagreement.
I am struggling to square the CF updates to the test. AFAICT the cleaning of a CF stack is now less aggressive. A dependent’s stack stack will now only be cleared if it has not completed (rather than if also the computation is nested). Thus in theory that test should run more efficiently?
It's not useful to clean the stack of a future that has completed because all of its dependents will be triggered anyways, so there's no risk of garbage accumulation. But jsr166 CVS already has followon changes in this area that are not part of this integration.
I was looking at those trying to understand where things are going. Current webrev is fine. Thanks, Paul.
On 04/07/2016 11:29 AM, Paul Sandoz wrote:
I am struggling to square the CF updates to the test. AFAICT the cleaning of a CF stack is now less aggressive. A dependent’s stack stack will now only be cleared if it has not completed (rather than if also the computation is nested). Thus in theory that test should run more efficiently?
It's not useful to clean the stack of a future that has completed because all of its dependents will be triggered anyways, so there's no risk of garbage accumulation. But jsr166 CVS already has followon changes in this area that are not part of this integration.
I was looking at those trying to understand where things are going.
Current webrev is fine.
Thanks for looking at these! We've been diagnosing and addressing various (potential) CompletableFuture performance issues, as they are used increasingly often. Some of the changes in jsr166 CVS aren't yet evaluated enough to commit to though. -Doug
participants (5)
-
Doug Lea
-
forax＠univ-mlv.fr
-
Martin Buchholz
-
Paul Sandoz
-
Remi Forax