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.samers...@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. >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.