<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<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="-webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; font-family: Times; ">
<pre>_last_gc_was_young = gcs_are_young() ? true : false;</pre>
</span><span class="Apple-style-span"
style="-webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; 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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/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>
</body>
</html>