<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
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>
<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>
</body>
</html>