> On Jun 3, 2015, at 12:22 PM, Peter Levart <peter.lev...@gmail.com> wrote: > > > On 06/03/2015 08:36 PM, 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> >> >> 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; > > Thanks, Mandy. This was my doing. The @SuppressWarnings can be moved to the > local var declaration in line 84 too. Here's how the fixed ReferenceQueue > looks like after those two changes: > > http://cr.openjdk.java.net/~plevart/misc/Finalizer.printFinalizationQueue/webrev.03/
This looks good. > >> >> FinalizerHistogram.java >> Copyright year needs update. >> >> 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. >> >> Should getFinalizerHistogram() return FinalizerHistogramEntry[]? The VM >> knows the returned type anyway. 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++". > > This FinalizerHistogram.Entry naming part came to my mind too, yes. If there > is a method for incrementing, then both fields can be marked private. Yup. Mandy > > Regards, Peter > >> >> 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. >> >> A minor naming comment - now you have a FinalizerHistogram class. Perhaps >> the getFinalizerHistogram method should be renamed e.g. "dump"? >> >> Mandy >
