<!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>
The latest webrev looks good to me.<br>
<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>
</body>
</html>