Staffan, Could you review a CCC request.
http://ccc.us.oracle.com/8059036 -Dmitry On 2015-06-05 15:24, Staffan Larsen wrote: > 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. > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
