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