<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><br>
On 2012-07-12 23:34, John Cuthbertson wrote:<br>
</div>
<blockquote cite="mid:4FFF42E6.3060406@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Hi Bengt,<br>
<br>
The latest webrev looks good to me.<br>
</blockquote>
<br>
Thanks, John!<br>
<br>
Bengt<br>
<br>
<blockquote cite="mid:4FFF42E6.3060406@oracle.com" type="cite">
<br>
JohnC<br>
<br>
On 07/11/12 13:58, Bengt Rutisson wrote:
<blockquote cite="mid:4FFDE8E6.7020907@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi John,<br>
<br>
Thanks a lot for looking at this! Good comments as always!<br>
<br>
I applied your latest comments. (Good catch about the
initialization of
the card cleaning timing variables.)<br>
<br>
Here is an updated webrev:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebrutisso/7178361/webrev-04/">http://cr.openjdk.java.net/~brutisso/7178361/webrev-04/</a><br>
<br>
For convenience, here is a webrev with just the latest changes
that I
applied based on your two last emails:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebrutisso/7178361/webrev-03-04-diff/">http://cr.openjdk.java.net/~brutisso/7178361/webrev-03-04-diff/</a><br>
<br>
I'm getting back to work next week and I'd like to start on
the next
step (to clean up the single threaded logging) based on this
change.
So, I'd like to get this pushed before Monday.<br>
<br>
If I don't get any objections before tomorrow I'll push this.
I can
always include some changes in my next push as well if there
is late
feedback.<br>
<br>
Thanks again for the review!<br>
Bengt<br>
<br>
On 2012-07-04 00:07, John Cuthbertson wrote:<br>
</div>
<blockquote cite="mid:4FF36D24.6010304@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Hi Bengt,<br>
<br>
Finished looking at the rest of the webrev. Replies are
inline.....<br>
<br>
On 06/29/12 05:37, Bengt Rutisson wrote:
<blockquote cite="mid:4FEDA19B.6050600@oracle.com" type="cite">
<blockquote cite="mid:4FECF617.3070707@oracle.com"
type="cite"><br>
Line 102:<br>
Rename "start" to something like - note_gc_start().
"start" is too
closely associated with threads IMO. Also should it also
take the start
time? That way the "end" routine<br>
could take an end time and do any necessary calculations.<br>
</blockquote>
<br>
Done.<br>
</blockquote>
<br>
Thanks again - I can see you even extended the idea to include
other
cachable items from the start of the pause. It also looks
good.<br>
<br>
<br>
<blockquote cite="mid:4FEDA19B.6050600@oracle.com" type="cite">
<blockquote cite="mid:4FECF617.3070707@oracle.com"
type="cite">
Line 236:<br>
"accumulate_par_times" is a bit misnamed.
"collapse_par_times",
"average_par_times" might be better. Also I don't think
you should be
calling this directly from with<br>
G1CollectedHeap. It would be better to call this from a
G1GCPhaseTimes::note_gc_end() type of routine, which is
called by
G1CollectorPolicy::record_collection_pause_end().<br>
</blockquote>
<br>
<br>
I renamed it to collapse_par_times(), but I still would like
to call it
directly from G1CollectedHeap. The reason is that
record_collection_pause_end() need this to be called, but
also the
note_gc_end() method. I don't like the note_gc_end() method
to depend
on a call to record_collection_pause_end().<br>
<br>
The clean solution would be to merge these two methods, but
that's not
possible without changing the timing for the ergonomics,
which is
exactly what I am trying to avoid.<br>
</blockquote>
<br>
OK. I was expecting note_gc_end() to be called by
record_collection_pause_end() but I see your point - they are
kind of
inter-dependent. I'm OK with the current solution. I agree
that it's
good not to change the timer for the ergonomics - it covers
the part of
the GC we can control and make changes to.<br>
<br>
<blockquote cite="mid:4FEDA19B.6050600@oracle.com" type="cite">
<blockquote cite="mid:4FECF617.3070707@oracle.com"
type="cite"><br>
g1CollectorPolicy.hpp<br>
---------------------<br>
Line 65-66:<br>
Why did you remove the indentation levels? IMO they are
easier to work
with than explicitly listing spaces.<br>
</blockquote>
<br>
Two reasons. One technical. I moved LineBuffer (that handled
the
indentation) into g1GCPhaseTime.cpp and I didn't really want
to expose
it outside of that module. The other, more design related
question, is
whether the TraceGen0Time output should really use the same
formatting
as the PrintGCDetails formatting. As it was it was not doing
a good job
with the indentations. It was also pretty difficult to read
the code
and guess how many white spaces would end up where. With the
white
spaces explicitly in the print statements it got clearer
what really
happened and I could fix the indentation issues.<br>
<br>
I'm sure we can introduce some indentation abstraction for
TraceGen0Time too, but since I didn't really like the way it
was and I
had to move the LineBuffer I thought it was easiest to
revert back to
using explicit space characters in the print statements.<br>
</blockquote>
<br>
I think we'll agree to disagree with this one - but I don't
feel that
strongly so let's go with your current solution.<br>
<br>
I'm OK with your replies to the remaining comments.<br>
<br>
Based upon the latest patch, however, I think you need the
following
initializers:<br>
<blockquote>
<pre><span class="removed">_min_clear_cc_time_ms(-1.0),</span>
<span class="removed">_max_clear_cc_time_ms(-1.0),</span>
<span class="removed">_cur_clear_cc_time_ms(0.0),</span>
<span class="removed">_cum_clear_cc_time_ms(0.0),</span>
<span class="removed">_num_cc_clears(0L),
</span></pre>
</blockquote>
<span class="removed">in the G1GCPhaseTimes</span> constructor
-
otherwise the min, max, and cumulative card count times end up
invalid:<br>
<br>
[Cur Clear CC: 0.0 ms]<br>
[Cum Clear CC:
-7478635795308384178678626234477338765623121979189748343809653167004330313976030486954447383286129802186779679841804100646674790742262819242223986790247088643994984488195310211210345403332642667378471000230265516264857328262241129957569855488.0
ms]<br>
[Min Clear CC: 0.0 ms]<br>
[Max Clear CC: 0.0 ms]<br>
<br>
Also, since the previous version of the finest log-level
didn't display
these items, perhaps only display them if Verbose is also
enabled? In
most cases card cache count clearing will exhibit itself as a
higher
HC Worker Other time for worker 0.<br>
<br>
Everything else looks OK.<br>
<br>
Thanks,<br>
<br>
JohnC<br>
</blockquote>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
<br>
</body>
</html>