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

[Derek White]

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