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

[Dmitry Samersoff]

Mandy,

Webrev updated in-place (press shift-reload).

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14

getFinalizerHistogram() now returns Entry[]

@library and @build removed from the test.

-Dmitry

On 2015-06-04 16:56, Mandy Chung wrote:
> 
>> 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.
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.