I also have some comments on Dmitry's code...
On 05/13/2015 11:52 PM, Derek White wrote:
Hi Dmitry,
Some review comments below...
On 5/12/15 1: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.
FinalReference.java
- Update copyright to 2015.
ReferenceQueue.java
- Update copyright to 2015.
class "mutableInt":
- Should be "MutableInt"
- Not a collections lawyer, but should MutableInt implement Comparable?
An alternative is a one-element int[] and the need for additional
MutableInt class is eliminated. If you must have it, then perhaps you
could make it Comparable and contain an additional String className
field. Why? You could then dump the values() of the Map into an array
and sort the array without the need for custom Comparator and then
iterate the sorted array to build a message.
countInstances():
- Javadoc should mention may return null.
- Can countInstances() be package-private?
It must be package-private, since ReferenceQueue is public API.
Regards, Peter
- After you get the lock in line 138, you should recheck for "head ==
null", and return null if so. Otherwise it might sometimes return null
and sometimes return an empty map.
- BIG: Is loop missing? Should "if" at line 140 be "while"?
- Merge lines 147, 148: "} else {"
- Check for consistent line spacing.
- Derek
-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.
