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 <david.holmes at oracle.com> 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<? super Object> 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<? extends 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++...