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

[Peter Levart]

On 06/03/2015 08:36 PM, Mandy Chung wrote:
>
>
> On 06/03/2015 08:29 AM, Dmitry Samersoff wrote:
>> Updated webrev:
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13
>
> I reviewed the jdk change.  The version looks okay and some comments:
>
> ReferenceQueue.java - you should eliminate the use of rawtype:
>
> 84             Reference rn = r.next;
>
> Change Reference to Reference<? extends T>
>
> The forEach method - eliminate the use of raw type and
> move @SuppressWarning to the field variable in line 182:
>
>       @SuppressWarnings("unchecked")
>       Reference<? extends T> rn = r.next;

Thanks, Mandy. This was my doing. The @SuppressWarnings can be moved to 
the local var declaration in line 84 too. Here's how the fixed 
ReferenceQueue looks like after those two changes:

http://cr.openjdk.java.net/~plevart/misc/Finalizer.printFinalizationQueue/webrev.03/

>
> FinalizerHistogram.java
>   Copyright year needs update.
>
> I personally think the VM code would be much simpler if you stay with 
> alternative entry of String and int than dealing with a 
> FinalizerHistogramEntry private class.  It's okay as 
> FinalizerHistogram class is separated.
>
> The comment in line 35 is suitable to move to the class description 
> and this is the suggestion.
>
>     // This FinalizerHistogram class is for GC.finalizer_info 
> diagnostic command support.
>     // It is invoked by the VM.
>
> Should getFinalizerHistogram() return FinalizerHistogramEntry[]? The 
> VM knows the returned type anyway.   It's an inner class of 
> FinalizerHistogram and it can simply be named as "Entry".   I suggest 
> to have Entry::increment method to replace ".instanceCount++".

This FinalizerHistogram.Entry naming part came to my mind too, yes. If 
there is a method for incrementing, then both fields can be marked private.

Regards, Peter

>
> The tests are in hotspot/test.    Can you add a unit test in 
> jdk/test?  Perhaps you can test 
> FinalizerHistogram.getFinalizerHistogram() via reflection - just a 
> sanity test.
>
> A minor naming comment - now you have a FinalizerHistogram class. 
> Perhaps the getFinalizerHistogram method should be renamed e.g. "dump"?
>
> Mandy

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20150603/8fe46ea1/attachment.html>