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

[Peter Levart]

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.