RFR(M,v3): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo
Derek White
derek.white at oracle.com
Wed May 13 21:52:33 UTC 2015
Hi Dmitry,
Some review comments below...
On 5/12/15 1: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.
FinalReference.java
- Update copyright to 2015.
ReferenceQueue.java
- Update copyright to 2015.
class "mutableInt":
- Should be "MutableInt"
- Not a collections lawyer, but should MutableInt implement Comparable?
countInstances():
- Javadoc should mention may return null.
- Can countInstances() be package-private?
- After you get the lock in line 138, you should recheck for "head ==
null", and return null if so. Otherwise it might sometimes return null
and sometimes return an empty map.
- BIG: Is loop missing? Should "if" at line 140 be "while"?
- Merge lines 147, 148: "} else {"
- Check for consistent line spacing.
- Derek
>
> -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 hotspot-gc-dev
mailing list