RFR(M, v13): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo
peter.levart at gmail.com
Wed Jun 3 19:22:22 UTC 2015
On 06/03/2015 08:36 PM, Mandy Chung wrote:
> On 06/03/2015 08:29 AM, Dmitry Samersoff wrote:
>> Updated webrev:
> 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:
> 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:
> 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.
> 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"?
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the serviceability-dev