<!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">
<br>
Hi Vitaly,<br>
<br>
Thanks for looking at this so quickly!<br>
<br>
On 2012-06-20 15:59, Vitaly Davidovich wrote:
<blockquote
cite="mid:CAHjP37Hiin5Ve9AUmUWZn9_oNfSM0JvBMoNYjrWKUen6=tv8UQ@mail.gmail.com"
type="cite">
<p>Hi Bengt,</p>
<p>Do you think it's worthwhile to add asserts to the double[]
setters in G1GCPhaseTimes to ensure that worker_i is within
bounds? It may not segfault but stomp memory instead if it's
outside the bounds. Maybe overkill though ...</p>
</blockquote>
<br>
Good point I added the asserts.<br>
<br>
I had been thinking of having extra checks for debug builds. We
could check that all values for the active threads have been set
before we try to use the values for anything. I think this might be
overkill though, so I'm leaving that out for now.<br>
<br>
<blockquote
cite="mid:CAHjP37Hiin5Ve9AUmUWZn9_oNfSM0JvBMoNYjrWKUen6=tv8UQ@mail.gmail.com"
type="cite">
<p>Also, should/can worker_i be uint? Or are negative values
expected? Or would this require casts from the caller because
the rest of the relevant code uses int?</p>
</blockquote>
<br>
Well, we use the worker_i value to directly index the arrays. A C++
array has to be indexed with a signed value.<br>
<br>
Instead I added asserts that the worker_i values are >= 0. I also
did (kind of the opposite of what you suggested) and made the
instance variable _active_workers into an int instead of uint. This
kind of make sense since we use int for the worker_i parameters.
Otherwise I would have had to add casts in the new asserts.<br>
<br>
I did one more minor change, so I'll send out updated webrevs in a
minute.<br>
<br>
Bengt<br>
<br>
<blockquote
cite="mid:CAHjP37Hiin5Ve9AUmUWZn9_oNfSM0JvBMoNYjrWKUen6=tv8UQ@mail.gmail.com"
type="cite">
<p>Regards,</p>
<p>VitalyVitaly</p>
<p>Sent from my phone</p>
<div class="gmail_quote">On Jun 20, 2012 9:14 AM, "Bengt Rutisson"
<<a moz-do-not-send="true"
href="mailto:bengt.rutisson@oracle.com">bengt.rutisson@oracle.com</a>>
wrote:<br type="attribution">
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<br>
Hi everyone,<br>
<br>
Could I please have some reviews for this change:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ebrutisso/7178361/webrev.00/"
target="_blank">http://cr.openjdk.java.net/~brutisso/7178361/webrev.00/</a><br>
<br>
Background<br>
As part of the PrintGC and PrintGCDetails logging there is
information about how long the GC pause was. The timing of the
pause was done differently in G1 depending on whether PrintGC
or PrintGCDetails were enabled. It turns out that
PrintGCDetails was just timing part of the pause.<br>
<br>
This change will make both PrintGC and PrintGCDetails use the
same timing. To achieve this I had to refactor the code a bit.
I moved all the timing data out of G1CollectorPolicy into a
new class called G1GCPhaseTimes.<br>
<br>
My intention is that this change should not alter the format
of the output of PrintGC or PrintGCDetails. It should just
correct the timing data.<br>
<br>
However, I did find that we are collecting timing information
about card counts, under an #ifdef. I moved this to the finest
log level instead. This does not change the existing format
for normal usage of PrintGC or PrintGCDetails.<br>
<br>
Also, I had to update how the TraceGen0Time data is logged. I
will have another look at this, but my idea was to leave the
format exactly as it was. However, I think the format has
decayed over time so maybe I'll try to clean it up.<br>
<br>
I intend to follow this change up with a change to remove the
special treatment of the single threaded case for
PrintGCDetails (tracked in CR 7178363).<br>
<br>
Finally, this work revealed an issue with how the ergonomics
in G1 measure the collection pauses. I did not want to change
this behavior now so I filed a separate RFE for that (7178365:
G1: Ergonomics only count part of the collection pause).<br>
<br>
Bengt<br>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>