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

[Mandy Chung]

> On Jun 4, 2015, at 6:38 AM, Dmitry Samersoff <dmitry.samersoff at oracle.com> wrote:
> 
> Mandy,
> 
> Thank you for the review.
> 
> Updated webrev is:
> 
> http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14
> 

Thanks for the update and the test.

> addressed comments, added a unit test to JDK workspace.
> 

This test does not need @library and @build, right?  

> 
> On 2015-06-03 21:36, Mandy Chung wrote:
> 
>> Should getFinalizerHistogram() return FinalizerHistogramEntry[]?
>> The VM knows the returned type anyway.
> 
> "[java/lang/ref/Entry" signature would need a separate symbol entry in
> swollen vmSymbols::init() so I would prefer to stay with more generic
> Object[]
> 

You already add several symbols - why is it an issue for another one?  Or if adding vm symbols is a concern, you should revert to String and int[] that you decide not to.   The decision should apply consistently (use String and int, or new Java type).


>> Perhaps the getFinalizerHistogram method should be renamed
>> e.g. "dump"?
> 
> The line in vmSymbols looks like:
> 
> template(
> get_finalizer_histogram_name, "getFinalizerHistogram")
> 
> I would prefer to keep method name specific enough to be able to
> find it by grep in jdk code.


Grep in jdk code is easy as you have a new class :)  

Mandy

> 
> (other comments are addressed)
> 
> -Dmitry
> 
> 
> On 2015-06-03 21:36, 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>
> 
> done.
> 
>> 
>> 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;
> 
> done.
> 
>> 
>> FinalizerHistogram.java
>>  Copyright year needs update.
> 
> done.
>> 
>> 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.
> 
> done.
> 
>> 
>> Should getFinalizerHistogram() return FinalizerHistogramEntry[]? The VM
>> knows the returned type anyway.  
> 
> "[java/lang/ref/Entry" signature would need a separate symbol entry in
> swollen vmSymbols::init() so I would prefer to stay with more generic
> Object[]
> 
>> 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++".
> 
> done.
> 
>> 
>> 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.
> 
> done. The test repeats hotspot one, but uses reflection instead of VM call.
> 
>> 
>> A minor naming comment - now you have a FinalizerHistogram class.
>> Perhaps the getFinalizerHistogram method should be renamed e.g. "dump"?
> 
> The line in vmSymbols looks like:
> 
> template(
> get_finalizer_histogram_name, "getFinalizerHistogram")
> 
> I would prefer to keep it specific enough to be able to
> find it by grep in jdk code.
> 
> 
> -- 
> Dmitry Samersoff
> Oracle Java development team, Saint Petersburg, Russia
> * I would love to change the world, but they won't give me the sources.