RFR(M, v14): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo
Staffan Larsen
staffan.larsen at oracle.com
Fri Jun 5 08:20:13 UTC 2015
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.
>
More information about the hotspot-gc-dev
mailing list