Thanks - this version looks good to me. /Staffan
> On 5 jun 2015, at 13:00, Dmitry Samersoff <dmitry.samers...@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.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. >>> >> > > > -- > Dmitry Samersoff > Oracle Java development team, Saint Petersburg, Russia > * I would love to change the world, but they won't give me the sources.
