RFR(M,v7): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo (original) (raw)

Peter Levart peter.levart at gmail.com
Tue May 19 09:52:38 UTC 2015

Hi Dmitry,

This looks good. Just alignment of new code in Finalizer seems a little wobbly.

Regards, Peter

On 05/18/2015 02:17 PM, Dmitry Samersoff wrote:

Everyone,

Please review updated version of the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.07/ Most important part of the fix provided by Peter Levart, so all credentials belongs to him. -Dmitry On 2015-05-16 15:48, Peter Levart wrote:

On 05/16/2015 02:38 PM, Peter Levart wrote:

On 05/16/2015 09:35 AM, Dmitry Samersoff wrote: Derek,

Personally, I'm for volatile over explicit fence too. So I'll change the code if Peter don't have objections. There are no objections. There's one unneeded volatile store to .next field then in enqueue(), but it is followed by another volatile store, so there shouldn't be overhead on Intel and SPARC at least. Regards, Peter If you make .next field volatile, then perhaps you could also cache it's value in reallyPoll() into a local variable, so that it is not read twice. Like this: private Reference<? extends T> reallyPoll() { /* Must hold lock */ Reference<? extends T> r = head; if (r != null) { Reference rn = r.next; // HERE !!! head = (rn == r) ? null : rn; // Unchecked due to the next field having a raw type in Reference r.queue = NULL; r.next = r; queueLength--; if (r instanceof FinalReference) { sun.misc.VM.addFinalRefCount(-1); } return r; } return null; } Peter -Dmitry On 2015-05-16 01:59, Derek White wrote: Hi Dmitry, Peter,

I should read my email more frequently - I missed Dmitry's last webrev email. My comments on a preference for volatile over Unsafe are not a strong objection to using Unsafe fences. I just don't have enough experience with Unsafe fences yet to agree that this use is correct. While looking over this Reference code I found 3-6 really simple things that need cleaning up that have been there for years, so I'm not a big cheerleader for more complicated things here :-) - Derek On 5/15/15 6:46 PM, Derek White wrote: Hi Peter,

Some comments below... On 5/14/15 3:50 AM, Peter Levart wrote: Hi Derek,

On 05/13/2015 10:34 PM, Derek White wrote: Hi Peter,

I don't have smoking gun evidence that your change introduces a bug, but I have some concerns... I did have a concern too, ... On 5/12/15 6:05 PM, Peter Levart wrote: Hi Dmitry,

You iterate the queue then, not the unfinalized list. That's more logical. Holding the queue's lock may pause reference handler and finalizer threads for the entire time of iteration. This can blow up the application. Suppose one wants to diagnose the application because he suspects that finalizer thread hardly keeps up with production of finalizable instances. This can happen if the allocation rate of finalizable objects is very high and/or finalize() methods are not coded to be fast enough. Suppose the queue of Finalizer(s) builds up gradually and is already holding several million objects. Now you initiate the diagnostic command to print the queue. The iteration over and grouping/counting of several millions of Finalizer(s) takes some time. This blocks finalizer thread and reference handler thread. But does not block the threads that allocate finalizable objects. By the time the iteration is over, the JVM blows up and application slows down to a halt doing GCs most of the time, getting OOMEs, etc... It is possible to iterate the elements of the queue for diagnostic purposes without holding a lock. The modification required to do that is the following (havent tested this, but I think it should work):

http://cr.openjdk.java.net/~plevart/misc/Finalizer.printFinalizationQueue/webrev.01/ One issue to watch out for is the garbage collectors inspect the Reference.next field from C code directly (to distinguish active vs. pending, iterate over oops, etc.). You can search hotspot for javalangrefReference::next*, javalangrefReference::setnext*. Your change makes "inactive" References superficially look like "enqueued" References. The GC code is rather subtle and I haven't yet seen a case where it would get confused by this change, but there could easily be something like that lurking in the GC code. ...but then I thought that GC can in no way treat a Reference differently whether it is still enqueued in a ReferenceQueue or already dequeued (inactive) - responsibility is already on the Java side. Currently the definition of Reference.next is this: /* When active: NULL * pending: this * Enqueued: next reference in queue (or this if last) * Inactive: this */ @SuppressWarnings("rawtypes") Reference next; We see that, unless GC inspects all ReferenceQueue instances and scans their lists too, the state of a Reference that is enqueued as last in list is indistinguishable from the state of inactive Reference. So I deduced that this distinction (enqueued/inactive) can't be established solely on the value of .next field ( == this or != this)... But I should inspect the GC code too to build a better understanding of that part of the story... Anyway. It turns out that there is already enough state in Reference to destinguish between it being enqueued as last in list and already dequeued (inactive) - the additional state is Reference.queue that transitions from ReferenceQueue.ENQUEUED -> ReferenceQueue.NULL in ReferenceQueue.reallyPoll. We need to just make sure the two fields (r.next and r.queue) are assigned and read in correct order. This can be achieved either by making Reference.next a volatile field or by a couple of explicit fences: http://cr.openjdk.java.net/~plevart/misc/Finalizer.printFinalizationQueue/webrev.02/ The assignment of r.queue to ReferenceQueue.ENQUEUED already happens before linking the reference into the queue's head in ReferenceQueue.enqueue(): r.queue = ENQUEUED; r.next = (head == null) ? r : head; head = r; Both stores are volatile. This sounds a bit better. I don't have a good handle on the pros and cons of a volatile field over explicit fences via Unsafe in this case. Kim might jump in soon. I'd like to suggest an entirely less-clever solution. Since the problem is that forEach() might see inconsistent values for the queue and next fields of a Reference, but we don't want to grab a lock walking the whole queue, we could instead grab the lock as we look at each Reference (and do the "back-up" trick if we were interfered with). Of course this is slower than going lock-free, but it's not any more overhead than the ReferenceHandler thread or FinalizerThreads incur. The only benefit of this solution over the others is that it is less clever, and I'm not sure how much cleverness this problem deserves. Given that, and ranking the solutions as "lock" < (clever) "volatile"_ _< "fence", I'd be happier with the "volatile" or "lock" solution if_ _that is sufficient._ _- Derek_ _I also suggest the addition to the ReferenceQueue to be contained_ _(package-private) and as generic as possible. That's why I suggest_ _forEach iteration method with no concrete logic._ _It would be possible to encapsulate the entire logic into a special_ _package-private class (say java.lang.ref.DiagnosticCommands) with_ _static method(s) (printFinalizationQueue)... You could just expose_ _a package-private forEach static method from Finalizer and code the_ _rest in DiagnosticCommands._ _That's good for encapsulation. But I'm concerned that if "forEach"_ _got exposed beyond careful use in DiagnosticCommands, and the_ _Referents were leaked back into the heap, then we could get_ _unexpected object resurrection after finalization. This isn't a bug_ _on it's own - any finalizer might resurrect its object if not_ _careful, but ordinarily /only/ the finalizer could resurrect the_ _object. This could invalidate application invariants?_ _Well, it all stays in the confines of package-private API - internal_ _to JDK. Any future additional use should be reviewed carefully._ _Comments on the forEach() method should warn about that._ _I agree that using a lock over the ReferenceQueue iteration opens up_ _/another/ case of diagnostics affecting application behavior. And_ _there are pathological scenarios where this gets severe. This is_ _unfortunate but not uncommon. There is enough complication here that_ _you should be sure that the fix for diagnostics performance doesn't_ _introduce subtle bugs to the system in general. A change in this_ _area should get a thorough review from both the runtime and GC sides._ _Is the webrev.02 proposed above more acceptable in that respect? It_ _does not introduce any logical changes to existing code._ _Better yet, the reference handling code in GC and runtime should_ _probably be thrown out and re-written, but that's another issue :-)_ _I may have some proposals in that direction. Stay tuned._ _Regards, Peter_ _- Derek_ _Regards, Peter_ _On 05/12/2015 07:10 PM, Dmitry Samersoff wrote:_ _Everybody,_ _Updated version:_ _http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.03/_ _Now it iterates over queue and output result sorted by number of instances._ _-Dmitry_ _On 2015-05-07 00:51, Derek White wrote:_ _Hi Dmitry, Staffan,_ _Lots of good comments here._ _On the topic of what list should be printed out, I think we should focus_ _on objects waiting to be finalized - e.g. the contents of the_ _ReferenceQueue. It's more of a pain to walk the ReferenceQueue, but you_ _could add a summerizeQueue(TreeMap<String, Integer>) method, or a general iterator and lambda. A histogram of objects with finalize methods might also be interesting, but you can get most of the same information from a heap histogram (ClassHistogramDCmd, and search for count of Finalizer instances). BTW, by only locking the ReferenceQueue, we should only be blocking the FinalizerThread from making progress (and I think there's a GC thread that runs after GC that sorts found References objects that need processing into their respective ReferenceQueues). But locking the "unfinalized" list blocks initializing any object with a finalize() method. The sorting suggestion is a nice touch. - Derek White, GC team On 5/5/15 10:40 AM, Peter Levart wrote: Hi Dmitry, Staffan, On 05/05/2015 12:38 PM, Staffan Larsen wrote: Dmitry, I think this should be reviewed on hotspot-gc and core-libs-dev as well considering the changes to Finalizer. I’m a little worried about the potentially very large String that is returned from printFinalizationQueue(). A possible different approach would be to write printFinalizationQueue() in C++ inside Hotspot. That would allow for outputting one line at a time. The output would still be saved in memory (since the stream is buffered), but at least the data is only saved once in memory, then. It would make the code a bit harder to write, so its a question of how scared we are of running out of memory. If the output is just a histogram of # of instances per class name, then it should not be too large, as there are not many different classes overriding finalize() method (I counted 72 overriddings of finalize() method in the whole jdk/src tree). I'm more concerned about the fact that while traversing the list, a lock is held, which might impact prompt finalization(). Is it acceptable for diagnostic output to impact performance and/or interfere with synchronization? It might be possible to scan the list without holding a lock for diagnostic purposes if Finalizer.remove() didn't set the removed Finalizer.next pointer to point back to itself: private void remove() { synchronized (lock) { if (unfinalized == this) { if (this.next != null) { unfinalized = this.next; } else { unfinalized = this.prev; } } if (this.next != null) { this.next.prev = this.prev; } if (this.prev != null) { this.prev.next = this.next; } // this.next = this; must not be set so that we can traverse the list unsynchronized this.prev = this; /* Indicates that this has been finalized */ } } For detecting whether a Finalizer is already processed, the 'prev' pointer could be used instead: private boolean hasBeenFinalized() { return (prev == this); } Also, to make sure a data race dereferencing 'unfinalized' in unsynchronized printFinalizationQueue() would get you a fully initialized Finalizer instance (in particular the next pointer), you would have to make the 'unfinalized' field volatile or insert an Unsafe.storeFence() before assigning to 'unfinalized': private void add() { synchronized (lock) { if (unfinalized != null) { this.next = unfinalized; unfinalized.prev = this; } // make sure a data race dereferencing 'unfinalized' // in printFinalizationQueue() does see the Finalizer // instance fully initialized // (in particular the 'next' pointer) U.storeFence(); unfinalized = this; } }

By doing these modifications, I think you can remove synchronized(lock) {} from printFinalizationQueue(). I see you are traversing the ‘unfinalized’ list in Finalizer, whereas the old SA code for ‘-finalizerinfo' traverses the ‘queue’ list. I am not sure of the difference, perhaps someone from the GC team can help out? The 'queue' is a ReferenceQueue of Finalizer (FinalReference) instances pending processing by finalizer thread because their referents are eligible for finalization (they are not reachable any more). The 'unfinalized' is a doubly-linked list of all Finalizer instances for which their referents have not been finalized yet (including those that are still reachable and alive). The later serves two purposes: - it keeps Finalizer instances reachable until they are processed - it is a source of unfinalized instances for running-finalizers-on-exit if requested by System.runFinalizersOnExit(true); So it really depends on what one would like to see. Showing the queue may be interesting if one wants to see how the finalizer thread is coping with processing the finalize() invocations. Showing unfinalized list may be interesting if one wants to know how many live + finalization pending instances are there on the heap that override finalize() method. Regards, Peter For the output, it would be a nice touch to sort it on the number of references for each type. Perhaps outputting it more like a table (see the old code again) would also make it easier to digest. In diagnosticCommand.hpp, the new commands should override permission() and limit access: static const JavaPermission permission() { JavaPermission p = {"java.lang.management.ManagementPermission", "monitor", NULL}; return p; } The two tests don’t validate the output in any way. Would it be possible to add validation? Perhaps hard to make sure an object is on the finalizer queue… Hmm. Thanks, /Staffan

On 5 maj 2015, at 12:01, Dmitry Samersoff <dmitry.samersoff at oracle.com> wrote: Everyone, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.01/ heap dcmd outputs the same information as SIGBREAK finalizerinfo dcmd outputs a list of all classes in finalization queue with count -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.

More information about the core-libs-dev mailing list