Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Mandy Chung]

> On Jun 29, 2016, at 6:42 PM, David Holmes  wrote:
> 
> The earlier you initialize Throwable the earlier you can try to create an 
> exception that has a backtrace. But there will always be a region of code 
> where we can't throw an OOME with a backtrace because of the missing 
> initialization of Throwable. So we can narrow that window by moving the 
> initialization of Throwable (which in turn requires a whole bunch of 
> collection classes - so the window has a fixed minimum size [ unless we do 
> some creative restructuring of Throwable's static initialization]). My 
> argument, which I think I've now convinced Kim of, is that we shouldn't be 
> trying to throw an OOME with a stacktrace if Throwable has not been 
> initialized - and that is where the change to gen_out_of_memory_error comes 
> in. It is actually passed the pre-allocated OOME that has no backtrace and 
> will never have a backtrace, and that is what should be thrown when Throwable 
> has not been initialized.

Thanks for the clarification.  It makes sense and the pre-allocated OOME with 
no backtrace should be used for this early OOM scenario.

Mandy

Semantics of VarHandle CAS methods

[Martin Buchholz]

VarHandle.compareAndSet is strong in two ways - non-spurious and
sequentially consistent.

VarHandle.weakCompareAndSet is weak in both these ways.
(That seems like a mistake to me.
The fact that j.u.c. Atomic classes are a precedent for this seems
unfortunate.)

http://download.java.net/java/jdk9/docs/api/java/lang/invoke/VarHandle.html#weakCompareAndSet-java.lang.Object...-

Sequential consistency should always be the default behavior of an
unqualified cas operation - users should have to go out of their way to
request relaxed reads or writes.  C++ reasonably makes you use
e.g. std::atomic_compare_exchange_weak_explicit.

I was surprised that weakCompareAndSetAcquire used relaxed writes!

I would expect that almost everyone would be happy with sequentially
consistent, and almost everyone else would be happy with acquire-release, a
variant we don't offer!  And a few others would be happy with fully relaxed
but single-variable sequentially consistent (but Java, unlike C++, doesn't
promise that!).

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Mandy Chung]

> On Jun 28, 2016, at 10:19 PM, David Holmes  wrote:
> 
> So here is what I see has happened.
> 
> Looking back at 9-b01, before we forced the initialization of 
> InterruptedException and thus Throwable we find:
> 
> 58 Initializing 'java/lang/Throwable' (0x00082990)
> 
> So Kim is right this was working by accident. It just seems that back then 
> there was less memory required by the initialization of all the collection 
> and other classes and so we didn't run into this problem.
> 
> Post the InterruptedException change the initialization order made it 
> unlikely an OOME could be encountered before Throwable was initialized (and 
> after we have reached a point where we can throw without the VM crashing or 
> instead doing a vm_exit_during_initialization).
> 
> Post modules those collection classes, and others, are now done earlier again 
> and before Throwable. And presumably their memory demands have increased.
> 

This is the new primordial class added by modules that causes additional 
classes to be loaded early before initPhase1.

  // The VM creates objects of this class.
  initialize_class(vmSymbols::java_lang_reflect_Module(), CHECK);

> Although we preallocate the OutOfMemoryError instances, and avoid executing 
> any java code to do so, we can't necessarily** "throw" them until after 
> Throwable is initialized. We now have a lot more initialization occurring 
> before we init Throwable and so OOME is more likely and so it will fail as 
> Kim observed.


Would initializing java.lang.Throwable after java.lang.reflect.Module address 
this issue?  I don’t think I fully follow the problem Kim observed and below.

> 
> ** I say necessarily because I still believe it is the fact we attempt to 
> fill in the stacktrace that leads to the dependency on Throwable being 
> initialized, and we should be able to avoid that if we check the VM 
> initialization state in gen_out_of_memory_error().

Mandy

Re: RFR: jsr166 jdk9 integration wave 7

[Doug Lea]

On 06/29/2016 01:28 PM, Peter Levart wrote:

I see it was intentional as it is described in the docs...

Is new behavior better?


In tests so far, I see smaller variances, and approximately same average,
so yes, a little better. Other usages of onSpinWait probably have more
effect than this one in StampedLock.

-Doug



Regards, Peter


On 06/29/2016 07:19 PM, Peter Levart wrote:

Hi Martin,

I'm looking at changes in StampedLock. I noticed that you have consistently
(in 4 places) replaced usages of the following idiom:


1044 if (LockSupport.nextSecondarySeed() >= 0)
1045 --spins;

with the following:

1056 --spins;
1057 Thread.onSpinWait();


Was that intentional? It looks almost like you thought that
LockSupport.nextSecondarySeed() >= 0 is always true and that it is used just
to introduce some pause into the spin loop. It is only true in roughly half of
invocations as nextSecondarySeed() has the full signed int span.

Regards, Peter


On 06/27/2016 09:38 PM, Martin Buchholz wrote:

jsr166 has been pervasively modified to use VarHandles, but there are some
other pending changes (that cannot be cleanly separated from VarHandle
conversion).  We expect this to be the last feature integration for jdk9.

http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/

We're asking Paul to do most of the review work here, as owner of
VarHandles JEP and as Oracle employee.
We need approval for API changes in

https://bugs.openjdk.java.net/browse/JDK-8157523
Various improvements to ForkJoin/SubmissionPublisher code

https://bugs.openjdk.java.net/browse/JDK-8080603
Replace Unsafe with VarHandle in java.util.concurrent classes

There is currently a VarHandle bootstrap problem with
ThreadLocal/AtomicInteger that causes
java/util/Locale/Bug4152725.java
to fail.  Again I'm hoping that Paul will figure out what to do.  In the
past rearranging the order of operations in  has worked for similar
problems.  It's not surprising we have problems, since j.u.c. needs
VarHandles initialized to do work, and j.l.invoke needs j.u.c. (notably
AtomicInteger and ConcurrentHashMap) to do work.  Maybe we need some very
low-level concurrency infrastructure that does not use VarHandles, only for
bootstrap?

Re: RFR: jsr166 jdk9 integration wave 7

[Doug Lea]

Responding to ForkJoinPool reviews (thanks for these!)...

On 06/29/2016 06:41 PM, Martin Buchholz wrote:


It may be better if the common pool and other pools are configured the same
way, by having a maxSpares parameter instead of a maximumPoolSize parameter.


The choice was between being consistent with the common pool
property vs the ThreadPoolExecutor constructor. The latter
should be easier for users to share configuration across pool
classes, so I used it at the expense of harder-to-decode
parameter specs, but now made a little cleaeer:



On Wed, Jun 29, 2016 at 8:38 AM, Daniel Fuchs > wrote:



I mean something like: "The maximum number of spare threads used by the
common pool is 256: to arrange the same value as is used by default for the
common pool, use {@code 256} plus the parallelism level for {@code
maximumPoolSize}."



Thanks! I added a variant of this sentence.

On 06/29/2016 02:13 PM, Roger Riggs wrote:

Hi,

Several constructors of ForkJoinPool are modified with comments like " using
defaults for all other parameters". However, there there are no other
parameters in each constructor in question.

It seems to imply a reference to the new full function constructor, that
reference should be explicit complete with a {@link...}



Thanks!. Done. At least I hope so.
Because of https://bugs.openjdk.java.net/browse/JDK-8136503
we can't run javadoc on pre-integrated sources to check how this came out.


On 06/28/2016 08:55 AM, Paul Sandoz wrote:


577  * ... So these explicit checks would exist in some form anyway.
...



The paragraph is referring to explicit checks (null and bounds checks) that
were removed when the reference to Unsafe was removed.


Thanks. I killed the explicit checks sentence.

-Doug

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

[Mandy Chung]

> On Jun 29, 2016, at 12:36 PM, Brent Christian  
> wrote:
> 
> Hi,
> 
> Please review the following change for JDK 9:
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8160370
> Webrev:
> http://cr.openjdk.java.net/~bchristi/8160370/webrev.00/
> 
> The fix for 7131356 fills in the "os.version" system property on Mac using 
> [NSProcessInfo operatingSystemVersion], available starting in Mac OS 10.9.
> 
> While JDK 9 will not support versions of Mac OS prior to 10.9 [1], not all 
> testing infrastructure has been updated, and we've seen failures of
> 
> java/lang/System/OsVersionTest.java
> 
> The code to restore behavior on older Mac systems is only a few lines, so 
> that seems like a good way to get testing going again.

I think we should move away from testing on unsupported platforms.  Is this 
just temporary?  when will this be removed?

Mandy

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

[Brian Burkhalter]

Approved.

Brian

On Jun 29, 2016, at 4:33 PM, Brent Christian  wrote:

> Thank you, Dave and Gerard.
> I believe I still need to hear from a JDK9 Reviewer.

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

[Brent Christian]

Thank you, Dave and Gerard.
I believe I still need to hear from a JDK9 Reviewer.

Thanks,
-Brent
On 06/29/2016 01:08 PM, David DeHaven wrote:


Fix looks good to me too.

-DrD-


On Jun 29, 2016, at 12:54 PM, Gerard Ziemski  wrote:

hi Brent,

Thank you for fixing the original issue and for putting in this follow-up fix!

Looks good! (for full disclosure I proposed the workaround)


cheers


On Jun 29, 2016, at 2:36 PM, Brent Christian  wrote:

Hi,

Please review the following change for JDK 9:

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

The fix for 7131356 fills in the "os.version" system property on Mac using 
[NSProcessInfo operatingSystemVersion], available starting in Mac OS 10.9.

While JDK 9 will not support versions of Mac OS prior to 10.9 [1], not all 
testing infrastructure has been updated, and we've seen failures of

java/lang/System/OsVersionTest.java

The code to restore behavior on older Mac systems is only a few lines, so that 
seems like a good way to get testing going again.

Thanks,
-Brent

1. https://jdk9.java.net/jdk9_supported_platforms.html
2. https://bugs.openjdk.java.net/browse/JDK-8156132

Re: RFR: jsr166 jdk9 integration wave 7

[Martin Buchholz]

I agree the ForkJoinPool docs are difficult to understand.
It's not immediately obvious that the maximumPoolSize of a user-constructed
pool is effectively unlimited, but the maximumPoolSize of the common pool
is availableProcessors + maxSpares.
It may be better if the common pool and other pools are configured the same
way, by having a maxSpares parameter instead of a maximumPoolSize
parameter.

(Using availableProcessors to size thread pools will increasingly become a
resource usage problem)


On Wed, Jun 29, 2016 at 8:38 AM, Daniel Fuchs 
wrote:

> Hi Martin,
>
> I was looking at the new constructor's API documentation
> in ForkJoinPool - and  somehow got confused by the sentence:
>
> 2235  * @param maximumPoolSize [...
> 2241  * ...]  To
> 2240  * arrange the same value as is used by default for the common
> 2241  * pool, use {@code 256} plus the parallelism level.
>
> I mean - this looks bizarre, until you understand that the
> common pool has a maxSpares of 256 - which means that its
> actual max core pool size is 256 + parallelism level
> (am I getting that part right?).
>
> I wonder if it would be worth expanding on the rationale
> for that value?
>
> I mean something like: "The maximum number of spare threads
> used by the common pool is 256: to arrange the same value as is
> used by default for the common pool, use {@code 256} plus the
> parallelism level for {@code maximumPoolSize}."
>
> best regards,
>
> -- daniel
>
>
>
>
> On 27/06/16 20:38, Martin Buchholz wrote:
>
>> jsr166 has been pervasively modified to use VarHandles, but there are some
>> other pending changes (that cannot be cleanly separated from VarHandle
>> conversion).  We expect this to be the last feature integration for jdk9.
>>
>>
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
>>
>> We're asking Paul to do most of the review work here, as owner of
>> VarHandles JEP and as Oracle employee.
>> We need approval for API changes in
>>
>> https://bugs.openjdk.java.net/browse/JDK-8157523
>> Various improvements to ForkJoin/SubmissionPublisher code
>>
>> https://bugs.openjdk.java.net/browse/JDK-8080603
>> Replace Unsafe with VarHandle in java.util.concurrent classes
>>
>> There is currently a VarHandle bootstrap problem with
>> ThreadLocal/AtomicInteger that causes
>> java/util/Locale/Bug4152725.java
>> to fail.  Again I'm hoping that Paul will figure out what to do.  In the
>> past rearranging the order of operations in  has worked for
>> similar
>> problems.  It's not surprising we have problems, since j.u.c. needs
>> VarHandles initialized to do work, and j.l.invoke needs j.u.c. (notably
>> AtomicInteger and ConcurrentHashMap) to do work.  Maybe we need some very
>> low-level concurrency infrastructure that does not use VarHandles, only
>> for
>> bootstrap?
>>
>>
>

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[David Holmes]

On 30/06/2016 5:12 AM, Kim Barrett wrote:

On Jun 29, 2016, at 2:54 PM, Kim Barrett  wrote:

I think the problem may have existed long before modules, but simply
wasn't noticed.  That is, I suspect trying the test case mentioned
below with a pre-June-2014 fastdebug build (when Reference started
initializing InterruptedException) might trip the same assert.  I'm
presently trying to obtain a sufficiently old fastdebug build to run
the experiment.


Nope, works fine with JDK 9 b01.


See my response to Mandy above.

David
-


./jdk1.9.0/fastdebug/bin/java -XX:AutoBoxCacheMax=2147483647 -version
Error occurred during initialization of VM
java.lang.OutOfMemoryError: Requested array size exceeds VM limit
at java.lang.Integer$IntegerCache.(Integer.java:799)
at java.lang.Integer.valueOf(Integer.java:827)
at sun.misc.Signal.handle(Signal.java:169)
at java.lang.Terminator.setup(Terminator.java:60)
at java.lang.System.initializeSystemClass(System.java:1197)

Re: RFR 8158023: SocketExceptions contain too little information sometimes

[Roger Riggs]

Hi Christph,

fine, push when ready.

On 6/29/2016 5:18 PM, Langer, Christoph wrote:

Hi Roger,

thanks for reviewing.

As for:

jni_util.c: line 216:

There I don't create an extra String but the Exception Object to throw, similar 
to the old function JNU_ThrowByNameWithLastError.

I misread line 216, thinking the exception class was fixed.



jni_util.h:

line 117-119, The original comment was just as informative as the

I think you are right - I will restore the old comment.

If no objections I consider this reviewed and will push it tomorrow with the 
reverted comment lines.

Yep,

Thanks, Roger



Thanks
Christoph


-Original Message-
From: nio-dev [mailto:nio-dev-boun...@openjdk.java.net] On Behalf Of Roger
Riggs
Sent: Mittwoch, 29. Juni 2016 20:20
To: nio-...@openjdk.java.net
Subject: Re: RFR 8158023: SocketExceptions contain too little information
sometimes

Hi Christoph,

Looking good, its unfortunate that the handling of mixed platform and
utf string require jni up calls to invoke java methods.

jni_util.c: line 216:

You should not need to create an extra string; the string s is
non-null and ready to use.

jni_util.h:

line 117-119, The original comment was just as informative as the
replacement.

The rest looks fine.

Roger

On 6/28/16 4:45 PM, Langer, Christoph wrote:

Hi Paul,

Ok, you kind of got me convinced and it is also a quite simple
modification. I changed from “java.net.SocketException: ioctl
SIOCGSIZIFCONF failed: Bad file number” to “java.net.SocketException:
Bad file number (ioctl SIOCGSIZIFCONF failed)” like you suggested.

The update is in place:
http://cr.openjdk.java.net/~clanger/webrevs/8158023.3/


Now I finally need a review…

Best regards

Christoph

*From:*Paul Benedict [mailto:pbened...@apache.org]
*Sent:* Montag, 27. Juni 2016 18:15
*To:* Langer, Christoph 
*Cc:* Kenji Kazumura ; Chris Hegarty
; nio-...@openjdk.java.net;
core-libs-dev@openjdk.java.net; net-...@openjdk.java.net
*Subject:* Re: RFR 8158023: SocketExceptions contain too little
information sometimes

Christoph, I didn't understand your explanation on your message
preference. Typically root cause information is printed last, not
first. Another reason not to change the ordering of the exception
message is that applications may be looking at existing strings. For
this example, if I may presume "Bad file number" is an existing
message, I would defer to the possibility applications may be exist
that test for that message condition.


Cheers,
Paul

On Mon, Jun 27, 2016 at 2:42 AM, Langer, Christoph
> wrote:

 Hi,

 eventually here is the - hopefully final - version of this fix:
 http://cr.openjdk.java.net/~clanger/webrevs/8158023.3/
 

 Now I leave JNU_ThrowByNameWithLastError untouched and I've also
 added conversion to the new function
 JNU_ThrowByNameWithMessageAndLastError. I've replaced
 JNU_ThrowByNameWithLastError with
 JNU_ThrowByNameWithMessageAndLastError in the java/net coding
 where I think it is appropriate (mostly in occasions when a
 SocketException is thrown kind of generically). @Paul: thanks for
 your suggestion regarding the output format but I would still
 prefer an output like "java.net.SocketException: ioctl
 SIOCGSIZIFCONF failed: Bad file number" vs. "
 java.net.SocketException: Bad file number (ioctl SIOCGSIZIFCONF
 failed)" as I'm always handing down a message to the new throw
 routine.

 Hoping for a review :)

 Best regards
 Christoph

 > -Original Message-
 > From: Kenji Kazumura [mailto:k...@jp.fujitsu.com]
 > Sent: Mittwoch, 8. Juni 2016 02:51
 > To: Langer, Christoph 
 > Cc: net-...@openjdk.java.net ;
 nio-...@openjdk.java.net ; core-libs-
 > d...@openjdk.java.net 
 > Subject: Re: RFR 8158023: SocketExceptions contain too little
 information
 > sometimes
 >
 > Christoph,
 >
 > You should not remove conversion codes (platform string to Java
 String)
 > at JNU_ThrowByNameWithLastError,
 > and you have to add conversion codes at
 > JNU_ThrowByNameWithMessageAndLastError.
 >
 > It seems that you assume strerror returns only ascii characters,
 but actually
 > not.
 > It depends on the locale of your environment where java programs
 runs.
 >
 >
 > -Kenji Kazumura
 >
 >
 > In message
 >

>

 >RFR 8158023: SocketExceptions contain too little information
 

RE: RFR 8158023: SocketExceptions contain too little information sometimes

[Langer, Christoph]

Hi Roger,

thanks for reviewing.

As for:
> jni_util.c: line 216:
There I don't create an extra String but the Exception Object to throw, similar 
to the old function JNU_ThrowByNameWithLastError.

> jni_util.h:
>
>line 117-119, The original comment was just as informative as the
I think you are right - I will restore the old comment.

If no objections I consider this reviewed and will push it tomorrow with the 
reverted comment lines.

Thanks
Christoph

> -Original Message-
> From: nio-dev [mailto:nio-dev-boun...@openjdk.java.net] On Behalf Of Roger
> Riggs
> Sent: Mittwoch, 29. Juni 2016 20:20
> To: nio-...@openjdk.java.net
> Subject: Re: RFR 8158023: SocketExceptions contain too little information
> sometimes
>
> Hi Christoph,
>
> Looking good, its unfortunate that the handling of mixed platform and
> utf string require jni up calls to invoke java methods.
>
> jni_util.c: line 216:
>
>You should not need to create an extra string; the string s is
> non-null and ready to use.
>
> jni_util.h:
>
>line 117-119, The original comment was just as informative as the
> replacement.
>
> The rest looks fine.
>
> Roger
>
> On 6/28/16 4:45 PM, Langer, Christoph wrote:
> >
> > Hi Paul,
> >
> > Ok, you kind of got me convinced and it is also a quite simple
> > modification. I changed from “java.net.SocketException: ioctl
> > SIOCGSIZIFCONF failed: Bad file number” to “java.net.SocketException:
> > Bad file number (ioctl SIOCGSIZIFCONF failed)” like you suggested.
> >
> > The update is in place:
> > http://cr.openjdk.java.net/~clanger/webrevs/8158023.3/
> > 
> >
> > Now I finally need a review…
> >
> > Best regards
> >
> > Christoph
> >
> > *From:*Paul Benedict [mailto:pbened...@apache.org]
> > *Sent:* Montag, 27. Juni 2016 18:15
> > *To:* Langer, Christoph 
> > *Cc:* Kenji Kazumura ; Chris Hegarty
> > ; nio-...@openjdk.java.net;
> > core-libs-dev@openjdk.java.net; net-...@openjdk.java.net
> > *Subject:* Re: RFR 8158023: SocketExceptions contain too little
> > information sometimes
> >
> > Christoph, I didn't understand your explanation on your message
> > preference. Typically root cause information is printed last, not
> > first. Another reason not to change the ordering of the exception
> > message is that applications may be looking at existing strings. For
> > this example, if I may presume "Bad file number" is an existing
> > message, I would defer to the possibility applications may be exist
> > that test for that message condition.
> >
> >
> > Cheers,
> > Paul
> >
> > On Mon, Jun 27, 2016 at 2:42 AM, Langer, Christoph
> > > wrote:
> >
> > Hi,
> >
> > eventually here is the - hopefully final - version of this fix:
> > http://cr.openjdk.java.net/~clanger/webrevs/8158023.3/
> > 
> >
> > Now I leave JNU_ThrowByNameWithLastError untouched and I've also
> > added conversion to the new function
> > JNU_ThrowByNameWithMessageAndLastError. I've replaced
> > JNU_ThrowByNameWithLastError with
> > JNU_ThrowByNameWithMessageAndLastError in the java/net coding
> > where I think it is appropriate (mostly in occasions when a
> > SocketException is thrown kind of generically). @Paul: thanks for
> > your suggestion regarding the output format but I would still
> > prefer an output like "java.net.SocketException: ioctl
> > SIOCGSIZIFCONF failed: Bad file number" vs. "
> > java.net.SocketException: Bad file number (ioctl SIOCGSIZIFCONF
> > failed)" as I'm always handing down a message to the new throw
> > routine.
> >
> > Hoping for a review :)
> >
> > Best regards
> > Christoph
> >
> > > -Original Message-
> > > From: Kenji Kazumura [mailto:k...@jp.fujitsu.com]
> > > Sent: Mittwoch, 8. Juni 2016 02:51
> > > To: Langer, Christoph 
> > > Cc: net-...@openjdk.java.net ;
> > nio-...@openjdk.java.net ; core-libs-
> > > d...@openjdk.java.net 
> > > Subject: Re: RFR 8158023: SocketExceptions contain too little
> > information
> > > sometimes
> > >
> > > Christoph,
> > >
> > > You should not remove conversion codes (platform string to Java
> > String)
> > > at JNU_ThrowByNameWithLastError,
> > > and you have to add conversion codes at
> > > JNU_ThrowByNameWithMessageAndLastError.
> > >
> > > It seems that you assume strerror returns only ascii characters,
> > but actually
> > > not.
> > > It depends on the locale of your environment where java programs
> > runs.
> > >
> > >
> > > -Kenji Kazumura
> > >
> > >
> > > In message
> > >
> 

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

[David DeHaven]

Fix looks good to me too.

-DrD-

> On Jun 29, 2016, at 12:54 PM, Gerard Ziemski  
> wrote:
> 
> hi Brent,
> 
> Thank you for fixing the original issue and for putting in this follow-up fix!
> 
> Looks good! (for full disclosure I proposed the workaround)
> 
> 
> cheers
> 
>> On Jun 29, 2016, at 2:36 PM, Brent Christian  
>> wrote:
>> 
>> Hi,
>> 
>> Please review the following change for JDK 9:
>> 
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8160370
>> Webrev:
>> http://cr.openjdk.java.net/~bchristi/8160370/webrev.00/
>> 
>> The fix for 7131356 fills in the "os.version" system property on Mac using 
>> [NSProcessInfo operatingSystemVersion], available starting in Mac OS 10.9.
>> 
>> While JDK 9 will not support versions of Mac OS prior to 10.9 [1], not all 
>> testing infrastructure has been updated, and we've seen failures of
>> 
>> java/lang/System/OsVersionTest.java
>> 
>> The code to restore behavior on older Mac systems is only a few lines, so 
>> that seems like a good way to get testing going again.
>> 
>> Thanks,
>> -Brent
>> 
>> 1. https://jdk9.java.net/jdk9_supported_platforms.html
>> 2. https://bugs.openjdk.java.net/browse/JDK-8156132
> 

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

[Gerard Ziemski]

hi Brent,

Thank you for fixing the original issue and for putting in this follow-up fix!

Looks good! (for full disclosure I proposed the workaround)


cheers

> On Jun 29, 2016, at 2:36 PM, Brent Christian  
> wrote:
> 
> Hi,
> 
> Please review the following change for JDK 9:
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8160370
> Webrev:
> http://cr.openjdk.java.net/~bchristi/8160370/webrev.00/
> 
> The fix for 7131356 fills in the "os.version" system property on Mac using 
> [NSProcessInfo operatingSystemVersion], available starting in Mac OS 10.9.
> 
> While JDK 9 will not support versions of Mac OS prior to 10.9 [1], not all 
> testing infrastructure has been updated, and we've seen failures of
> 
> java/lang/System/OsVersionTest.java
> 
> The code to restore behavior on older Mac systems is only a few lines, so 
> that seems like a good way to get testing going again.
> 
> Thanks,
> -Brent
> 
> 1. https://jdk9.java.net/jdk9_supported_platforms.html
> 2. https://bugs.openjdk.java.net/browse/JDK-8156132

RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

[Brent Christian]

Hi,

Please review the following change for JDK 9:

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

The fix for 7131356 fills in the "os.version" system property on Mac 
using [NSProcessInfo operatingSystemVersion], available starting in Mac 
OS 10.9.


While JDK 9 will not support versions of Mac OS prior to 10.9 [1], not 
all testing infrastructure has been updated, and we've seen failures of


java/lang/System/OsVersionTest.java

The code to restore behavior on older Mac systems is only a few lines, 
so that seems like a good way to get testing going again.


Thanks,
-Brent

1. https://jdk9.java.net/jdk9_supported_platforms.html
2. https://bugs.openjdk.java.net/browse/JDK-8156132

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Kim Barrett]

> On Jun 29, 2016, at 2:54 PM, Kim Barrett  wrote:
> 
> I think the problem may have existed long before modules, but simply
> wasn't noticed.  That is, I suspect trying the test case mentioned
> below with a pre-June-2014 fastdebug build (when Reference started
> initializing InterruptedException) might trip the same assert.  I'm
> presently trying to obtain a sufficiently old fastdebug build to run
> the experiment.

Nope, works fine with JDK 9 b01.

./jdk1.9.0/fastdebug/bin/java -XX:AutoBoxCacheMax=2147483647 -version
Error occurred during initialization of VM
java.lang.OutOfMemoryError: Requested array size exceeds VM limit
at java.lang.Integer$IntegerCache.(Integer.java:799)
at java.lang.Integer.valueOf(Integer.java:827)
at sun.misc.Signal.handle(Signal.java:169)
at java.lang.Terminator.setup(Terminator.java:60)
at java.lang.System.initializeSystemClass(System.java:1197)

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Kim Barrett]

> On Jun 28, 2016, at 7:23 PM, David Holmes  wrote:
> 
> On 29/06/2016 8:43 AM, Kim Barrett wrote:
>> Updated webrevs:
>> 
>> Full:
>> http://cr.openjdk.java.net/~kbarrett/8156500/jdk.01/
>> http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.01/
>> 
>> Incremental:
>> http://cr.openjdk.java.net/~kbarrett/8156500/jdk.01.inc/
> 
> Did Reference not work? Just curious. I used to be a qualified Java 
> programmer back in the Java 5 era, but wildcards were always a bit iffy :)

It did not, or at least not without more contortions than I wanted to
make.  The old code used , so I decided to fall back to that.

Specifically, the type parameter in the type of "ref" needs to be
compatible with the type parameter in the type of "q", and that seems
to be messy to make happen when wildcards are involved.  Having popXXX
return Reference and splitting most of the body of
tryHandlePending into a new parameterized helper

   boolean tryHandlePendingAux(Reference ref, boolean should_wait)

did seem to do the trick, but also seemed like it was trying too hard...

>> http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.01.inc/
>> 
>> Still investigating the initialization order for core exceptions.
> 
> I suspect it is as Coleen indicated that the module changes introduced the 
> new failure path that you hit. I did a quick check of the initialization 
> order in an old b50:
> 
> 33 Initializing 'java/lang/Throwable' (0x001780002990)
> ...
> 46 Initializing 'java/lang/Exception'(no method) (0x001780003158)
> 47 Initializing 'java/lang/InterruptedException'(no method) 
> (0x0017800178a8)
> 
> Compare that with a current build:
> 
> 60 Initializing 'java/lang/ref/Reference$ReferenceHandler' 
> (0x00080001e3f0)
> 61 Initializing 'java/lang/Throwable' (0x000829f8)
> 62 Initializing 'java/lang/Exception'(no method) (0x000831a8)
> 63 Initializing 'java/lang/InterruptedException'(no method) 
> (0x00080001e6e0)
> 64 Initializing 'java/lang/ref/PhantomReference'(no method) 
> (0x00086440)
> 
> Initialization of Throwable is much, much later (large parts of java.lang and 
> java.util are now initialized first!) and is obviously a direct consequence 
> of preinitializing InterruptedException.
> 
> So I would say that the module change did break this and that initialization 
> of Throwable (only) should be restored to a much higher place in the 
> initialization sequence.

I think the problem may have existed long before modules, but simply
wasn't noticed.  That is, I suspect trying the test case mentioned
below with a pre-June-2014 fastdebug build (when Reference started
initializing InterruptedException) might trip the same assert.  I'm
presently trying to obtain a sufficiently old fastdebug build to run
the experiment.

The thing that has changed is that we now have a test (ol' reliable
tickler of bugs TestOptionsWithRanges) that tries all kinds of wacky
configurations that had never been seen before.

It tripped the init order problem by trying -XX:AutoBoxCacheMax with a
value of jint_max, which led to an attempt to allocate an array whose
size exceeds the maximum allowed, which is reported as OOME with a
specialized message, which led to the assertion failure in
java_lang_Throwable::fill_in_stack_trace_of_preallocated_backtrace
because java_lang_Throwable::unassigned_stacktrace() == NULL.

Note that with a smaller -XX:AutoBoxCacheMax value of 1G we instead
get a VM shutdown during initialization, because we are trying to GC
before that's allowed, rather than trying to make an array with an
impossible size.

Aside: Reporting an attempt to allocate a mere 2G element objarray
using an OOME seems a bit quaint these days. :-)

Re: RFR: jsr166 jdk9 integration wave 7

[Roger Riggs]

Hi,

Several constructors of ForkJoinPool are modified with comments like " 
using defaults for all other parameters".
However, there there are no other parameters in each constructor in 
question.


It seems to imply a reference to the new full function constructor, that 
reference should be explicit

complete with a {@link...}

$.02, Roger


On 6/29/2016 11:38 AM, Daniel Fuchs wrote:

Hi Martin,

I was looking at the new constructor's API documentation
in ForkJoinPool - and  somehow got confused by the sentence:

2235  * @param maximumPoolSize [...
2241  * ...]  To
2240  * arrange the same value as is used by default for the common
2241  * pool, use {@code 256} plus the parallelism level.

I mean - this looks bizarre, until you understand that the
common pool has a maxSpares of 256 - which means that its
actual max core pool size is 256 + parallelism level
(am I getting that part right?).

I wonder if it would be worth expanding on the rationale
for that value?

I mean something like: "The maximum number of spare threads
used by the common pool is 256: to arrange the same value as is
used by default for the common pool, use {@code 256} plus the
parallelism level for {@code maximumPoolSize}."

best regards,

-- daniel



On 27/06/16 20:38, Martin Buchholz wrote:
jsr166 has been pervasively modified to use VarHandles, but there are 
some

other pending changes (that cannot be cleanly separated from VarHandle
conversion).  We expect this to be the last feature integration for 
jdk9.


http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/ 



We're asking Paul to do most of the review work here, as owner of
VarHandles JEP and as Oracle employee.
We need approval for API changes in

https://bugs.openjdk.java.net/browse/JDK-8157523
Various improvements to ForkJoin/SubmissionPublisher code

https://bugs.openjdk.java.net/browse/JDK-8080603
Replace Unsafe with VarHandle in java.util.concurrent classes

There is currently a VarHandle bootstrap problem with
ThreadLocal/AtomicInteger that causes
java/util/Locale/Bug4152725.java
to fail.  Again I'm hoping that Paul will figure out what to do.  In the
past rearranging the order of operations in  has worked for 
similar

problems.  It's not surprising we have problems, since j.u.c. needs
VarHandles initialized to do work, and j.l.invoke needs j.u.c. (notably
AtomicInteger and ConcurrentHashMap) to do work.  Maybe we need some 
very
low-level concurrency infrastructure that does not use VarHandles, 
only for

bootstrap?

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Mandy Chung]

> On Jun 28, 2016, at 3:43 PM, Kim Barrett  wrote:
> 
> Updated webrevs:
> 
> Full:
> http://cr.openjdk.java.net/~kbarrett/8156500/jdk.01/
> http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.01/
> 

The VM change does simplify the implementation a lot.  Does/should the 
reference processing and enqueue them in the pending list at a safepoint? Or 
does the enqueuing happen while other Java threads are running except that it 
holds Heap_lock?

My main concern is that the ReferenceHandler thread now has to make a VM upcall 
to dequeue a reference object for every iteration.  I believe this is what the 
original implementation tried to avoid.  Would it be feasible for 
popReferencePendingList to be intrinsified?  If it is infeasible, we should 
think through other alternatives for example like Peter’s suggestion  to get 
the entire pending list at a time.

Comments on Reference.java

 143 /* Atomically pop the next pending reference.  If should_wait is
 144  * true and there are no pending references, block until the
 145  * pending reference list becomes non-empty.  The GC adds
 146  * references to the list, and notifies waiting threads when
 147  * references are added.
 148  */
 149 private static native Reference 
popReferencePendingList(boolean should_wait);

The parameter name “should_wait” - I would avoid using the word “should” and 
`_`.  What about “await”?

The spec needs to specify:

What happens if the thread calling this method is interrupted?  What does it 
return?  Does it throw InterruptedException?  What happens to the thread’s 
interrupted status?

This method is significant and can you add @param, @return, @throws to make it 
a proper javadoc.   

This change is small but the implication is significant.  JDK-7103238 would be 
more appropriate for this change than JDK-8156400 and its synopsis that does 
not highlight what the changeset is about.

 
> Still investigating the initialization order for core exceptions.
> 

This would be good to understand what’s going on there.

Mandy

Re: RFR: jsr166 jdk9 integration wave 7

[Peter Levart]

I see it was intentional as it is described in the docs...

Is new behavior better?

Regards, Peter


On 06/29/2016 07:19 PM, Peter Levart wrote:

Hi Martin,

I'm looking at changes in StampedLock. I noticed that you have 
consistently (in 4 places) replaced usages of the following idiom:



1044 if (LockSupport.nextSecondarySeed() >= 0)
1045 --spins;

with the following:

1056 --spins;
1057 Thread.onSpinWait();


Was that intentional? It looks almost like you thought that 
LockSupport.nextSecondarySeed() >= 0 is always true and that it is 
used just to introduce some pause into the spin loop. It is only true 
in roughly half of invocations as nextSecondarySeed() has the full 
signed int span.


Regards, Peter


On 06/27/2016 09:38 PM, Martin Buchholz wrote:

jsr166 has been pervasively modified to use VarHandles, but there are some
other pending changes (that cannot be cleanly separated from VarHandle
conversion).  We expect this to be the last feature integration for jdk9.

http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/

We're asking Paul to do most of the review work here, as owner of
VarHandles JEP and as Oracle employee.
We need approval for API changes in

https://bugs.openjdk.java.net/browse/JDK-8157523
Various improvements to ForkJoin/SubmissionPublisher code

https://bugs.openjdk.java.net/browse/JDK-8080603
Replace Unsafe with VarHandle in java.util.concurrent classes

There is currently a VarHandle bootstrap problem with
ThreadLocal/AtomicInteger that causes
java/util/Locale/Bug4152725.java
to fail.  Again I'm hoping that Paul will figure out what to do.  In the
past rearranging the order of operations in  has worked for similar
problems.  It's not surprising we have problems, since j.u.c. needs
VarHandles initialized to do work, and j.l.invoke needs j.u.c. (notably
AtomicInteger and ConcurrentHashMap) to do work.  Maybe we need some very
low-level concurrency infrastructure that does not use VarHandles, only for
bootstrap?

Re: RFR: jsr166 jdk9 integration wave 7

[Peter Levart]

Hi Martin,

I'm looking at changes in StampedLock. I noticed that you have 
consistently (in 4 places) replaced usages of the following idiom:



1044 if (LockSupport.nextSecondarySeed() >= 0)
1045 --spins;

with the following:

1056 --spins;
1057 Thread.onSpinWait();


Was that intentional? It looks almost like you thought that 
LockSupport.nextSecondarySeed() >= 0 is always true and that it is used 
just to introduce some pause into the spin loop. It is only true in 
roughly half of invocations as nextSecondarySeed() has the full signed 
int span.


Regards, Peter


On 06/27/2016 09:38 PM, Martin Buchholz wrote:

jsr166 has been pervasively modified to use VarHandles, but there are some
other pending changes (that cannot be cleanly separated from VarHandle
conversion).  We expect this to be the last feature integration for jdk9.

http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/

We're asking Paul to do most of the review work here, as owner of
VarHandles JEP and as Oracle employee.
We need approval for API changes in

https://bugs.openjdk.java.net/browse/JDK-8157523
Various improvements to ForkJoin/SubmissionPublisher code

https://bugs.openjdk.java.net/browse/JDK-8080603
Replace Unsafe with VarHandle in java.util.concurrent classes

There is currently a VarHandle bootstrap problem with
ThreadLocal/AtomicInteger that causes
java/util/Locale/Bug4152725.java
to fail.  Again I'm hoping that Paul will figure out what to do.  In the
past rearranging the order of operations in  has worked for similar
problems.  It's not surprising we have problems, since j.u.c. needs
VarHandles initialized to do work, and j.l.invoke needs j.u.c. (notably
AtomicInteger and ConcurrentHashMap) to do work.  Maybe we need some very
low-level concurrency infrastructure that does not use VarHandles, only for
bootstrap?

Re: RFR(L): 8143211: provide bytecode intrinsics for loop and try/finally executors

[Vladimir Ivanov]

thank you, Claes, Vladimir, and Paul, for your reviews. See 
http://cr.openjdk.java.net/~mhaupt/8143211/webrev.01/ for the next round.

The difference to the previous version is, roughly, as follows. Rev 01 ...

... honours the possibility of there being more than one loop in a LambdaForm; 
InvokerBytecodeGenerator now has extra state summarising characteristics of the 
loops, while the concrete types are extracted from the LambdaForm during the 
actual compilation step.

... does away with the extra intrinsic data for loops. Instead, the 
LambdaFormEditor is used to create (and cache) an appropriate LambdaForm as a 
specialisation of a template. This was a very good suggestion from Vladimir 
(thanks!), and it makes the code much more tidy in my opinion.


I agree it looks better now. But I think LF structure can be improved 
further.


I'd prefer to see loopStateTypes attached to the intrinsic itself. Right 
now, it's unrelated and you look it up by offset. You can pass it as an 
additional parameter into MHI.loop instead and access it directly from 
the Name marked as the loop intrinsic.


LFE.noteLoopLocalTypesForm() looks fragile. It assumes the LF being 
edited contains only loop intrinsic and adds type info as the first 
non-argument Name. What if you explicitly pass loop intrinsic position 
into it, check that the Name represents a loop, and substitute loop 
types argument.


Such representation will support multiple loops inside a single LF and 
allow per-loop editing.


Regarding loopStarts, loopStateStarts, and currentLoop fields in IBG.

Since IBG iterates over the LF sequentially, you should be able to get 
rid of arrays and convert the position where loop state should be stored 
into a local variable in IBG.generateCustomizedCodeBytes().


The only problem is localsMapSize (which is eagerly allocated).
Current eager initialization logic looks over-complicated and I think it 
doesn't work as expected (mapLoopLocalState() overwrites locals which 
are used by Names following the loop).


I'd prefer suggest to see it simplified.

You can just do a single pass over the LF being compiled and sum up 
slots needed to represent loops state. After that, you can initialize 
them incrementally during compilation when you encounter a loop intrinsic.


If you put loop state after locals used for other LF Names, you don't 
need to adjust local slot indexes for non-loop Names.



Another thought: what if loop state is so large it overflows 65k locals 
limit? I don't see any checks of clauses array size in MHs.loop().



... contains various small fixes that were suggested.

Things I think should be expressed in other issues rather than being 
piggybacked on this one:
* Removing IntrinsicMethodHandle.
* Taking care of Transform cache cleanup.

I'm fine with that.

FTR IntrinsicMH removal was just an idea.

From the design perspective, attaching intrinsic info to MethodHandle 
doesn't look optimal. But I haven't done any experiments and don't know 
how it the code will look.


Best regards,
Vladimir Ivanov


Am 16.06.2016 um 15:17 schrieb Michael Haupt :

Dear all,

please review this change.
RFE: https://bugs.openjdk.java.net/browse/JDK-8143211
Webrev: http://cr.openjdk.java.net/~mhaupt/8143211/webrev.00/

The change puts the tryFinally and loop method handle combinator 
implementations, which were introduced as part of the JEP 274 effort, on a 
LambdaForm basis, which executes in bytecode generating (default) and 
LambdaForm interpretation 
(-Djava.lang.invoke.MethodHandle.COMPILE_THRESHOLD=-1) modes. It also changes 
the output formatting of LambdaForms, introducing a (hopefully) more readable 
format.

Thanks,

Michael

Re: RFR: jsr166 jdk9 integration wave 7

[Daniel Fuchs]

Hi Martin,

I was looking at the new constructor's API documentation
in ForkJoinPool - and  somehow got confused by the sentence:

2235  * @param maximumPoolSize [...
2241  * ...]  To
2240  * arrange the same value as is used by default for the common
2241  * pool, use {@code 256} plus the parallelism level.

I mean - this looks bizarre, until you understand that the
common pool has a maxSpares of 256 - which means that its
actual max core pool size is 256 + parallelism level
(am I getting that part right?).

I wonder if it would be worth expanding on the rationale
for that value?

I mean something like: "The maximum number of spare threads
used by the common pool is 256: to arrange the same value as is
used by default for the common pool, use {@code 256} plus the
parallelism level for {@code maximumPoolSize}."

best regards,

-- daniel



On 27/06/16 20:38, Martin Buchholz wrote:

jsr166 has been pervasively modified to use VarHandles, but there are some
other pending changes (that cannot be cleanly separated from VarHandle
conversion).  We expect this to be the last feature integration for jdk9.

http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/

We're asking Paul to do most of the review work here, as owner of
VarHandles JEP and as Oracle employee.
We need approval for API changes in

https://bugs.openjdk.java.net/browse/JDK-8157523
Various improvements to ForkJoin/SubmissionPublisher code

https://bugs.openjdk.java.net/browse/JDK-8080603
Replace Unsafe with VarHandle in java.util.concurrent classes

There is currently a VarHandle bootstrap problem with
ThreadLocal/AtomicInteger that causes
java/util/Locale/Bug4152725.java
to fail.  Again I'm hoping that Paul will figure out what to do.  In the
past rearranging the order of operations in  has worked for similar
problems.  It's not surprising we have problems, since j.u.c. needs
VarHandles initialized to do work, and j.l.invoke needs j.u.c. (notably
AtomicInteger and ConcurrentHashMap) to do work.  Maybe we need some very
low-level concurrency infrastructure that does not use VarHandles, only for
bootstrap?

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Peter Levart]

Hi again,

On 06/29/2016 03:30 PM, Peter Levart wrote:
Just to show what I mean, here's a simple optimization that doesn't 
use a private pendingList of references shared among callers of 
tryHandlePanding(true/false), but instead uses course-grained 
synchronization and waiting for tryHandlePanding(false) callers while 
ReferenceHandler is privately processing the whole list of pending 
references:


http://cr.openjdk.java.net/~plevart/misc/PendingReferenceHandling/webrev.02/ 




...and now, on top of above, for some more involved optimization that is 
most effective when a chunk of pending references is discovered where 
majority are destined for the same ReferenceQueue (as in our benchmark):


http://cr.openjdk.java.net/~plevart/misc/PendingReferenceHandling/webrev.03/

The results of benchmark:

Original JDK 9:

Benchmark(refCount)  Mode  Cnt 
Score  Error  Units
ReferenceEnqueueBench.dequeueReferences  10ss  100 38410.515 
± 1011.769  us/op


Patched (by Kim):

Benchmark(refCount)  Mode  Cnt 
Score  Error  Units
ReferenceEnqueueBench.dequeueReferences  10ss  100 42197.522 
± 1161.451  us/op


Proposed (by Peter, webrev):

Benchmark(refCount)  Mode  Cnt 
Score  Error  Units
ReferenceEnqueueBench.dequeueReferences  10ss  100 34134.977 
± 1274.753  us/op


Proposed (by Peter, webrev.02):

Benchmark(refCount)  Mode  Cnt 
Score  Error  Units
ReferenceEnqueueBench.dequeueReferences  10ss  100 27935.929 
± 1128.678  us/op


Proposed (by Peter, webrev.03):

Benchmark(refCount)  Mode  Cnt Score
Error  Units
ReferenceEnqueueBench.dequeueReferences  10ss  100 6603.009 
± 59.051  us/op



That's already 6x faster than baseline!

I'm not suggesting that any of these optimizations is applied. I just 
wanted to show that a JNI call that allows retrieving the whole pending 
list has potential to allow such optimizations.


Regards, Peter

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Peter Levart]

Hi Kim,


On 06/29/2016 01:22 PM, Peter Levart wrote:
Transfering the whole list in one JNI invocation has the potential for 
further optimizations on the Java side (like handling the whole popped 
list privately without additional synchronization - if we ever find a 
way for java.nio.Bits to wait for it reliably - or even enqueue-ing a 
chunk of consecutive references destined for the same queue using a 
single synchronized action on the queue, etc...) 


Just to show what I mean, here's a simple optimization that doesn't use 
a private pendingList of references shared among callers of 
tryHandlePanding(true/false), but instead uses course-grained 
synchronization and waiting for tryHandlePanding(false) callers while 
ReferenceHandler is privately processing the whole list of pending 
references:


http://cr.openjdk.java.net/~plevart/misc/PendingReferenceHandling/webrev.02/

This further improves benchmark results and it still passes the 
DirectBufferAllocTest:


Original JDK 9:

Benchmark(refCount)  Mode  Cnt 
Score  Error  Units
ReferenceEnqueueBench.dequeueReferences  10ss  100 38410.515 
± 1011.769  us/op


Patched (by Kim):

Benchmark(refCount)  Mode  Cnt 
Score  Error  Units
ReferenceEnqueueBench.dequeueReferences  10ss  100 42197.522 
± 1161.451  us/op


Proposed (by Peter, webrev):

Benchmark(refCount)  Mode  Cnt 
Score  Error  Units
ReferenceEnqueueBench.dequeueReferences  10ss  100 34134.977 
± 1274.753  us/op


Proposed (by Peter, webrev.02):

Benchmark(refCount)  Mode  Cnt 
Score  Error  Units
ReferenceEnqueueBench.dequeueReferences  10ss  100 27935.929 
± 1128.678  us/op



Regards, Peter

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Peter Levart]

Hi Kim,

Let me chime-in although it's a bit late...

I think this is a good change to finally get rid of OOME problems in 
this area.


On 06/28/2016 07:45 PM, Kim Barrett wrote:

On Jun 28, 2016, at 5:33 AM, Per Liden  wrote:
Patch looks good. The only thing I don't feel qualified to review is the 
initialization order change in thread.cpp, so I'll let others comments on that.

Thanks.  I’ll be following up on that area.


I like the pop-one-reference-at-a-time semantics, which simplifies things a lot 
and keeps the interface nice and clean. I was previously afraid that it might 
cause a noticeable performance degradation compared to lifting the whole list 
into Java in one go, but your testing seem to prove that's not the case.

I was concerned about that too, and had tried a different approach that also still 
supported the existing "some callers wait and others don’t" API, but it was a 
bit messy.  Coleen convinced me to try this (since it was easy) and do the measurement, 
and it worked out well.



The repeated JNI invocations do not present a big overhead, but it is 
measurable. For example, the following benchmark measures the time it 
takes after a GC cycle for a bunch of references to be transfered to 
Java side, enqueued into ReferenceQueue and dequeued from it:


http://cr.openjdk.java.net/~plevart/misc/PendingReferenceHandling/ReferenceEnqueueBench.java

The results on my i7-4771 PC:

Original JDK 9:

Benchmark(refCount)  Mode  Cnt 
Score  Error  Units
ReferenceEnqueueBench.dequeueReferences  10ss  100 38410.515 
± 1011.769  us/op


Patched (by Kim):

Benchmark(refCount)  Mode  Cnt 
Score  Error  Units
ReferenceEnqueueBench.dequeueReferences  10ss  100 42197.522 
± 1161.451  us/op



So, about 10% worse for this benchmark.

Transfering the whole list in one JNI invocation has the potential for 
further optimizations on the Java side (like handling the whole popped 
list privately without additional synchronization - if we ever find a 
way for java.nio.Bits to wait for it reliably - or even enqueue-ing a 
chunk of consecutive references destined for the same queue using a 
single synchronized action on the queue, etc...)


If the JNI API was something like the following:

/* Atomically pop the pending reference list if wholeList is true,
 * or just next pending reference if wholeList is false.
 * If wait is true and the pending reference list is empty, blocks 
until

 * it becomes non-empty, or returns null if wait is false.
 */
private static native Reference popReferencePendingList(boolean 
wholeList, boolean wait);



Then not too much complication is needed in Reference.java (diff to 
current JDK 9 sources) to consume this API and already have some benefit 
from it:


http://cr.openjdk.java.net/~plevart/misc/PendingReferenceHandling/webrev/

There is a possible race here between ReferenceHandler calling 
tryHandlePending(true) and java.nio.Bits calling tryHandlePending(false) 
that can make the method return false to java.nio.Bits when 
ReferenceHandler has just popped the whole list and not yet installed it 
in the private pendingList field, but java.nio.Bits makes several 
retries in this case with exponentially increasing delays, so that does 
not currently present a problem. 
java/nio/Buffer/DirectBufferAllocTest.java test passes with this change.


And the above benchmark shows improvement instead of regression:

Proposed (my Peter):

Benchmark(refCount)  Mode  Cnt 
Score  Error  Units
ReferenceEnqueueBench.dequeueReferences  10ss  100 34134.977 
± 1274.753  us/op



So what do you think? Is it worth the little additional logic on the 
Java side?



Regards, Peter

Re: RFR(L): 8143211: provide bytecode intrinsics for loop and try/finally executors

[Michael Haupt]

... small correction: the FC extension for this RFE has been *requested*.

Best,

Michael

> Am 29.06.2016 um 11:49 schrieb Michael Haupt :
> 
> Hi again,
> 
> as there have been no comments on the .01 version, I have updated it in place 
> with a new revision. This was necessary because there was a bug in 
> InvokerBytecodeGenerator that led to the benchmarks crashing. The current 
> revision, which is still available from 
> http://cr.openjdk.java.net/~mhaupt/8143211/webrev.01 
> , fixes this.
> 
> The bug consisted in a lack of CHECKCAST generation that would only occur if 
> the verifier was turned on (which it is not by default for 
> Unsafe.defineAnonymousClass), or if, voilà, C2 kicked in for compilation.
> 
> The benchmark results are attached. Explanation:
> * "unpatched" scores are without; "patched", with bytecode intrinsics (scores 
> are given in ns/op, i.e., smaller is better)
> * "bl" are baseline measurements; simple "bl" is plain Java, "blMH" is 
> invoking the constituent handles
> * "Cr" are handle creation measurements
> * "Inv" are handle invocation measurements
> * "Itr" are for iteratedLoop
> * "Cnt" are for countedLoop (with 3 and 4 handle arguments)
> * "Wh" and "DoWh" are for whileLoop and doWhileLoop
> * "L" are for plain loop
> * "TF" are for tryFinally
> 
> Observations:
> * considerable performance improvements for all creation and handle constructs
> * exception: iteratedLoop is slowed down
> 
> Proposal:
> * accept the change as is (RFE with FC extension)
> * deal with necessary further improvements as performance bugs
> 
> Reviews please! :-)
> 
> Thanks,
> 
> Michael
> 
>> Am 23.06.2016 um 16:43 schrieb Michael Haupt :
>> 
>> Dear all,
>> 
>> thank you, Claes, Vladimir, and Paul, for your reviews. See 
>> http://cr.openjdk.java.net/~mhaupt/8143211/webrev.01/ for the next round.
>> 
>> The difference to the previous version is, roughly, as follows. Rev 01 ...
>> 
>> ... honours the possibility of there being more than one loop in a 
>> LambdaForm; InvokerBytecodeGenerator now has extra state summarising 
>> characteristics of the loops, while the concrete types are extracted from 
>> the LambdaForm during the actual compilation step.
>> 
>> ... does away with the extra intrinsic data for loops. Instead, the 
>> LambdaFormEditor is used to create (and cache) an appropriate LambdaForm as 
>> a specialisation of a template. This was a very good suggestion from 
>> Vladimir (thanks!), and it makes the code much more tidy in my opinion.
>> 
>> ... contains various small fixes that were suggested.
>> 
>> Things I think should be expressed in other issues rather than being 
>> piggybacked on this one:
>> * Removing IntrinsicMethodHandle.
>> * Taking care of Transform cache cleanup.
>> 
>> Best,
>> 
>> Michael
>> 
>>> Am 16.06.2016 um 15:17 schrieb Michael Haupt :
>>> 
>>> Dear all,
>>> 
>>> please review this change.
>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8143211
>>> Webrev: http://cr.openjdk.java.net/~mhaupt/8143211/webrev.00/
>>> 
>>> The change puts the tryFinally and loop method handle combinator 
>>> implementations, which were introduced as part of the JEP 274 effort, on a 
>>> LambdaForm basis, which executes in bytecode generating (default) and 
>>> LambdaForm interpretation 
>>> (-Djava.lang.invoke.MethodHandle.COMPILE_THRESHOLD=-1) modes. It also 
>>> changes the output formatting of LambdaForms, introducing a (hopefully) 
>>> more readable format.
>>> 
>>> Thanks,
>>> 
>>> Michael

-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG | Schiffbauergasse 14 | 14467 Potsdam, Germany

ORACLE Deutschland B.V. & Co. KG | Hauptverwaltung: Riesstraße 25, D-80992 
München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V. | Hertogswetering 163/167, 
3543 AS Utrecht, Niederlande
Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
  Oracle is committed to developing 
practices and products that help protect the environment

Re: RFR(L): 8143211: provide bytecode intrinsics for loop and try/finally executors

[Michael Haupt]

Hi again,

as there have been no comments on the .01 version, I have updated it in place 
with a new revision. This was necessary because there was a bug in 
InvokerBytecodeGenerator that led to the benchmarks crashing. The current 
revision, which is still available from 
http://cr.openjdk.java.net/~mhaupt/8143211/webrev.01 
, fixes this.

The bug consisted in a lack of CHECKCAST generation that would only occur if 
the verifier was turned on (which it is not by default for 
Unsafe.defineAnonymousClass), or if, voilà, C2 kicked in for compilation.

The benchmark results are attached. Explanation:
* "unpatched" scores are without; "patched", with bytecode intrinsics (scores 
are given in ns/op, i.e., smaller is better)
* "bl" are baseline measurements; simple "bl" is plain Java, "blMH" is invoking 
the constituent handles
* "Cr" are handle creation measurements
* "Inv" are handle invocation measurements
* "Itr" are for iteratedLoop
* "Cnt" are for countedLoop (with 3 and 4 handle arguments)
* "Wh" and "DoWh" are for whileLoop and doWhileLoop
* "L" are for plain loop
* "TF" are for tryFinally

Observations:
* considerable performance improvements for all creation and handle constructs
* exception: iteratedLoop is slowed down

Proposal:
* accept the change as is (RFE with FC extension)
* deal with necessary further improvements as performance bugs

Reviews please! :-)

Thanks,

Michael

> Am 23.06.2016 um 16:43 schrieb Michael Haupt :
> 
> Dear all,
> 
> thank you, Claes, Vladimir, and Paul, for your reviews. See 
> http://cr.openjdk.java.net/~mhaupt/8143211/webrev.01/ for the next round.
> 
> The difference to the previous version is, roughly, as follows. Rev 01 ...
> 
> ... honours the possibility of there being more than one loop in a 
> LambdaForm; InvokerBytecodeGenerator now has extra state summarising 
> characteristics of the loops, while the concrete types are extracted from the 
> LambdaForm during the actual compilation step.
> 
> ... does away with the extra intrinsic data for loops. Instead, the 
> LambdaFormEditor is used to create (and cache) an appropriate LambdaForm as a 
> specialisation of a template. This was a very good suggestion from Vladimir 
> (thanks!), and it makes the code much more tidy in my opinion.
> 
> ... contains various small fixes that were suggested.
> 
> Things I think should be expressed in other issues rather than being 
> piggybacked on this one:
> * Removing IntrinsicMethodHandle.
> * Taking care of Transform cache cleanup.
> 
> Best,
> 
> Michael
> 
>> Am 16.06.2016 um 15:17 schrieb Michael Haupt :
>> 
>> Dear all,
>> 
>> please review this change.
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8143211
>> Webrev: http://cr.openjdk.java.net/~mhaupt/8143211/webrev.00/
>> 
>> The change puts the tryFinally and loop method handle combinator 
>> implementations, which were introduced as part of the JEP 274 effort, on a 
>> LambdaForm basis, which executes in bytecode generating (default) and 
>> LambdaForm interpretation 
>> (-Djava.lang.invoke.MethodHandle.COMPILE_THRESHOLD=-1) modes. It also 
>> changes the output formatting of LambdaForms, introducing a (hopefully) more 
>> readable format.
>> 
>> Thanks,
>> 
>> Michael


-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG | Schiffbauergasse 14 | 14467 Potsdam, Germany

ORACLE Deutschland B.V. & Co. KG | Hauptverwaltung: Riesstraße 25, D-80992 
München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V. | Hertogswetering 163/167, 
3543 AS Utrecht, Niederlande
Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
  Oracle is committed to developing 
practices and products that help protect the environment

Re: RFR 8160439: Replace asserts in VarHandle.AccessMode with tests

[Paul Sandoz]

> On 28 Jun 2016, at 21:37, Martin Buchholz  wrote:
> 
> Looks good.
> 

Thanks.


> I'm still nervous about using assert in performance critical code due to 
> hotspot bytecode count inlining heuristics.
> 

The assert i left in place (because it is asserting on internal data) and the 
one what was removed were not on the critical performance path. Stil, I learned 
a lesson to be careful using asserts with code that is used early in the 
startup process.

Paul.