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

Dmitry Samersoff dmitry.samersoff at oracle.com
Mon May 18 12:17:31 UTC 2015


Everyone,

Please review updated version of the fix:

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.07/

Most important part of the fix provided by Peter Levart, so all
credentials belongs to him.

-Dmitry

On 2015-05-16 15:48, Peter Levart wrote:
> 
> 
> On 05/16/2015 02:38 PM, Peter Levart wrote:
>>
>>
>> On 05/16/2015 09:35 AM, Dmitry Samersoff wrote:
>>> Derek,
>>>
>>> Personally, I'm for volatile over explicit fence too.
>>>
>>> So I'll change the code if Peter don't have objections.
>>
>> There are no objections. There's one unneeded volatile store to .next
>> field then in enqueue(), but it is followed by another volatile store,
>> so there shouldn't be overhead on Intel and SPARC at least.
>>
>> Regards, Peter
> 
> If you make .next field volatile, then perhaps you could also cache it's
> value in reallyPoll() into a local variable, so that it is not read
> twice. Like this:
> 
>     private Reference<? extends T> reallyPoll() {       /* Must hold lock */
>         Reference<? extends T> r = head;
>         if (r != null) {
>             Reference rn = r.next; // HERE !!!
>             head = (rn == r) ?
>                 null :
>                 rn; // Unchecked due to the next field having a raw type
> in Reference
>             r.queue = NULL;
>             r.next = r;
>             queueLength--;
>             if (r instanceof FinalReference) {
>                 sun.misc.VM.addFinalRefCount(-1);
>             }
>             return r;
>         }
>         return null;
>     }
> 
> 
> 
> Peter
> 
> 
>>
>>> -Dmitry
>>>
>>> On 2015-05-16 01:59, Derek White wrote:
>>>> Hi Dmitry, Peter,
>>>>
>>>> I should read my email more frequently - I missed Dmitry's last webrev
>>>> email.
>>>>
>>>> My comments on a preference for volatile over Unsafe are not a strong
>>>> objection to using Unsafe fences. I just don't have enough experience
>>>> with Unsafe fences yet to agree that this use is correct.
>>>>
>>>> While looking over this Reference code I found 3-6 really simple things
>>>> that need cleaning up that have been there for years, so I'm not a big
>>>> cheerleader for more complicated things here :-)
>>>>
>>>>  - Derek
>>>>
>>>> On 5/15/15 6:46 PM, Derek White wrote:
>>>>> Hi Peter,
>>>>>
>>>>> Some comments below...
>>>>>
>>>>> On 5/14/15 3:50 AM, 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.
>>>>> This sounds a bit better. I don't have a good handle on the pros and
>>>>> cons of a volatile field over explicit fences via Unsafe in this case.
>>>>> Kim might jump in soon.
>>>>>
>>>>> I'd like to suggest an entirely less-clever solution. Since the
>>>>> problem is that forEach() might see inconsistent values for the queue
>>>>> and next fields of a Reference, but we don't want to grab a lock
>>>>> walking the whole queue, we could instead grab the lock as we look at
>>>>> each Reference (and do the "back-up" trick if we were interfered
>>>>> with). Of course this is slower than going lock-free, but it's not any
>>>>> more overhead than the ReferenceHandler thread or FinalizerThreads incur.
>>>>>
>>>>> The only benefit of this solution over the others is that it is less
>>>>> clever, and I'm not sure how much cleverness this problem deserves.
>>>>> Given that, and ranking the solutions as "lock" < (clever) "volatile"
>>>>> < "fence", I'd be happier with the "volatile" or "lock" solution if
>>>>> that is sufficient.
>>>>>
>>>>>  - Derek
>>>>>>>> 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 hotspot-gc-dev mailing list