<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
Hi John,<br>
<br>
Thanks for reviewing this!<br>
<br>
On 2012-05-31 18:42, John Cuthbertson wrote:
<blockquote cite="mid:4FC79F5C.60805@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Hi Bengt,<br>
<br>
I think this looks really good. Great clean up. <br>
<br>
One suggestion (but it's completely optional):<br>
<br>
* Could you add a comment saying that TraceGen0Time actually
collects
data on _both_ young and mixed evacuation pauses (the latter may
contain non-young regions - i.e. regions that are technically in
Gen1)
while TraceGen1Time collects data about full GCs?<br>
</blockquote>
<br>
Good idea! I added this comment to the TraceGen0TimeData class:<br>
<br>
// TraceGen0Time collects data on _both_ young and mixed evacuation
pauses<br>
// (the latter may contain non-young regions - i.e. regions that are<br>
// technically in Gen1) while TraceGen1Time collects data about full
GCs.<br>
class TraceGen0TimeData : public CHeapObj {<br>
<br>
<br>
All set to push this now.<br>
<br>
Thanks again for the review!<br>
Bengt<br>
<br>
<blockquote cite="mid:4FC79F5C.60805@oracle.com" type="cite">
<br>
JohnC<br>
<br>
On 05/30/12 05:32, Bengt Rutisson wrote:
<blockquote cite="mid:4FC61344.7080101@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<br>
Hi Charlie,<br>
<br>
Thanks for looking at this!<br>
<br>
On 2012-05-30 04:41, Charlie Hunt wrote:
<blockquote
cite="mid:EB86E2CA-9D43-465A-8559-0BB333DF7BB7@salesforce.com"
type="cite">Nice cleanup Bengt.
<div><br>
</div>
<div>One observation which I found a bit confusing.</div>
<div><br>
</div>
<div>In g1CollectorPolicy.cpp, line 2285 - 2287 we have the
following code:</div>
<div><span class="Apple-style-span" style="font-family:
Times;">
<pre>_last_gc_was_young = gcs_are_young() ? true : false;</pre>
</span><span class="Apple-style-span" style="font-family:
Times;">
<pre><span class="new" style="color: blue; font-weight: bold;">_trace_gen0_time_data.increment_collection_count(!_last_gc_was_young);</span></pre>
</span>
<div>The !_last_gc_was_young threw me a bit since we were
incrementing the collection count on a "trace gen0". Has
there been
consideration of using an enumerated type for a young GC
and mixed GC
type rather than using a boolean to represent the two?
Or,
alternatively introducing a mutator method for
incrementing only
_young_pause_num and a second mutator method for
incrementing
_mixed_pause_num ? <br>
</div>
</div>
</blockquote>
<br>
Good point. I went with your last suggestion and added separate
mutator
methods for mixed and young collections. Here is an updated
webrev:<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebrutisso/7172279/webrev.02/">http://cr.openjdk.java.net/~brutisso/7172279/webrev.02/</a><br>
<br>
<blockquote
cite="mid:EB86E2CA-9D43-465A-8559-0BB333DF7BB7@salesforce.com"
type="cite">
<div><br>
</div>
<div>I didn't see anything incorrect in the implementation ...
just
found this bit a little confusing.</div>
</blockquote>
<br>
Actually there was a mistake in the code too. I had moved the
young and
mixed counters from G1CollectorPolicy to TraceGen0TimeData. But
since I
wanted to verify my changes I had left the counters in
G1CollectorPolicy and I forgot to remove them before the first
webrev.
So I removed them now too.<br>
<br>
Thanks for drawing my attention to this as well! :-)<br>
Bengt<br>
<br>
<br>
<br>
<blockquote
cite="mid:EB86E2CA-9D43-465A-8559-0BB333DF7BB7@salesforce.com"
type="cite">
<div><br>
</div>
<div>hths,</div>
<div><br>
</div>
<div>charlie ...</div>
<div><br>
<div>
<div>On May 29, 2012, at 1:58 AM, Bengt Rutisson wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div><br>
Hi again,<br>
<br>
I should also have mentioned that this change should
not alter the log <br>
output. (Except for the ParallelGCThreads = 0 case
which now will look <br>
the same as PrallelGCThreads >= 1 case.)<br>
<br>
I tested this by leaving the old code in and comparing
the output from <br>
the old code with the new code. In my tests they look
exactly the same.<br>
<br>
Bengt<br>
<br>
On 2012-05-28 22:37, Bengt Rutisson wrote:<br>
<blockquote type="cite"><br>
</blockquote>
<blockquote type="cite">Hi all,<br>
</blockquote>
<blockquote type="cite"><br>
</blockquote>
<blockquote type="cite">Can I have a couple of code
reviews for
this change?<br>
</blockquote>
<blockquote type="cite"><a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ebrutisso/7172279/webrev.01/">http://cr.openjdk.java.net/~brutisso/7172279/webrev.01/</a><br>
</blockquote>
<blockquote type="cite"><br>
</blockquote>
<blockquote type="cite">Background<br>
</blockquote>
<blockquote type="cite">I am preparing a change to
remove the
special treatment of the single <br>
</blockquote>
<blockquote type="cite">threaded case for G1
PrintGCDetails
output. We should be able to use <br>
</blockquote>
<blockquote type="cite">the same code for both
ParallelGCThreads
= 0 and PrallelGCThreads >= <br>
</blockquote>
<blockquote type="cite">1. This will hopefully
simplify the
logging code.<br>
</blockquote>
<blockquote type="cite"><br>
</blockquote>
<blockquote type="cite">In preparation for that change
I would
like to clean up the <br>
</blockquote>
<blockquote type="cite">TraceGen0Time and
TraceGen1Time data
gathering code a bit. It is <br>
</blockquote>
<blockquote type="cite">currently kind of difficult to
follow the
code and there is no clear <br>
</blockquote>
<blockquote type="cite">distinction between the data
gathered for
these two cases compared to <br>
</blockquote>
<blockquote type="cite">the data gathered for
PrintGCDetails
output.<br>
</blockquote>
<blockquote type="cite"><br>
</blockquote>
<blockquote type="cite">Here is what I tried to do:<br>
</blockquote>
<blockquote type="cite"><br>
</blockquote>
<blockquote type="cite">- Remove the unnecessary
define_num_seq
macro and the multiple <br>
</blockquote>
<blockquote type="cite">inheritance of the Summary
class.<br>
</blockquote>
<blockquote type="cite">- Replaced the above with two
classes,
TraceGen0TimeData and <br>
</blockquote>
<blockquote type="cite">TraceGen1TimeData, that will
encapsulate
the data needed.<br>
</blockquote>
<blockquote type="cite">- Made sure that the data is
only updated
if the corresponding flags <br>
</blockquote>
<blockquote type="cite">are turned on.<br>
</blockquote>
<blockquote type="cite">- Removed the separate
"non-parallel"
case. All data is available even <br>
</blockquote>
<blockquote type="cite">with ParallelGCThreads = 0.<br>
</blockquote>
<blockquote type="cite">- Removed the ExitAfterGCNum
flag. I
hardly think this is an <br>
</blockquote>
<blockquote type="cite">appropriate product flag to
have in the
code.<br>
</blockquote>
<blockquote type="cite">- Removed some unused methods.<br>
</blockquote>
<blockquote type="cite">- Removed 7 year old
assert(true) with
accompanying comments.<br>
</blockquote>
<blockquote type="cite">- Removed the unused aux time
concept.<br>
</blockquote>
<blockquote type="cite"><br>
</blockquote>
<blockquote type="cite">There was one thing that I was
hesitant
to remove, but finally decided <br>
</blockquote>
<blockquote type="cite">to remove anyway. It was the
G1CollectorPolicy::check_other_times() <br>
</blockquote>
<blockquote type="cite">method. First I added the same
method to
TraceGen0TimeData. But it is <br>
</blockquote>
<blockquote type="cite">a lot of code and I am not
sure it is
very useful. It can't do any <br>
</blockquote>
<blockquote type="cite">exact checks and it seems
unlikely to me
that it will detect any <br>
</blockquote>
<blockquote type="cite">issues. Also, it seems to me
like the
code was in the wrong place. I <br>
</blockquote>
<blockquote type="cite">would have preferred it in
some kind of
verify method rather than in a <br>
</blockquote>
<blockquote type="cite">print method.<br>
</blockquote>
<blockquote type="cite"><br>
</blockquote>
<blockquote type="cite">Thanks,<br>
</blockquote>
<blockquote type="cite">Bengt<br>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>