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