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

Dmitry Samersoff dmitry.samersoff at oracle.com
Thu May 14 17:41:53 UTC 2015


Could we just bail out on r == r.next?

It gives us less accurate result, but I suspect that If we restart from
head we need to flush all counters.

As far I understand queue poller removes items one by one from a queue
end so if we overtaken by queue poller we can safely assume that
we are at the end of the queue.

Is it correct?


On 2015-05-14 10:50, Peter Levart wrote:
> Hi Derek,
> On 05/13/2015 10:34 PM, Derek White wrote:
>> Hi Peter,
>> I don't have smoking gun evidence that your change introduces a bug,
>> but I have some concerns...
> I did have a concern too, ...
>> On 5/12/15 6:05 PM, Peter Levart wrote:
>>> 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/
>> One issue to watch out for is the garbage collectors inspect the
>> Reference.next field from C code directly (to distinguish active vs.
>> pending, iterate over oops, etc.). You can search hotspot for
>> java_lang_ref_Reference::next*, java_lang_ref_Reference::set_next*.
>> Your change makes "inactive" References superficially look like
>> "enqueued" References. The GC code is rather subtle and I haven't yet
>> seen a case where it would get confused by this change, but there
>> could easily be something like that lurking in the GC code.
> ...but then I thought that GC can in no way treat a Reference
> differently whether it is still enqueued in a ReferenceQueue or already
> dequeued (inactive) - responsibility is already on the Java side.
> Currently the definition of Reference.next is this:
>     /* When active:   NULL
>      *     pending:   this
>      *    Enqueued:   next reference in queue (or this if last)
>      *    Inactive:   this
>      */
>     @SuppressWarnings("rawtypes")
>     Reference next;
> We see that, unless GC inspects all ReferenceQueue instances and scans
> their lists too, the state of a Reference that is enqueued as last in
> list is indistinguishable from the state of inactive Reference. So I
> deduced that this distinction (enqueued/inactive) can't be established
> solely on the value of .next field ( == this or != this)...
> But I should inspect the GC code too to build a better understanding of
> that part of the story...
> Anyway. It turns out that there is already enough state in Reference to
> destinguish between it being enqueued as last in list and already
> dequeued (inactive) - the additional state is Reference.queue that
> transitions from ReferenceQueue.ENQUEUED -> ReferenceQueue.NULL in
> ReferenceQueue.reallyPoll. We need to just make sure the two fields
> (r.next and r.queue) are assigned and read in correct order. This can be
> achieved either by making Reference.next a volatile field or by a couple
> of explicit fences:
> http://cr.openjdk.java.net/~plevart/misc/Finalizer.printFinalizationQueue/webrev.02/
> The assignment of r.queue to ReferenceQueue.ENQUEUED already happens
> before linking the reference into the queue's head in
> ReferenceQueue.enqueue():
>             r.queue = ENQUEUED;
>             r.next = (head == null) ? r : head;
>             head = r;
> Both stores are volatile.
>>> 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.
>> That's good for encapsulation. But I'm concerned that if "forEach" got
>> exposed beyond careful use in DiagnosticCommands, and the Referents
>> were leaked back into the heap, then we could get unexpected object
>> resurrection after finalization. This isn't a bug on it's own - any
>> finalizer might resurrect its object if not careful, but ordinarily
>> /only/ the finalizer could resurrect the object. This could invalidate
>> application invariants?
> Well, it all stays in the confines of package-private API - internal to
> JDK. Any future additional use should be reviewed carefully. Comments on
> the forEach() method should warn about that.
>> I agree that using a lock over the ReferenceQueue iteration opens up
>> /another/ case of diagnostics affecting application behavior. And
>> there are pathological scenarios where this gets severe. This is
>> unfortunate but not uncommon. There is enough complication here that
>> you should be sure that the fix for diagnostics performance doesn't
>> introduce subtle bugs to the system in general. A change in this area
>> should get a thorough review from both the runtime and GC sides.
> Is the webrev.02 proposed above more acceptable in that respect? It does
> not introduce any logical changes to existing code.
>> Better yet, the reference handling code in GC and runtime should
>> probably be thrown out and re-written, but that's another issue :-)
> I may have some proposals in that direction. Stay tuned.
> Regards, Peter
>> - Derek
>>> 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.

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 core-libs-dev mailing list