Mandy, Thank you for the review.
Updated webrev is: http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14 addressed comments, added a unit test to JDK workspace. 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[] > 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. (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.
