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

[Daniel D. Daugherty]

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

make/mapfiles/libjava/mapfile-vers
     No comments.

src/java.base/share/classes/java/lang/ref/Reference.java
     Much cleaner (pun might have been intended :-)).

src/java.base/share/native/include/jvm.h
     No comments.

src/java.base/share/native/libjava/Reference.c
     No comments.

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

make/symbols/symbols-unix
     No comments.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Threads.java
     No comments.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/soql/sa.js
     No comments.

src/share/vm/ci/ciReplay.cpp
     No comments.

src/share/vm/classfile/javaClasses.cpp
     No comments.

src/share/vm/classfile/javaClasses.hpp
     No comments.

src/share/vm/compiler/compileBroker.cpp
     No comments.

src/share/vm/gc/cms/concurrentMarkSweepThread.cpp
     So no more Surrogate Locker Thread (SLT)? Sound good to me.

src/share/vm/gc/cms/vmCMSOperations.cpp
     No comments.

src/share/vm/gc/cms/vmCMSOperations.hpp
     No comments.

src/share/vm/gc/g1/concurrentMarkThread.cpp
     No comments.

src/share/vm/gc/g1/g1CollectedHeap.hpp
     No comments.

src/share/vm/gc/g1/vm_operations_g1.cpp
     No comments.

src/share/vm/gc/g1/vm_operations_g1.hpp
     No comments.

src/share/vm/gc/shared/collectedHeap.hpp
     No comments.

src/share/vm/gc/shared/genCollectedHeap.hpp
     No comments.

src/share/vm/gc/shared/referenceProcessor.cpp
     No comments.

src/share/vm/gc/shared/referenceProcessor.hpp
     No comments.

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();
           }

src/share/vm/gc/shared/vmGCOperations.hpp
     No comments.

src/share/vm/memory/universe.cpp
     No comments.

src/share/vm/memory/universe.hpp
     No comments.

src/share/vm/oops/method.cpp
     No comments.

src/share/vm/prims/jvm.cpp
     No comments.

src/share/vm/prims/jvm.h
     No comments.

src/share/vm/runtime/thread.cpp
     So this change moves call_initPhase1() after the initialize_class()
     calls and that just sets off alarm bells in my head. Yes, you have
     a really big comment below explaining this. Still worries me!

src/share/vm/runtime/vmStructs.cpp
     No comments.

src/share/vm/gc/shared/referencePendingListLocker.cpp
src/share/vm/gc/shared/referencePendingListLocker.hpp
     Both files are deleted and don't need review.


Other than the possible missing notify_all() in vmGCOperations.cpp,
this all looks great.

Dan


On 6/24/16 2:09 PM, 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
>
> 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.
>