RFR(M, v14): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo
Dmitry Samersoff
dmitry.samersoff at oracle.com
Thu Jun 4 13:38:15 UTC 2015
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.
More information about the hotspot-gc-dev
mailing list