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

[Dmitry Samersoff]

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.
>>
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.