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

Dmitry Samersoff dmitry.samersoff at oracle.com
Sat May 16 07:35:39 UTC 2015


Derek,

Personally, I'm for volatile over explicit fence too.

So I'll change the code if Peter don't have objections.

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