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

Staffan Larsen staffan.larsen at oracle.com
Fri Jun 5 12:24:33 UTC 2015


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.



More information about the hotspot-gc-dev mailing list