Re: RFR[9]: 8158169: MethodHandles.dropArgumentsToMatch(...)

[shilpi.rast...@oracle.com]

Thanks Michael!!

Please see http://cr.openjdk.java.net/~srastogi/8158169/webrev.01/

DropArgumentsTest.java has method "dropArgumentsToMatchIAEData()" which 
throws IAE.


Regards,
Shilpi

On 6/28/2016 11:30 AM, Michael Haupt wrote:

Hi Shilpi,

in that case, please make sure to give the test that already covers the IAE a 
@bug annotation for this issue.

Thanks,

Michael


Am 27.06.2016 um 17:18 schrieb "shilpi.rast...@oracle.com" 
:

Thanks Michael and Paul for comments!!

Yes we already have negative test cases which throws IAE.
This test i have added to test the scenario where pos is equal to 
newTpes.size() and skip is equal to target.type.parameterList.size(). (positive)

Best Regards,
Shilpi

On 6/27/2016 6:42 PM, Paul Sandoz wrote:
Hi,



On 27 Jun 2016, at 14:22, shilpi.rast...@oracle.com wrote:

Hi All,

Please review fix for

https://bugs.openjdk.java.net/browse/JDK-8158169
http://cr.openjdk.java.net/~srastogi/8158169/webrev.00/

The test you added is not testing the constraints you added to the 
documentation? are those constraints already tested?

Paul.

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

[Per Liden]

Hi David,

On 2016-06-28 06:04, David Holmes wrote:

Hi Kim,

I like all the removal of the pending-list-locker related code, but
can't really comment on the details of how it was replaced and interacts
with all the GC code. The interface to the Reference class is fine, it
is the GC code that pushes into the reference queue that I can't really
comment on.

I have a couple of queries following up from Coleen's and Dan's reviews.

First, the synchronization of the pending-reference-list leaves me
somewhat perplexed. As Coleen noted you require the heap_lock to be held
but also use an Atomic::xchg. You have a comment:

+   // owns the lock.  Swap is used by parallel non-concurrent reference
+   // processing threads, where some higher level controller owns
+   // Heap_lock, so requires the lock is locked, but not necessarily by
+   // the current thread.

Can you clarify the higher-level protocol that is at play there.
Asserting that "someone" owns a lock seems only marginally useful if you
can't be sure who that owner is, or when they might release it. Some
more direct detecting of being within this higher-level protocol would
be better, if it is possible to detect.


The Heap_lock protects the pending list from access from the Reference 
Handler thread (or any other non-GC thread). During a GC, we grab the 
Heap_lock so that the GC then "owns" the list and is free to modify it. 
However, the GC itself potentially has many GC worker threads doing 
reference enqueuing. The atomic swap is there to allow concurrent 
insertion (prepending) into the list by these GC worker threads. So, 
each GC worker concurrently calls into 
ReferenceProcessor::enqueue_discovered_reflist(), with it's own/local 
list of references to enqueue onto the global pending list.


Hope that clarifies things.

cheers,
Per



---

As previously mentioned the change to the initialization order is a
concern to me - there are always unexpected consequences of making such
changes, no matter how straight-forward they seem at the time.
Pre-initialization of exception classes is not really essential as the
attempt to throw any of them from Java code will force initialization
anyway - it's only for exceptions created and thrown from the VM code
that direct initialization is needed. The problematic case is OOME
because throwing the OOME may trigger a secondary OOME during that class
initialization etc. Hence we try to deal with that by pre-allocating
instances that do not have stacktraces etc, so they can be thrown
safely. Hence my earlier comment that I think the real bug in your
situation is that gen_out_of_memory_error() is not considering how far
into the VM init sequence we are, and that it should be returning the
pre-allocated instance with no backtrace.

---

A query on the JDK side:

src/java.base/share/classes/java/lang/ref/Reference.java

  private static native Reference
popReferencePendingList(boolean wait);

I'm intrigued by the use of the lower-bounded wildcard using Object. As
Object has no super-types this looks very strange and should be
equivalent to the more common upper-bounded wildcard using Object ie
Reference or more concisely Reference. I see this
construct exists in the current code for the ReferenceQueue, but I am
quite intrigued as to why? (Someone getting ready for Any-types? :) )

Thanks,
David
-

On 25/06/2016 6:09 AM, Kim Barrett wrote:

Please review this change which moves the Reference pending list and
locking from the java.lang.ref.Reference class into the VM.  This
includes elimination of the ReferencePendingListLocker mechanism in
the VM. This fixes various fragility issues arising from having the
management of this list split between Java and the VM (GC).

This change addresses JDK-8156500 by eliminating the possibility of
suspension while holding the lock. Because the locking is now done in
the VM, we have full control over where suspension may occur.

This change retains the existing interface between the reference
processor and the nio.Bits package, e.g. Reference.tryHandlePending
has the same signature and behavior; this change just pushes the
pending list manipulation down into the VM.  There are open bugs
reports, enhancement requests, and discussions related to that
interface; see below. This change does not attempt to address them.

This change additionally addresses or renders moot
https://bugs.openjdk.java.net/browse/JDK-8055232
(ref) Exceptions while processing Reference pending list

and
https://bugs.openjdk.java.net/browse/JDK-7103238
Ensure pending list lock is held on behalf of GC before enqueuing
references on to the pending list

It is also relevant for followup discussion of
https://bugs.openjdk.java.net/browse/JDK-8022321
java/lang/ref/OOMEInReferenceHandler.java fails immediately

and
https://bugs.openjdk.java.net/browse/JDK-8149925
We don't need jdk.internal.ref.Cleaner any more - part 1

and has implications for:
https://bugs.openjdk.java.net/browse/JDK-8066859
java/lang/ref/OOMEInRef

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

[Per Liden]

Hi Dan,

On 2016-06-28 03:34, Daniel D. Daugherty wrote:
[...]


src/share/vm/gc/shared/vmGCOperations.cpp
 L103: Heap_lock->unlock();
 You did not add a conditional "Heap_lock->notify_all()" before
 the Heap_lock->unlock() like you've done in other places.
 Since you deleted a release_and_notify_pending_list_lock()
 call, I would think you need the:

   if (Universe::has_reference_pending_list()) {
 Heap_lock->notify_all();
   }


This path is only taken if the GC was skipped and never ran. In this 
case, we know we didn't discover and enqueued any new references on the 
pending list, hence no need to notify any waiting thread (the Reference 
Handler thread in this case).


cheers,
Per

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

[Per Liden]

Hi Kim,

On 2016-06-24 22:09, Kim Barrett wrote:

Please review this change which moves the Reference pending list and
locking from the java.lang.ref.Reference class into the VM.  This
includes elimination of the ReferencePendingListLocker mechanism in
the VM. This fixes various fragility issues arising from having the
management of this list split between Java and the VM (GC).

This change addresses JDK-8156500 by eliminating the possibility of
suspension while holding the lock. Because the locking is now done in
the VM, we have full control over where suspension may occur.

This change retains the existing interface between the reference
processor and the nio.Bits package, e.g. Reference.tryHandlePending
has the same signature and behavior; this change just pushes the
pending list manipulation down into the VM.  There are open bugs
reports, enhancement requests, and discussions related to that
interface; see below. This change does not attempt to address them.

This change additionally addresses or renders moot
https://bugs.openjdk.java.net/browse/JDK-8055232
(ref) Exceptions while processing Reference pending list

and
https://bugs.openjdk.java.net/browse/JDK-7103238
Ensure pending list lock is held on behalf of GC before enqueuing references on 
to the pending list

It is also relevant for followup discussion of
https://bugs.openjdk.java.net/browse/JDK-8022321
java/lang/ref/OOMEInReferenceHandler.java fails immediately

and
https://bugs.openjdk.java.net/browse/JDK-8149925
We don't need jdk.internal.ref.Cleaner any more - part 1

and has implications for:
https://bugs.openjdk.java.net/browse/JDK-8066859
java/lang/ref/OOMEInReferenceHandler.java failed with java.lang.Exception: 
Reference Handler thread died

In addition to the obviously relevant changes, there are a couple of
changes whose presence here might seem surprising and require further
explanation.

- Removal of a stale comment in create_vm, noticed because it was near
some code being removed as part of this change.  The comment was
rendered obsolete by JDK-8028275.

- Moved initialization of exception classes earlier in
initialize_java_lang_classes. The previous order only worked by
accident, at least for OOME.  During the bulk of the library
initialization, OOME may be thrown, perhaps due to poorly chosen
command line options.  That attempts to use one of the pre-allocated
OOME objects, and tries to fill in its stack trace.  If the Throwable
class is not yet initialized, this leads to a failed assert in the VM
because Throwable.UNASSIGNED_STACK still has a null value.  This
initialization order issue was being masked by the old pending list
implementation; the initialization of Reference ensured
InterruptedException was initialized (thereby initializing its
Throwable base class), and the initialization of Reference just
happened to occur early enough that Throwable was initialized by the
time it was needed when running certain tests.  The forced
initialization of InterruptedException is no longer needed by
Reference, but removal of it triggered test failures (and could
trigger corresponding product failures) because of this ordering
issue.

CR:
https://bugs.openjdk.java.net/browse/JDK-8156500


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.


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.


cheers,
Per



Webrev:
http://cr.openjdk.java.net/~kbarrett/8156500/jdk.00/
http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.00/

Testing:
RBT ad hoc nightly runtime & gc tests.

With the proposed patch for JDK-8153711 applied, locally ran nearly
35K iterations of the OomDebugTest that is part of that patch, with no
failures / deadlocks.  Without this change it would typically only take
on the order of 100 iterations to hit a deadlock failure.

Locally ran direct byte buffer allocation test:
jdk/test/java/nio/Buffer/DirectBufferAllocTest.java
This change reported 3% faster allocation times, and 1/2 the standard
deviation for allocation times.

Locally ran failing test from JDK-8022321 and JDK-8066859 a few
thousand times.
jdk/test/java/lang/ref/OOMEInReferenceHandler.java
No errors, but failures were noted as hard to reproduce.

Re: RFR: jsr166 jdk9 integration wave 7

[Paul Sandoz]

Hi Martin,

Thanks, i will review carefully this week and do the necessary paper work for 
the FC exception required for API changes and the CCC.

I found the cause of the test failure with java/util/Locale/Bug4152725.java.

The constructor of VarHandle.AccessMode has some assert statements. These 
asserts were getting executed and causing locale-based classes to get loaded 
early in the startup process. I don’t yet know the why the test fails but i am 
guessing some form of circular dependency in charset related code results in a 
null value that obliterates any non-default locale data.

I will replace the asserts with tests:

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


Paul.

> On 27 Jun 2016, at 21: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: JVM (1.8.0_91) crashes in Windows native code

[Alex Kashchenko]

Hi,

On 06/24/2016 03:03 PM, Alex Kashchenko wrote:

Hi Vinay,

On 06/23/2016 06:56 PM, Vinay Purohit wrote:

Hello,



I'm not sure if this is the right forum for posting an issue we are
experiencing with JRE 1.8.0_92.



The JVM crashes in the Windows native code and generates a log (see
attachment, hs_err_pid15724.log), but not the usual stack-trace due to
uncaught exceptions. The root cause is unclear and we haven't figured
out a
consistent way to reproduce the problem, but it appears related to the
use
of the java.io.File class for probing attributes of non-standard files
(junctions, symbolic links, or use of file names disallowed on Windows
due
to use of special chars, but allowed on other platforms like Mac/Linux).



The same Java code and JVM version on Mac OS X (10.10) runs ok,
suggesting
that the problem is Windows-related.



I have also created a bug report, JI-9040363. Other versions of the
JRE may
be susceptible as well, but haven't been tested against.



Any suggestions or insights would be appreciated.


Thanks for this bugreport. Would you mind posting the details of
JI-9040363 (it cannot be accessed outside of Oracle) and resending
hs_err_pid15724.log (inline, or uploading it somewhere - it wasn't come
through the list)?


According to the stacktrace (from a crash report that was sent to me 
privately - added below) the crash is probably caused by a WinAPI call 
from this function:


http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/b44d695f738b/src/windows/native/java/io/WinNTFileSystem_md.c#l226

This code is there for some years and it's not clear how it can cause 
the crash.


If the crash can be reproduced only on a virtual file system (mounted to 
a guest VM etc) then it can be caused by the problems in VFS driver 
(example of a similar crash: 
http://www.ghisler.ch/board/viewtopic.php?t=36410 ).


If the crash is on a bare metal windows instance and can be reproduced 
only on a single machine, it possibly may be caused by hardware problems.




#/ hs_err_pid15724.log 
*/

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  EXCEPTION_ACCESS_VIOLATION (0xc005) at pc=0x77069fe9, pid=15724, 
tid=19792

#
# JRE version: Java(TM) SE Runtime Environment (8.0_91-b14) (build 
1.8.0_91-b14)

# Java VM: Java HotSpot(TM) Client VM (25.91-b14 mixed mode windows-x86 )
# Problematic frame:
# C  [ntdll.dll+0x49fe9]
#
# Failed to write core dump. Minidumps are not enabled by default on 
client versions of Windows

#
# If you would like to submit a bug report, please visit:
#   http://bugreport.java.com/bugreport/crash.jsp
# The crash happened outside the Java Virtual Machine in native code.
# See problematic frame for where to report the bug.
#

---  T H R E A D  ---

Current thread (0x17090400):  JavaThread "Thread-39" [_thread_in_native, 
id=19792, stack(0x1b23,0x1b28)]


siginfo: ExceptionCode=0xc005, reading address 0x

Registers:
EAX=0x16508753, EBX=0x, ECX=0x005c, EDX=0x0050
ESP=0x1b27f34c, EBP=0x1b27f368, ESI=0x165086f0, EDI=0x00a0
EIP=0x77069fe9, EFLAGS=0x00010206

Top of Stack: (sp=0x1b27f34c)
0x1b27f34c:   1b27f428 1b27f480 00ac 00a200a0
0x1b27f35c:   165086f0   1b27f408
0x1b27f36c:   77069d87 115d846a  1b27f458
0x1b27f37c:    0003 0002 00b8
0x1b27f38c:   1b27f47c 0009 0050 165084bc
0x1b27f39c:   0009 0012 00a0 0016
0x1b27f3ac:    00b81328 1b27f428 1b27f444
0x1b27f3bc:   77060ea9   

Instructions: (pc=0x77069fe9)
0x77069fc9:   fe 5c 0f 84 9b fc ff ff b8 5c 00 00 00 66 89 04
0x77069fd9:   4b 41 89 4d a4 46 89 75 b0 e9 7b fe ff ff 8b cf
0x77069fe9:   83 c0 02 d1 e9 8d 0c 4e 3b c1 0f 83 5b fc ff ff
0x77069ff9:   66 8b 08 66 83 c9 20 0f b7 c9 83 f9 70 74 18 83


Register to memory mapping:

EAX=0x16508753 is an unknown value
EBX=0x is an unknown value
ECX=0x005c is an unknown value
EDX=0x0050 is an unknown value
ESP=0x1b27f34c is pointing into the stack for thread: 0x17090400
EBP=0x1b27f368 is pointing into the stack for thread: 0x17090400
ESI=0x165086f0 is an unknown value
EDI=0x00a0 is an unknown value


Stack: [0x1b23,0x1b28],  sp=0x1b27f34c,  free space=316k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, 
C=native code)

C  [ntdll.dll+0x49fe9]
C  [ntdll.dll+0x49d87]
C  [ntdll.dll+0x66070]
C  [KERNELBASE.dll+0x1802f]
C  [java.dll+0x7fd4]
C  [java.dll+0x80cd]
J 314364 C1 java.io.File.isDirectory()Z (43 bytes) @ 0x0292372c 
[0x02923640+0xec]

j  cbDirDataXferThread.dirDataBackup(Ljava/io/File;IZLTsFile;IZ)I+118
j  cbDirDataXferThread.doOneTask(ILjava/util/AbstractCollection;)I+342
j  Sync.doNext()I+371
j  cbDirDataXferThread.run()V+182
v  ~StubRoutines::call_stub
V  [jvm.dll+0x1594e5]
V  [jvm.dll+0x21f0ae]
V  [jvm.dll+0x15957e]
V  [jvm.dll+0x159706]
V  [jvm.dll+0x159777]
V  [jvm.dll+0xfdeff]
V  [

RFR 8160439: Replace asserts in VarHandle.AccessMode with tests

[Paul Sandoz]

Hi,

Please review:

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8160439-vh-access-mode-remove-assert/webrev/
 


This removes an assert that can load dependent locale-based classes too early 
in the startup process when say AtomicInteger is modified to use VarHandle (in 
a 166 webrev Martin has prepared). This appears to kibosh locale-behaviour 
which was thankfully caught in a test (java/util/Locale/Bug4152725.java).

The removed assert has been replaced with a test.

—

Separately there might be a more principled way to ensure VarHandle and 
dependent classes are loaded/initialized at a well defined point. Although in 
this case it would not have helped, it would help define a clearer barrier 
before which a VarHandle cannot be used.

Paul.

Re: RFR: jsr166 jdk9 integration wave 7

[Paul Sandoz]

Hi,

This first the part of my review that concentrates mostly on the implementation.

Minor stuff. I don’t claim to know enough about the changes in FJ/CF, so i 
especially focused on the VH changes for those classes.

Paul.

CompletableFutureTest
—

3383 public void testRejectingExecutor() {
3384 for (Integer v : new Integer[] { 1, null }) {
3385
3386 final CountingRejectingExecutor e = new 
CountingRejectingExecutor();


3473 public void testRejectingExecutorNeverInvoked() {
3474 final CountingRejectingExecutor e = new 
CountingRejectingExecutor();
3475
3476 for (Integer v : new Integer[] { 1, null }) {
3477
3478 final CompletableFuture complete = 
CompletableFuture.completedFuture(v);

No indent for code within the for loop block


ForkJoin
—

 571  * Style notes
 572  * ===
 573  *
 574  * Memory ordering relies mainly on VarHandles.  This can be
 575  * awkward and ugly, but also reflects the need to control
 576  * outcomes across the unusual cases that arise in very racy code
 577  * with very few invariants. So these explicit checks would exist
 578  * in some form anyway.  All fields are read into locals before
 579  * use, and null-checked if they are references.  This is usually
 580  * done in a "C"-like style of listing declarations at the heads
 581  * of methods or blocks, and using inline assignments on first
 582  * encounter.  Nearly all explicit checks lead to bypass/return,
 583  * not exception throws, because they may legitimately arise due
 584  * to cancellation/revocation during shutdown.

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


2257  * @param saturate if nonnull, a predicate invoked upon attempts

s/nonnull/non-null



StampedLock
—

I notice lots of use of @ReservedStackAccess (same for ReentrantReadWriteLock), 
including Fred who might wanna look too.


ConcurrentLinkedDeque
—

linkFirst uses HEAD.compareAndSet, where as linkLast uses 
TAIL.weakCompareAndSetVolatile, can the former use the weak variant too since, 
as commented, failure is OK.


AtomicInteger
—

 185 public final int incrementAndGet() {
 186 return (int)VALUE.getAndAdd(this, 1) + 1;
 187 }


You can use VALUE.addAndGet same for other similar methods here and in other 
classes.




> On 27 Jun 2016, at 21: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 8160439: Replace asserts in VarHandle.AccessMode with tests

[Roger Riggs]

Hi Paul,

Looks fine, very straightforward

Regards, Roger


On 6/28/2016 6:50 AM, Paul Sandoz wrote:

Hi,

Please review:

   
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8160439-vh-access-mode-remove-assert/webrev/
 


This removes an assert that can load dependent locale-based classes too early 
in the startup process when say AtomicInteger is modified to use VarHandle (in 
a 166 webrev Martin has prepared). This appears to kibosh locale-behaviour 
which was thankfully caught in a test (java/util/Locale/Bug4152725.java).

The removed assert has been replaced with a test.

—

Separately there might be a more principled way to ensure VarHandle and 
dependent classes are loaded/initialized at a well defined point. Although in 
this case it would not have helped, it would help define a clearer barrier 
before which a VarHandle cannot be used.

Paul.

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

[Vladimir Ivanov]

Looks good.

Best regards,
Vladimir Ivanov

On 6/28/16 1:50 PM, Paul Sandoz wrote:

Hi,

Please review:

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8160439-vh-access-mode-remove-assert/webrev/
 


This removes an assert that can load dependent locale-based classes too early 
in the startup process when say AtomicInteger is modified to use VarHandle (in 
a 166 webrev Martin has prepared). This appears to kibosh locale-behaviour 
which was thankfully caught in a test (java/util/Locale/Bug4152725.java).

The removed assert has been replaced with a test.

—

Separately there might be a more principled way to ensure VarHandle and 
dependent classes are loaded/initialized at a well defined point. Although in 
this case it would not have helped, it would help define a clearer barrier 
before which a VarHandle cannot be used.

Paul.

Re: RFR: jsr166 jdk9 integration wave 7

[Martin Buchholz]

On Tue, Jun 28, 2016 at 5:55 AM, Paul Sandoz  wrote:

>
>
> CompletableFutureTest
> —
>
> 3383 public void testRejectingExecutor() {
> 3384 for (Integer v : new Integer[] { 1, null }) {
> 3385
> 3386 final CountingRejectingExecutor e = new
> CountingRejectingExecutor();
>
>
> 3473 public void testRejectingExecutorNeverInvoked() {
> 3474 final CountingRejectingExecutor e = new
> CountingRejectingExecutor();
> 3475
> 3476 for (Integer v : new Integer[] { 1, null }) {
> 3477
> 3478 final CompletableFuture complete =
> CompletableFuture.completedFuture(v);
>
> No indent for code within the for loop block
>
>
This test class uses Weird Indentation intentionally.  But more locally
regular like this:

--- src/test/tck/CompletableFutureTest.java 27 Jun 2016 21:41:17 - 1.160
+++ src/test/tck/CompletableFutureTest.java 28 Jun 2016 14:46:25 -
@@ -3354,8 +3354,8 @@
  * Test submissions to an executor that rejects all tasks.
  */
 public void testRejectingExecutor() {
-for (Integer v : new Integer[] { 1, null }) {
-
+for (Integer v : new Integer[] { 1, null })
+{
 final CountingRejectingExecutor e = new
CountingRejectingExecutor();

 final CompletableFuture complete =
CompletableFuture.completedFuture(v);
@@ -3434,9 +3434,7 @@
 checkCompletedWithWrappedException(future, e.ex);

 assertEquals(futures.size(), e.count.get());
-
-}
-}
+}}

 /**
  * Test submissions to an executor that rejects all tasks, but
@@ -3444,10 +3442,10 @@
  * explicitly completed.
  */
 public void testRejectingExecutorNeverInvoked() {
+for (Integer v : new Integer[] { 1, null })
+{
 final CountingRejectingExecutor e = new
CountingRejectingExecutor();

-for (Integer v : new Integer[] { 1, null }) {
-
 final CompletableFuture complete =
CompletableFuture.completedFuture(v);
 final CompletableFuture incomplete = new
CompletableFuture<>();

@@ -3495,9 +3493,7 @@
 checkCompletedNormally(future, null);

 assertEquals(0, e.count.get());
-
-}
-}
+}}

 /**
  * toCompletableFuture returns this CompletableFuture.


>
> 2257  * @param saturate if nonnull, a predicate invoked upon attempts
>
> s/nonnull/non-null
>
>
Done.

Index: src/main/java/util/concurrent/ForkJoinPool.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/ForkJoinPool.java,v
retrieving revision 1.317
diff -u -r1.317 ForkJoinPool.java
--- src/main/java/util/concurrent/ForkJoinPool.java 17 Jun 2016 13:03:45
- 1.317
+++ src/main/java/util/concurrent/ForkJoinPool.java 28 Jun 2016 14:41:28
-
@@ -2225,7 +2225,7 @@
  * acceptable when submitted tasks cannot have dependencies
  * requiring additional threads.
  *
- * @param saturate if nonnull, a predicate invoked upon attempts
+ * @param saturate if non-null, a predicate invoked upon attempts
  * to create more than the maximum total allowed threads.  By
  * default, when a thread is about to block on a join or {@link
  * ManagedBlocker}, but cannot be replaced because the
Index: src/main/java/util/concurrent/locks/StampedLock.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/locks/StampedLock.java,v
retrieving revision 1.61
diff -u -r1.61 StampedLock.java
--- src/main/java/util/concurrent/locks/StampedLock.java 17 Jun 2016
13:03:20 - 1.61
+++ src/main/java/util/concurrent/locks/StampedLock.java 28 Jun 2016
14:41:28 -
@@ -1310,7 +1310,7 @@
  * AbstractQueuedSynchronizer (see its detailed explanation in AQS
  * internal documentation).
  *
- * @param node if nonnull, the waiter
+ * @param node if non-null, the waiter
  * @param group either node or the group node is cowaiting with
  * @param interrupted if already interrupted
  * @return INTERRUPTED if interrupted or Thread.interrupted, else zero

RFR(XS): 8160457: VersionProps.versionNumbers() is broken

[Volker Simonis]

Hi,

can somebody please review this trivial fix:

http://cr.openjdk.java.net/~simonis/webrevs/2016/8160457/
https://bugs.openjdk.java.net/browse/JDK-8160457

The problem is that VersionProps.versionNumbers() incorrectly parses a
Java version string because it doesn't skip the separating dots. The
current version only works for one digit versions like "9" but will
fail for any longer version string like for example "9.0.0.1" with:

at 
java.lang.NumberFormatException.forInputString(java.base@9.0.0.1-internal/NumberFormatException.java:65)
at 
java.lang.Integer.parseInt(java.base@9.0.0.1-internal/Integer.java:791)
at 
java.lang.VersionProps.versionNumbers(java.base@9.0.0.1-internal/VersionProps.java:76)
at 
java.lang.Runtime.version(java.base@9.0.0.1-internal/Runtime.java:940)

This also breaks the build which uses a newly built jdk for
bootstrapping if we set '--with-version-patch=1' for example.

An can you PLEASE, PLEASE finally do your internal/early access builds
with '--with-version-patch=1'. It seems really careless to me that you
introduce a new, up to four digit versioning schema but only test the
shortcut version with one digit. I wouldn't be surprised if a version
like "9.0.0.1" breaks more Java applications than the sun.misc.Unsafe
removal :)

Thank you and best regards,
Volker

RFR: JDK-8160459 - jlink minor code clean up

[Jim Laskey (Oracle)]

http://cr.openjdk.java.net/~jlaskey/8160459/webrev/index.html 

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

Re: RFR: JDK-8160459 - jlink minor code clean up

[Sundararajan Athijegannathan]

Looks good

-Sundar


On 6/28/2016 8:39 PM, Jim Laskey (Oracle) wrote:
> http://cr.openjdk.java.net/~jlaskey/8160459/webrev/index.html 
> 
> https://bugs.openjdk.java.net/browse/JDK-8160459 
> 
>

Re: RFR: JDK-8160459 - jlink minor code clean up

[Mandy Chung]

> On Jun 28, 2016, at 8:09 AM, Jim Laskey (Oracle)  
> wrote:
> 
> http://cr.openjdk.java.net/~jlaskey/8160459/webrev/index.html 
> 
> https://bugs.openjdk.java.net/browse/JDK-8160459 
> 
> 

 170 @SuppressWarnings("LeakingThisInConstructor”)

This should be taken out.

Otherwise looks fine.

Mandy

Re: RFR(XXS): 8149519: Investigate implementation of java.specification.version

[Volker Simonis]

Hi,

what about this bug?
It is still assigned to me and I would be happy to fix it as proposed in:

http://cr.openjdk.java.net/~simonis/webrevs/2016/8149519
https://bugs.openjdk.java.net/browse/JDK-8149519

If anybody has better ideas I'm open to transfer the bug to him :)

But I still think this should be fixed before JDK 9 will be shipped.
It's simply not right if 'java.specification.version' equals to
'java.version'.

Regards,
Volker

On Thu, Apr 28, 2016 at 6:34 AM, Iris Clark  wrote:
> Hi, Volker.
>
> Sorry for the delay.
>
>> Ping - shouldn't we fix this issue before JDK 9 Feature Complete?
>
> Ideally, yes.  I am hoping to get this resolved in the next few weeks.
>
> My first priority for Verona is JDK-8144062 which moves jdk.Version into a 
> standard API (Alan mentioned this bugid earlier in this thread).  That 
> absolutely must be completed before Feature Complete next month.
>
> Thanks,
> iris
>
> -Original Message-
> From: Volker Simonis [mailto:volker.simo...@gmail.com]
> Sent: Wednesday, April 27, 2016 1:28 AM
> To: Iris Clark
> Cc: Java Core Libs; verona-...@openjdk.java.net; Alex Buckley; Kumar 
> Srinivasan; Marvin Ma
> Subject: Re: RFR(XXS): 8149519: Investigate implementation of 
> java.specification.version
>
> Ping - shouldn't we fix this issue before JDK 9 Feature Complete?
>
> Could you please also comment on my remarks regarding the relation of
> java.lang.Package.getSpecificationVersion() to JEP and 223 and and my 
> question why the Version class is not in a standard java package.
>
> Thank you and best regards,
> Volker
>
>
> On Thu, Apr 7, 2016 at 12:11 PM, Volker Simonis  
> wrote:
>> On Wed, Apr 6, 2016 at 8:28 PM, Iris Clark  wrote:
>>> Hi, Volker.
>>>
>>> Sorry for the delay.  I agree that the old implementation isn't quite 
>>> correct.  I can't see us potentially having a JCP MR for a security or 
>>> patch release (9.0.0.X and 9.0.X respectively).
>>>
>>> I could see a MR for an very unusual minor release (9.X).  If we had an MR 
>>> there's no guarantee that we'd need to change the 
>>> java.specification.version system property.   However, in the event that we 
>>> did need to change the java.specification.version, it should match that 
>>> release's $MAJOR.$MINOR, even if it meant that we had a sequence of 
>>> specification version numbers with gaps.
>>>
>>> As an example, let's say that JDK 9 is released via umbrella JSR with 
>>> java.specification.value of "9".  The system property would remain at "9" 
>>> for all releases regardless of type until we choose to have a MR.  Should 
>>> that MR occur while we're working on minor release 9.3.X and there is a 
>>> need to change the value of java.specification.value, it would become "9.3" 
>>> and would remain so in every release until the next MR.
>>>
>>> While we haven't changed the system property recently, I think that we need 
>>> to future-proof ourselves a little bit for MRs as described above.
>>>
>>> Assuming that we change the syntax of java.specification.version to
>>> $MAJOR.$MINOR (zeros truncated, value dependent on JCP) then we need
>>> to make a similar change to the syntax of
>>> java.vm.specification.version.  [ Note that in the current
>>> implementation, I believe that the values of
>>> java.specification.version and java.vm.specification.version are tied
>>> to each other. ]
>>>
>>> Changing the syntax of java{.vm}?specification.version requires a CCC which 
>>> I will file once we have agreement on the necessary changes.
>>>
>>
>> Hi Iris,
>>
>> thanks for your comments. I don't think that using $MAJOR.$MINOR for
>> java.specification.version is a good solution. As far as I understand,
>> JEP 223 (i.e. the new version scheme) is an Oracle/OpenJDK
>> implementation detail. There is no JSR for this and it won't be
>> enforced trough a JCK/TCK test (please correct me if I'm wrong). The
>> new versioning schema references the JCP in that is says that the
>> $MAJOR number corresponds to the "Java SE Platform Specification"
>> number as specified by the JCP in the corresponding JSR. But not vice
>> versa - i.e. there's no JSR referencing JEP 223.
>>
>> JEP 223 also says that the $MINOR number will be increased if this is
>> mandated by a Maintenance Release of the relevant Platform
>> Specification (by the JCP). But as you correctly noticed, in reality,
>> $MINOR is expected to be increased frequently compared to the number
>> of Java SE Maintenance Releases (if there will be any at all). So if
>> the JCP should decide to publish a Maintenance Release, why should it
>> name if after the then actual $MINOR update release number of the
>> Oracle/OpenJDK. I think a natural choice for such a MR would be "9.1",
>> no difference at which update release version Oracle/OpenJDK will be
>> at that time.
>>
>> So I think it would be best to decouple java.specification.version
>> from the Java versioning schema. We can start with
>> java.specification.version == $MAJOR. If there should be a MR
>> sometimes

Re: RFR(XS): 8160457: VersionProps.versionNumbers() is broken

[Claes Redestad]

Hi,

I'm terribly sorry, it seems this fell out from my patch to 816 as I 
was juggling patches back and forth between my workstation and my 
laptop. I did test manually with --with-version-patch for various 
numbers and was certain this was in in the final version and didn't 
re-test.


I'd do prevIndex = index + 1, since ++index mutates index for no reason, 
but I'm good either way.


/Claes

On 2016-06-28 16:57, Volker Simonis wrote:

Hi,

can somebody please review this trivial fix:

http://cr.openjdk.java.net/~simonis/webrevs/2016/8160457/
https://bugs.openjdk.java.net/browse/JDK-8160457

The problem is that VersionProps.versionNumbers() incorrectly parses a
Java version string because it doesn't skip the separating dots. The
current version only works for one digit versions like "9" but will
fail for any longer version string like for example "9.0.0.1" with:

 at 
java.lang.NumberFormatException.forInputString(java.base@9.0.0.1-internal/NumberFormatException.java:65)
 at 
java.lang.Integer.parseInt(java.base@9.0.0.1-internal/Integer.java:791)
 at 
java.lang.VersionProps.versionNumbers(java.base@9.0.0.1-internal/VersionProps.java:76)
 at 
java.lang.Runtime.version(java.base@9.0.0.1-internal/Runtime.java:940)

This also breaks the build which uses a newly built jdk for
bootstrapping if we set '--with-version-patch=1' for example.

An can you PLEASE, PLEASE finally do your internal/early access builds
with '--with-version-patch=1'. It seems really careless to me that you
introduce a new, up to four digit versioning schema but only test the
shortcut version with one digit. I wouldn't be surprised if a version
like "9.0.0.1" breaks more Java applications than the sun.misc.Unsafe
removal :)

Thank you and best regards,
Volker

Re: RFR(XS): 8160457: VersionProps.versionNumbers() is broken

[Mandy Chung]

> On Jun 28, 2016, at 7:57 AM, Volker Simonis  wrote:
> 
> Hi,
> 
> can somebody please review this trivial fix:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160457/
> https://bugs.openjdk.java.net/browse/JDK-8160457
> 

+1

> The problem is that VersionProps.versionNumbers() incorrectly parses a
> Java version string because it doesn't skip the separating dots. The
> current version only works for one digit versions like "9" but will
> fail for any longer version string like for example "9.0.0.1" with:
> 
>at 
> java.lang.NumberFormatException.forInputString(java.base@9.0.0.1-internal/NumberFormatException.java:65)
>at 
> java.lang.Integer.parseInt(java.base@9.0.0.1-internal/Integer.java:791)
>at 
> java.lang.VersionProps.versionNumbers(java.base@9.0.0.1-internal/VersionProps.java:76)
>at 
> java.lang.Runtime.version(java.base@9.0.0.1-internal/Runtime.java:940)
> 
> This also breaks the build which uses a newly built jdk for
> bootstrapping if we set '--with-version-patch=1' for example.
> 
> An can you PLEASE, PLEASE finally do your internal/early access builds
> with '--with-version-patch=1’.

Iris and I both have asked to verify the builds with --with-version-patch.  

Claes - did you see the exception during your verification?

> It seems really careless to me that you
> introduce a new, up to four digit versioning schema but only test the
> shortcut version with one digit.

I brought up similar question on the testing side.  Automatic builds configured 
to build different version number could catch this and should be considered to 
do. 

Mandy

> I wouldn't be surprised if a version
> like "9.0.0.1" breaks more Java applications than the sun.misc.Unsafe
> removal :)
> 
> Thank you and best regards,
> Volker

Re: RFR(XS): 8160457: VersionProps.versionNumbers() is broken

[Mandy Chung]

That’s a great idea to add a test patching VersionProps generated with 
different version strings.

We should file a JBS issue and add such a new test.

Mandy

> On Jun 28, 2016, at 9:07 AM, Daniel Fuchs  wrote:
> 
> Hi,
> 
> WRT to testing - one thing that could be done (possibly in a
> followup patch) would be:
> 
> class VersionProps {
> 
>   ...
> 
>   static List parseVersionNumbers(String versionNumber) {
>   // parsing code goes there
>   }
> 
>   static List versionNumbers() {
>   return parseVersionNumbers(VERSION_NUMBER);
>   }
> 
>   ...
> }
> 
> that would allow writing/adding a unit test for the algorithm
> implemented in parseVersionNumbers (either using white-box
> with -Xpatch or using reflection and the proper
> incantations to invoke parseVersionNumbers from outside the
> package).
> 
> cheers,
> 
> -- daniel
> 
> 
> On 28/06/16 15:57, Volker Simonis wrote:
>> Hi,
>> 
>> can somebody please review this trivial fix:
>> 
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160457/
>> https://bugs.openjdk.java.net/browse/JDK-8160457
>> 
>> The problem is that VersionProps.versionNumbers() incorrectly parses a
>> Java version string because it doesn't skip the separating dots. The
>> current version only works for one digit versions like "9" but will
>> fail for any longer version string like for example "9.0.0.1" with:
>> 
>>at 
>> java.lang.NumberFormatException.forInputString(java.base@9.0.0.1-internal/NumberFormatException.java:65)
>>at 
>> java.lang.Integer.parseInt(java.base@9.0.0.1-internal/Integer.java:791)
>>at 
>> java.lang.VersionProps.versionNumbers(java.base@9.0.0.1-internal/VersionProps.java:76)
>>at 
>> java.lang.Runtime.version(java.base@9.0.0.1-internal/Runtime.java:940)
>> 
>> This also breaks the build which uses a newly built jdk for
>> bootstrapping if we set '--with-version-patch=1' for example.
>> 
>> An can you PLEASE, PLEASE finally do your internal/early access builds
>> with '--with-version-patch=1'. It seems really careless to me that you
>> introduce a new, up to four digit versioning schema but only test the
>> shortcut version with one digit. I wouldn't be surprised if a version
>> like "9.0.0.1" breaks more Java applications than the sun.misc.Unsafe
>> removal :)
>> 
>> Thank you and best regards,
>> Volker
>> 
> 

Re: RFR(XS): 8160457: VersionProps.versionNumbers() is broken

[Daniel Fuchs]

Hi,

WRT to testing - one thing that could be done (possibly in a
followup patch) would be:

class VersionProps {

   ...

   static List parseVersionNumbers(String versionNumber) {
   // parsing code goes there
   }

   static List versionNumbers() {
   return parseVersionNumbers(VERSION_NUMBER);
   }

   ...
}

that would allow writing/adding a unit test for the algorithm
implemented in parseVersionNumbers (either using white-box
with -Xpatch or using reflection and the proper
incantations to invoke parseVersionNumbers from outside the
package).

cheers,

-- daniel


On 28/06/16 15:57, Volker Simonis wrote:

Hi,

can somebody please review this trivial fix:

http://cr.openjdk.java.net/~simonis/webrevs/2016/8160457/
https://bugs.openjdk.java.net/browse/JDK-8160457

The problem is that VersionProps.versionNumbers() incorrectly parses a
Java version string because it doesn't skip the separating dots. The
current version only works for one digit versions like "9" but will
fail for any longer version string like for example "9.0.0.1" with:

at 
java.lang.NumberFormatException.forInputString(java.base@9.0.0.1-internal/NumberFormatException.java:65)
at 
java.lang.Integer.parseInt(java.base@9.0.0.1-internal/Integer.java:791)
at 
java.lang.VersionProps.versionNumbers(java.base@9.0.0.1-internal/VersionProps.java:76)
at 
java.lang.Runtime.version(java.base@9.0.0.1-internal/Runtime.java:940)

This also breaks the build which uses a newly built jdk for
bootstrapping if we set '--with-version-patch=1' for example.

An can you PLEASE, PLEASE finally do your internal/early access builds
with '--with-version-patch=1'. It seems really careless to me that you
introduce a new, up to four digit versioning schema but only test the
shortcut version with one digit. I wouldn't be surprised if a version
like "9.0.0.1" breaks more Java applications than the sun.misc.Unsafe
removal :)

Thank you and best regards,
Volker

RE: RFR(XS): 8160457: VersionProps.versionNumbers() is broken

[Iris Clark]

Hi, Volker.

> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160457/

I think this change is fine.

Thanks,
iris

Re: RFR: jsr166 jdk9 integration wave 7

[Martin Buchholz]

On Tue, Jun 28, 2016 at 5:55 AM, Paul Sandoz  wrote:

>
> AtomicInteger
> —
>
>  185 public final int incrementAndGet() {
>  186 return (int)VALUE.getAndAdd(this, 1) + 1;
>  187 }
>
>
> You can use VALUE.addAndGet same for other similar methods here and in
> other classes.
>

Done:

 Index: src/main/java/util/concurrent/atomic/AtomicInteger.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/atomic/AtomicInteger.java,v
retrieving revision 1.47
diff -u -U 1 -r1.47 AtomicInteger.java
--- src/main/java/util/concurrent/atomic/AtomicInteger.java 17 Jun 2016
14:27:58 - 1.47
+++ src/main/java/util/concurrent/atomic/AtomicInteger.java 28 Jun 2016
16:28:39 -
@@ -156,3 +156,3 @@
 public final int incrementAndGet() {
-return (int)VALUE.getAndAdd(this, 1) + 1;
+return (int)VALUE.addAndGet(this, 1);
 }
@@ -165,3 +165,3 @@
 public final int decrementAndGet() {
-return (int)VALUE.getAndAdd(this, -1) - 1;
+return (int)VALUE.addAndGet(this, -1);
 }
@@ -175,3 +175,3 @@
 public final int addAndGet(int delta) {
-return (int)VALUE.getAndAdd(this, delta) + delta;
+return (int)VALUE.addAndGet(this, delta);
 }
Index: src/main/java/util/concurrent/atomic/AtomicIntegerArray.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/atomic/AtomicIntegerArray.java,v
retrieving revision 1.48
diff -u -U 1 -r1.48 AtomicIntegerArray.java
--- src/main/java/util/concurrent/atomic/AtomicIntegerArray.java 17 Jun
2016 14:27:58 - 1.48
+++ src/main/java/util/concurrent/atomic/AtomicIntegerArray.java 28 Jun
2016 16:28:39 -
@@ -170,3 +170,3 @@
 public final int incrementAndGet(int i) {
-return (int)AA.getAndAdd(array, i, 1) + 1;
+return (int)AA.addAndGet(array, i, 1);
 }
@@ -180,3 +180,3 @@
 public final int decrementAndGet(int i) {
-return (int)AA.getAndAdd(array, i, -1) - 1;
+return (int)AA.addAndGet(array, i, -1);
 }
@@ -191,3 +191,3 @@
 public final int addAndGet(int i, int delta) {
-return (int)AA.getAndAdd(array, i, delta) + delta;
+return (int)AA.addAndGet(array, i, delta);
 }
Index: src/main/java/util/concurrent/atomic/AtomicLong.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/atomic/AtomicLong.java,v
retrieving revision 1.53
diff -u -U 1 -r1.53 AtomicLong.java
--- src/main/java/util/concurrent/atomic/AtomicLong.java 17 Jun 2016
14:27:58 - 1.53
+++ src/main/java/util/concurrent/atomic/AtomicLong.java 28 Jun 2016
16:28:39 -
@@ -171,3 +171,3 @@
 public final long incrementAndGet() {
-return (long)VALUE.getAndAdd(this, 1L) + 1L;
+return (long)VALUE.addAndGet(this, 1L);
 }
@@ -180,3 +180,3 @@
 public final long decrementAndGet() {
-return (long)VALUE.getAndAdd(this, -1L) - 1L;
+return (long)VALUE.addAndGet(this, -1L);
 }
@@ -190,3 +190,3 @@
 public final long addAndGet(long delta) {
-return (long)VALUE.getAndAdd(this, delta) + delta;
+return (long)VALUE.addAndGet(this, delta);
 }
Index: src/main/java/util/concurrent/atomic/AtomicLongArray.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/atomic/AtomicLongArray.java,v
retrieving revision 1.48
diff -u -U 1 -r1.48 AtomicLongArray.java
--- src/main/java/util/concurrent/atomic/AtomicLongArray.java 17 Jun 2016
14:27:58 - 1.48
+++ src/main/java/util/concurrent/atomic/AtomicLongArray.java 28 Jun 2016
16:28:39 -
@@ -169,3 +169,3 @@
 public final long incrementAndGet(int i) {
-return (long)AA.getAndAdd(array, i, 1L) + 1L;
+return (long)AA.addAndGet(array, i, 1L);
 }
@@ -179,3 +179,3 @@
 public final long decrementAndGet(int i) {
-return (long)AA.getAndAdd(array, i, -1L) - 1L;
+return (long)AA.addAndGet(array, i, -1L);
 }
@@ -190,3 +190,3 @@
 public long addAndGet(int i, long delta) {
-return (long)AA.getAndAdd(array, i, delta) + delta;
+return (long)AA.addAndGet(array, i, delta);
 }

Re: RFR: jsr166 jdk9 integration wave 7

[Doug Lea]

On 06/28/2016 12:38 PM, Martin Buchholz wrote:



On Tue, Jun 28, 2016 at 5:55 AM, Paul Sandoz mailto:paul.san...@oracle.com>> wrote:



You can use VALUE.addAndGet same for other similar methods here and in other
classes.


Done:


And, amazingly, not in conflict with a pass I just did to delegate
all atomics-related specs to VarHandle, so that in the future, all
memory-model clarifications can be done in VarHandle without needing
to update others.

-Doug

Re: RFR: jsr166 jdk9 integration wave 7

[Martin Buchholz]

On Tue, Jun 28, 2016 at 5:55 AM, Paul Sandoz  wrote:

> ConcurrentLinkedDeque
> —
>
> linkFirst uses HEAD.compareAndSet, where as linkLast uses
> TAIL.weakCompareAndSetVolatile, can the former use the weak variant too
> since, as commented, failure is OK.
>

Well spotted!

 --- src/main/java/util/concurrent/ConcurrentLinkedDeque.java 17 Jun 2016
13:03:45 - 1.73
+++ src/main/java/util/concurrent/ConcurrentLinkedDeque.java 28 Jun 2016
16:59:10 -
@@ -300,8 +300,8 @@
 // Successful CAS is the linearization point
 // for e to become an element of this deque,
 // and for newNode to become "live".
-if (p != h) // hop two nodes at a time
-HEAD.compareAndSet(this, h, newNode);  //
Failure OK.
+if (p != h) // hop two nodes at a time; failure is
OK
+HEAD.weakCompareAndSetVolatile(this, h,
newNode);
 return;
 }
 // Lost CAS race to another thread; re-read prev

RFR: 8056285: java/util/logging/CheckLockLocationTest.java java.lang.RuntimeException: Test failed: should have been able to create FileHandler for %t/writable-dir/log.log in writable directory.

[Daniel Fuchs]

Hi,

JDK-8056285 has been sighted again (sigh).
I strongly suspect a configuration issue but I have
no proof. Here is an attempt at gathering some more
data to nail down the root cause of the issue.

JBS
https://bugs.openjdk.java.net/browse/JDK-8056285

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

best regards,

-- daniel

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

[Kim Barrett]

> 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.

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

[Kim Barrett]

> On Jun 28, 2016, at 12:04 AM, David Holmes  wrote:
> 
> Hi Kim,
> 
> I like all the removal of the pending-list-locker related code, but can't 
> really comment on the details of how it was replaced and interacts with all 
> the GC code. The interface to the Reference class is fine, it is the GC code 
> that pushes into the reference queue that I can't really comment on.

Thanks David.

> I have a couple of queries following up from Coleen's and Dan's reviews.
> 
> First, the synchronization of the pending-reference-list leaves me somewhat 
> perplexed. As Coleen noted you require the heap_lock to be held but also use 
> an Atomic::xchg. You have a comment:
> 
> +   // owns the lock.  Swap is used by parallel non-concurrent reference
> +   // processing threads, where some higher level controller owns
> +   // Heap_lock, so requires the lock is locked, but not necessarily by
> +   // the current thread.
> 
> Can you clarify the higher-level protocol that is at play there. Asserting 
> that "someone" owns a lock seems only marginally useful if you can't be sure 
> who that owner is, or when they might release it. Some more direct detecting 
> of being within this higher-level protocol would be better, if it is possible 
> to detect.

Was Per’s explanation sufficient?

> As previously mentioned the change to the initialization order is a concern 
> to me - there are always unexpected consequences of making such changes, no 
> matter how straight-forward they seem at the time. Pre-initialization of 
> exception classes is not really essential as the attempt to throw any of them 
> from Java code will force initialization anyway - it's only for exceptions 
> created and thrown from the VM code that direct initialization is needed. The 
> problematic case is OOME because throwing the OOME may trigger a secondary 
> OOME during that class initialization etc. Hence we try to deal with that by 
> pre-allocating instances that do not have stacktraces etc, so they can be 
> thrown safely. Hence my earlier comment that I think the real bug in your 
> situation is that gen_out_of_memory_error() is not considering how far into 
> the VM init sequence we are, and that it should be returning the 
> pre-allocated instance with no backtrace.

I'm also wondering why the existing order must have worked before the
pre-initialization of InterruptedException by Reference, but now
doesn't when we take that out.  I'll see if I can figure out what's
going on there.  It might be some bug was introduced but was being
masked by the Reference initialization.

> ---
> 
> A query on the JDK side:
> 
> src/java.base/share/classes/java/lang/ref/Reference.java
> 
> private static native Reference 
> popReferencePendingList(boolean wait);
> 
> I'm intrigued by the use of the lower-bounded wildcard using Object. As 
> Object has no super-types this looks very strange and should be equivalent to 
> the more common upper-bounded wildcard using Object ie Reference Object> or more concisely Reference. I see this construct exists in the 
> current code for the ReferenceQueue, but I am quite intrigued as to why? 
> (Someone getting ready for Any-types? :) )

I botched that. I meant to have a real Java programmer (which I don't
qualify as) look at that part of the change before sending it out for
review, but seem to have forgotten to do so. And lulled into a false
sense of security, since neither the compiler nor the runtime
complained, unlike the reams of template instantiation error output or
segfaults at runtime that I'd expect with C++...

Re: RFR 8054213: Class name repeated in output of Type.toString()

[Svetlana Nikandrova]

May be someone can find a minute?

On 27.06.2016 21:25, Svetlana Nikandrova wrote:

Kindly reminder

On 24.06.2016 14:58, Svetlana Nikandrova wrote:

Hello,

let me try again with updated version of webrev:
Webrev:
http://cr.openjdk.java.net/~snikandrova/8054213/webrev.01/ 


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

Many thanks goes to Sergey Ustimenko for his valuable advises. I 
believe fix looks nicer now.

JPRT tested.

Thank you,
Svetlana

On 17.06.2016 21:45, Svetlana Nikandrova wrote:

Hello,

could you please review this fix for toString() method of 
ParameterizedTypeImpl.
The problem is that when we obtain simple name of nested type shared 
prefix is only removed for ParameterizedType owner. We need to 
remove it for other cases too.


Please note that I also changed delimiter between outer and inner 
class from "." to "$". I believe as "$" is usually used to separate 
nested class name "." looks inconsistent and confusing.

Let me know if you have any objections.

Webrev:
http://cr.openjdk.java.net/~msolovie/8054213/webrev.00/ 


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

Thank you,
Svetlana

Re: RFR 8054213: Class name repeated in output of Type.toString()

[joe darcy]

Hello Svetlana,

I'm not convinced '$' should be replaced with '.' in this context as '.' 
is what the separator used in source code.


In any case, the test should be restructured. I recommend declaring an 
annotation type to hold the expected to string output. You can them loop 
over the methods from the class object and for the methods with the 
annotation verifying the toString output is as expected. For a guide see


test/java/lang/reflect/Constructor/GenericStringTest.java

Thanks,

-Joe


On 6/28/2016 11:25 AM, Svetlana Nikandrova wrote:

May be someone can find a minute?

On 27.06.2016 21:25, Svetlana Nikandrova wrote:

Kindly reminder

On 24.06.2016 14:58, Svetlana Nikandrova wrote:

Hello,

let me try again with updated version of webrev:
Webrev:
http://cr.openjdk.java.net/~snikandrova/8054213/webrev.01/ 


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

Many thanks goes to Sergey Ustimenko for his valuable advises. I 
believe fix looks nicer now.

JPRT tested.

Thank you,
Svetlana

On 17.06.2016 21:45, Svetlana Nikandrova wrote:

Hello,

could you please review this fix for toString() method of 
ParameterizedTypeImpl.
The problem is that when we obtain simple name of nested type 
shared prefix is only removed for ParameterizedType owner. We need 
to remove it for other cases too.


Please note that I also changed delimiter between outer and inner 
class from "." to "$". I believe as "$" is usually used to separate 
nested class name "." looks inconsistent and confusing.

Let me know if you have any objections.

Webrev:
http://cr.openjdk.java.net/~msolovie/8054213/webrev.00/ 


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

Thank you,
Svetlana

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

[Martin Buchholz]

Looks good.

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

On Tue, Jun 28, 2016 at 3:50 AM, Paul Sandoz  wrote:

> Hi,
>
> Please review:
>
>
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8160439-vh-access-mode-remove-assert/webrev/
> <
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8160439-vh-access-mode-remove-assert/webrev/
> >
>
> This removes an assert that can load dependent locale-based classes too
> early in the startup process when say AtomicInteger is modified to use
> VarHandle (in a 166 webrev Martin has prepared). This appears to kibosh
> locale-behaviour which was thankfully caught in a test
> (java/util/Locale/Bug4152725.java).
>
> The removed assert has been replaced with a test.
>
> —
>
> Separately there might be a more principled way to ensure VarHandle and
> dependent classes are loaded/initialized at a well defined point. Although
> in this case it would not have helped, it would help define a clearer
> barrier before which a VarHandle cannot be used.
>
> Paul.
>

RE: RFR 8158023: SocketExceptions contain too little information sometimes

[Langer, Christoph]

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 
mailto:christoph.lan...@sap.com>> 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 
mailto:christoph.lan...@sap.com>>
   > 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
   > 
mailto:decc19cdab854bbeac7126cb8e236...@dewdfe13de11.global.corp.sap>>
   >RFR 8158023: SocketExceptions contain too little information sometimes
   >"Langer, Christoph" 
mailto:christoph.lan...@sap.com>> wrote:
   >
   > > Hi all,
   > >
   > > please review the following change:
   > > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8158023.1/
   > > Bug: https://bugs.openjdk.java.net/browse/JDK-8158023
   > >
   > > During error analysis I stumbled over a place where I encountered a
   > SocketException which was thrown along with some strerror information as
   > message. I found it hard to find the originating code spot with that 
information.
   > >
   > > So I looked at the places where we throw exceptions, namely JNU_Throw...
   > and NET_Throw... functions and came up with the following enhancement:
   > > - NET_ThrowByNameWithLastError can go completely as it does not provide
   > any benefit over JNU_ThrowByNameWithLastError.
   > > - JNU_ThrowByNameWithLastError can be cleaned up.
   > >
   > > - I added JNU_ThrowByNameWithMessageAndLastError to print out a string
   > like message + ": " + last error.
   > >
   > > - I went over all places where NET_ThrowByNameWithLastError is used and
   > replaced it appropriately.
   > >
   > > Do you think this change is desirable/possible?
   > >
   > > Though it's mainly a net topic, I'm posting it to nio-dev and 
core-libs-dev as
   > well as JNU_Throw... code affects all.
   > >
   > > Best regards
   > > Christoph
   > >

RFR JDK-6233323: ZipEntry.isDirectory() may return false incorrectly

[Xueming Shen]

Hi,

Please help codereview the change for  JDK-6233323/8144977

issue:
https://bugs.openjdk.java.net/browse/JDK-6233323
https://bugs.openjdk.java.net/browse/JDK-8144977

webrev: http://cr.openjdk.java.net/~sherman/62333233_8144977/webrev

This is an implementation issue from day-one. ZIP spec specifies the
name of a directory entry must be ended with a slash, so a directory
name "dir" in ZIP world should have the name "dir/".

ZipFile.getEntry() works well with these type of "canonicalized" directory
names. However, its implementation also tries to be "liberal" when
taking name that doesn't end with "/". Where there is no such entry (file)
found, it tries a second lookup with a "/" appended, and returns such entry
if find one. The original implementation always to use the original passed
in name (the one without the ending slash) as the entry name for the
returned ZipEntry. This is fine if the entry is from the first round of lookup,
in which the name stored in zip file matches the passed in name and to
use the name directly obviously helps avoiding creating another String
object from the binary name (byte[] in zip entry). But in case of the entry
is from the second round lookup, the direct consequence is the isDirectory()
method of the returned entry always returns false (ZipEntry.isDirectory()
implementation simply checks if the name ends with / or not), even it is
actually a directory entry.

The fix is to simply return the name stored in the zip file. It appears this
should be the desired and expected result.

Thanks,
Sherman

Re: RFR JDK-6233323: ZipEntry.isDirectory() may return false incorrectly

[Roger Riggs]

Looks fine,
Roger


On 6/28/2016 4:58 PM, Xueming Shen wrote:

Hi,

Please help codereview the change for  JDK-6233323/8144977

issue:
https://bugs.openjdk.java.net/browse/JDK-6233323
https://bugs.openjdk.java.net/browse/JDK-8144977

webrev: http://cr.openjdk.java.net/~sherman/62333233_8144977/webrev

This is an implementation issue from day-one. ZIP spec specifies the
name of a directory entry must be ended with a slash, so a directory
name "dir" in ZIP world should have the name "dir/".

ZipFile.getEntry() works well with these type of "canonicalized" 
directory

names. However, its implementation also tries to be "liberal" when
taking name that doesn't end with "/". Where there is no such entry 
(file)
found, it tries a second lookup with a "/" appended, and returns such 
entry
if find one. The original implementation always to use the original 
passed

in name (the one without the ending slash) as the entry name for the
returned ZipEntry. This is fine if the entry is from the first round 
of lookup,

in which the name stored in zip file matches the passed in name and to
use the name directly obviously helps avoiding creating another String
object from the binary name (byte[] in zip entry). But in case of the 
entry
is from the second round lookup, the direct consequence is the 
isDirectory()

method of the returned entry always returns false (ZipEntry.isDirectory()
implementation simply checks if the name ends with / or not), even it is
actually a directory entry.

The fix is to simply return the name stored in the zip file. It 
appears this

should be the desired and expected result.

Thanks,
Sherman

JDK 9 RFR of problem listing of several http2 tests

[Joseph D. Darcy]

Hello,

Several of the http2 tests have been observed to fail intermittently 
with relatively high frequency on various of our tests systems. I'd like 
to problem lists these tests while the underlying issues are being 
worked on. Patch below.


Thanks,

-Joe

diff -r 5cfbcb4e6009 test/ProblemList.txt
--- a/test/ProblemList.txtTue Jun 28 16:07:23 2016 -0300
+++ b/test/ProblemList.txtTue Jun 28 14:55:13 2016 -0700
@@ -164,6 +164,10 @@

 java/net/DatagramSocket/SendDatagramToBadAddress.java 7143960 macosx-all

+java/net/httpclient/http2/BasicTest.java 8157408 linux-all
+java/net/httpclient/http2/ErrorTest.java 8158127 solaris-all
+java/net/httpclient/http2/TLSConnection.java 8157482 macosx-all
+
 

 # jdk_nio

Re: JDK 9 RFR of problem listing of several http2 tests

[Joseph D. Darcy]

PS The line for ErrorTest is better as

+java/net/httpclient/http2/ErrorTest.java 8158127 solaris-all,windows-all

Cheers,

-Joe

On 6/28/2016 2:57 PM, Joseph D. Darcy wrote:

Hello,

Several of the http2 tests have been observed to fail intermittently 
with relatively high frequency on various of our tests systems. I'd 
like to problem lists these tests while the underlying issues are 
being worked on. Patch below.


Thanks,

-Joe

diff -r 5cfbcb4e6009 test/ProblemList.txt
--- a/test/ProblemList.txtTue Jun 28 16:07:23 2016 -0300
+++ b/test/ProblemList.txtTue Jun 28 14:55:13 2016 -0700
@@ -164,6 +164,10 @@

 java/net/DatagramSocket/SendDatagramToBadAddress.java 7143960 macosx-all

+java/net/httpclient/http2/BasicTest.java 8157408 linux-all
+java/net/httpclient/http2/ErrorTest.java 8158127 solaris-all
+java/net/httpclient/http2/TLSConnection.java 8157482 macosx-all
+
  



 # jdk_nio

Re: JDK 9 RFR of problem listing of several http2 tests

[Lance Andersen]

looks fine joe
> On Jun 28, 2016, at 5:59 PM, Joseph D. Darcy  wrote:
> 
> PS The line for ErrorTest is better as
> 
> +java/net/httpclient/http2/ErrorTest.java 8158127 solaris-all,windows-all
> 
> Cheers,
> 
> -Joe
> 
> On 6/28/2016 2:57 PM, Joseph D. Darcy wrote:
>> Hello,
>> 
>> Several of the http2 tests have been observed to fail intermittently with 
>> relatively high frequency on various of our tests systems. I'd like to 
>> problem lists these tests while the underlying issues are being worked on. 
>> Patch below.
>> 
>> Thanks,
>> 
>> -Joe
>> 
>> diff -r 5cfbcb4e6009 test/ProblemList.txt
>> --- a/test/ProblemList.txtTue Jun 28 16:07:23 2016 -0300
>> +++ b/test/ProblemList.txtTue Jun 28 14:55:13 2016 -0700
>> @@ -164,6 +164,10 @@
>> 
>> java/net/DatagramSocket/SendDatagramToBadAddress.java 7143960 macosx-all
>> 
>> +java/net/httpclient/http2/BasicTest.java 8157408 linux-all
>> +java/net/httpclient/http2/ErrorTest.java 8158127 solaris-all
>> +java/net/httpclient/http2/TLSConnection.java 8157482 macosx-all
>> +
>>  
>> 
>> # jdk_nio
>> 
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

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

[Kim Barrett]

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/
http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.01.inc/

Still investigating the initialization order for core exceptions.

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

[Kim Barrett]

> On Jun 27, 2016, at 9:34 PM, Daniel D. Daugherty 
>  wrote:
> Other than the possible missing notify_all() in vmGCOperations.cpp,
> this all looks great.

Thanks Dan.

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

[David Holmes]

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 :)



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.


Thanks,
David

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

[Mandy Chung]

> On Jun 28, 2016, at 4: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 :)


A related post in compiler-dev w.r.t. Reference:
  http://mail.openjdk.java.net/pipermail/compiler-dev/2014-January/008457.html

Looks like no reply on that thread though.

> 
>> 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)
> 

How do you get this list?

> 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.
> 

Do you mind trying b110 before JEP 261 was integrated in jdk-9+111 and see any 
difference?

initPhase1 is expected to be called very early in the VM initialization after 
the primordial classes are loaded.

IIRC, the exception classes are preallocated to improve diagnosability in the 
case when we run out of memory VM can still throw a preallocated instance with 
an preallocated stack trace buffer.  I don’t think these exception classes are 
expected to be loaded initPhase1.

More to dig here.

Mandy

> 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.
> 
> Thanks,
> David
> 

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

[David Holmes]

Hi Mandy,

On 29/06/2016 12:28 PM, Mandy Chung wrote:



On Jun 28, 2016, at 4: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 :)



A related post in compiler-dev w.r.t. Reference:
  http://mail.openjdk.java.net/pipermail/compiler-dev/2014-January/008457.html

Looks like no reply on that thread though.




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)



How do you get this list?


For current builds: java -Xlog:class+init -version (and I trimmed the 
output)


For slightly older builds: java -Xlog:classinit -version

For pre UL you need a fastdebug build and: java 
-XX:+TraceClassInitialization -version





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.



Do you mind trying b110 before JEP 261 was integrated in jdk-9+111 and see any 
difference?


Here's fragment from b109:

31 Initializing 'java/lang/ref/Reference' (0x00085a08)
32 Initializing 'java/lang/ref/Reference$Lock'(no method) 
(0x0008000181c0)
33 Initializing 'java/lang/ref/Reference$ReferenceHandler' 
(0x0008000183b8)

34 Initializing 'java/lang/Throwable' (0x000828f0)


initPhase1 is expected to be called very early in the VM initialization after 
the primordial classes are loaded.

IIRC, the exception classes are preallocated to improve diagnosability in the 
case when we run out of memory VM can still throw a preallocated instance with 
an preallocated stack trace buffer.  I don’t think these exception classes are 
expected to be loaded initPhase1.


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.


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.


** 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().


Thanks,
David


More to dig here.

Mandy


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.

Thanks,
David