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.ch...@oracle.com> wrote: > > >> On Jun 4, 2015, at 8:49 AM, Dmitry Samersoff <dmitry.samers...@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.samers...@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. >