<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
On 2012-05-30 14:44, Charlie Hunt wrote:
<blockquote
cite="mid:99012D24-F925-458E-B8C4-996C19A73FB0@salesforce.com"
type="cite">I like it. :-)</blockquote>
<br>
Thanks, Charlie!<br>
<br>
Bengt<br>
<br>
<blockquote
cite="mid:99012D24-F925-458E-B8C4-996C19A73FB0@salesforce.com"
type="cite">
<div><br>
</div>
<div>charlie ...</div>
<div><br>
<div>
<div>On May 30, 2012, at 7:32 AM, Bengt Rutisson wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div 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 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>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</body>
</html>