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

Dmitry Samersoff dmitry.samersoff at oracle.com
Mon Jun 8 11:05:27 UTC 2015


Staffan,

Could you review a CCC request.

http://ccc.us.oracle.com/8059036

-Dmitry

On 2015-06-05 15:24, Staffan Larsen wrote:
> Thanks - this version looks good to me.
> 
> /Staffan
> 
>> On 5 jun 2015, at 13:00, Dmitry Samersoff <dmitry.samersoff at oracle.com> wrote:
>>
>> Staffan,
>>
>> Thank you for review!
>>
>> Done. Webrev updated in-place (press shift-reload).
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14
>>
>> -Dmitry
>>
>> On 2015-06-05 11:20, Staffan Larsen wrote:
>>> Dmitry,
>>>
>>> I’d like to propose the following change to get prettier output (more in line with GC.class_histogram):
>>>
>>> diff --git a/src/share/vm/services/diagnosticCommand.cpp b/src/share/vm/services/diagnosticCommand.cpp
>>> --- a/src/share/vm/services/diagnosticCommand.cpp
>>> +++ b/src/share/vm/services/diagnosticCommand.cpp
>>> @@ -341,7 +341,6 @@
>>> void FinalizerInfoDCmd::execute(DCmdSource source, TRAPS) {
>>>   ResourceMark rm;
>>>
>>> -  output()->print_cr("Unreachable instances awaiting finalization:");
>>>   Klass* k = SystemDictionary::resolve_or_null(
>>>     vmSymbols::finalizer_histogram_klass(), THREAD);
>>>   assert(k != NULL, "FinalizerHistogram class is not accessible");
>>> @@ -375,12 +374,15 @@
>>>
>>>   assert(count_res != NULL && name_res != NULL, "Unexpected layout of FinalizerHistogramEntry");
>>>
>>> +  output()->print_cr("Unreachable instances waiting for finalization");
>>> +  output()->print_cr("#instances  class name");
>>> +  output()->print_cr("-----------------------");
>>>   for (int i = 0; i < result_oop->length(); ++i) {
>>>     oop element_oop = result_oop->obj_at(i);
>>>     oop str_oop = element_oop->obj_field(name_fd.offset());
>>>     char *name = java_lang_String::as_utf8_string(str_oop);
>>>     int count = element_oop->int_field(count_fd.offset());
>>> -    output()->print_cr("Class %s - %d", name, count);
>>> +    output()->print_cr("%10d  %s", count, name);
>>>   }
>>> }
>>>
>>>
>>> A couple of nits:
>>>
>>> diagnosticCommand.cpp:359 - extra space after =
>>> diagnosticCommand.cpp:361 - spelling: finlalization -> finalization
>>> FinalizerInfoTest.java:69: - spelling: intialized -> initialized
>>> FinalizerInfoTest.java:71 - I’d like to see the ; on a new line
>>>
>>>
>>> Thanks,
>>> /Staffan
>>>
>>>
>>>> On 5 jun 2015, at 00:20, Mandy Chung <mandy.chung at oracle.com> wrote:
>>>>
>>>>
>>>>> On Jun 4, 2015, at 8:49 AM, Dmitry Samersoff <dmitry.samersoff at oracle.com> wrote:
>>>>>
>>>>> 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.
>>>>
>>>>
>>>> Looks fine.
>>>>
>>>> Thanks
>>>> Mandy
>>>>
>>>>>
>>>>> -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.
>>>>
>>>
>>
>>
>> -- 
>> 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.


More information about the hotspot-gc-dev mailing list