<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body style="background-color: rgb(255, 255, 255); color: rgb(0, 0,
0);" bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 06/03/2015 08:36 PM, Mandy Chung
wrote:<br>
</div>
<blockquote cite="mid:556F491E.3070709@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<br>
<br>
On 06/03/2015 08:29 AM, Dmitry Samersoff wrote:
<br>
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->Updated
webrev:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13">http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13</a>
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
I reviewed the jdk change. The version looks okay and some
comments:
<br>
<br>
ReferenceQueue.java - you should eliminate the use of rawtype:
<br>
<br>
84 Reference rn = r.next;
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<blockquote cite="mid:556F491E.3070709@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<br>
Change Reference to Reference<? extends T>
<br>
<br>
The forEach method - eliminate the use of raw type and
<br>
move @SuppressWarning to the field variable in line 182:
<br>
<br>
@SuppressWarnings("unchecked")
<br>
Reference<? extends T> rn = r.next;
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
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:<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~plevart/misc/Finalizer.printFinalizationQueue/webrev.03/">http://cr.openjdk.java.net/~plevart/misc/Finalizer.printFinalizationQueue/webrev.03/</a><br>
<br>
<blockquote cite="mid:556F491E.3070709@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<br>
FinalizerHistogram.java
<br>
Copyright year needs update.
<br>
<br>
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.
<br>
<br>
The comment in line 35 is suitable to move to the class
description and this is the suggestion.
<br>
<br>
// This FinalizerHistogram class is for GC.finalizer_info
diagnostic command support.
<br>
// It is invoked by the VM.
<br>
<br>
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++".
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
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.<br>
<br>
Regards, Peter<br>
<br>
<blockquote cite="mid:556F491E.3070709@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<br>
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.
<br>
<br>
A minor naming comment - now you have a FinalizerHistogram class.
Perhaps the getFinalizerHistogram method should be renamed e.g.
"dump"?
<br>
<br>
Mandy
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
</body>
</html>