> On Jun 28, 2016, at 12:04 AM, David Holmes <david.hol...@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++...
