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

[Mandy Chung]

> On Jun 3, 2015, at 12:22 PM, Peter Levart <peter.levart at gmail.com> wrote:
> 
> 
> 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/

This looks good.

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

Yup.

Mandy

> 
> 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 
>