RFR(M, v14): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo
Dmitry Samersoff
dmitry.samersoff at oracle.com
Fri Jun 5 11:00:27 UTC 2015
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