> 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.
