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.samers...@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.
