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.
