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

[Peter Levart]

Hi Dmitry,

You iterate the queue then, not the unfinalized list. That's more logical.

Holding the queue's lock may pause reference handler and finalizer 
threads for the entire time of iteration. This can blow up the 
application. Suppose one wants to diagnose the application because he 
suspects that finalizer thread hardly keeps up with production of 
finalizable instances. This can happen if the allocation rate of 
finalizable objects is very high and/or finalize() methods are not coded 
to be fast enough. Suppose the queue of Finalizer(s) builds up gradually 
and is already holding several million objects. Now you initiate the 
diagnostic command to print the queue. The iteration over and 
grouping/counting of several millions of Finalizer(s) takes some time. 
This blocks finalizer thread and reference handler thread. But does not 
block the threads that allocate finalizable objects. By the time the 
iteration is over, the JVM blows up and application slows down to a halt 
doing GCs most of the time, getting OOMEs, etc...

It is possible to iterate the elements of the queue for diagnostic 
purposes without holding a lock. The modification required to do that is 
the following (havent tested this, but I think it should work):

http://cr.openjdk.java.net/~plevart/misc/Finalizer.printFinalizationQueue/webrev.01/

I also suggest the addition to the ReferenceQueue to be contained 
(package-private) and as generic as possible. That's why I suggest 
forEach iteration method with no concrete logic.

It would be possible to encapsulate the entire logic into a special 
package-private class (say java.lang.ref.DiagnosticCommands) with static 
method(s) (printFinalizationQueue)... You could just expose a 
package-private forEach static method from Finalizer and code the rest 
in DiagnosticCommands.

Regards, Peter


On 05/12/2015 07: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.
>
> -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.