<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>
Hi Vitaly,<br>
<br>
Thanks for looking at this change too!<br>
<br>
On 2012-08-22 14:22, Vitaly Davidovich wrote:<br>
</div>
<blockquote
cite="mid:CAHjP37GisSK_M3u3PzWXncANsgcda_LHY+-FSVGLJmuRPYAwiA@mail.gmail.com"
type="cite">
<p>Hi Bengt,</p>
<p>A few minor comments.</p>
<p>In g1CollectedHeap.cpp:</p>
<p>1) probably doesn't matter much in its current form, but would
it be better to call os::elapsedTime() right before
prepare_verify() so that the timing info captures just the
verification time and not printing to tty/gclog and constructing
the HandleMark? This is in verify().</p>
</blockquote>
<br>
Actually, I think it is good that the extra printing is included in
the timing. We want to time the extra cost of turning on the
verification, thus we should include the full cost of it.<br>
<br>
<blockquote
cite="mid:CAHjP37GisSK_M3u3PzWXncANsgcda_LHY+-FSVGLJmuRPYAwiA@mail.gmail.com"
type="cite">
<p>2) it seems a bit weird to have the "guard" parameter in the
verify() method. Why wouldn't caller just check the guard and
then only call if it passes? Not a big deal though, just
stylistic.</p>
</blockquote>
<br>
I see your point. As you say, it is just a style question. If I
remove the guard parameter to verify() I'd have to add the test to
<title>Snippet</title>
verify_before_gc() and verify_after_gc() and that kind of
re-introduces some of the code duplication that I was aiming to
remove. If you feel strongly about it I'll change.<br>
<br>
<blockquote
cite="mid:CAHjP37GisSK_M3u3PzWXncANsgcda_LHY+-FSVGLJmuRPYAwiA@mail.gmail.com"
type="cite">
<p>3) in verify_before/after(), would it make sense to only record
the time if it's not 0.0? I'm thinking that the pointer chasing
may possible get a cache miss only to record a 0. Again, pure
speculation.</p>
</blockquote>
<br>
These values are set for each GC. If we don't set them to 0
somewhere we will get the values from the last GC. By always setting
the value there is no risk that we get the wrong values.<br>
<br>
<blockquote
cite="mid:CAHjP37GisSK_M3u3PzWXncANsgcda_LHY+-FSVGLJmuRPYAwiA@mail.gmail.com"
type="cite">
<p>4) is os::elapsedTime() using a monotonic time source? If not,
what would happen if you get a negative value in the timing?</p>
</blockquote>
<br>
os::elapsedTime() is a monotonic time source.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote
cite="mid:CAHjP37GisSK_M3u3PzWXncANsgcda_LHY+-FSVGLJmuRPYAwiA@mail.gmail.com"
type="cite">
<p>Looks good otherwise.</p>
<p>Thanks</p>
<p>Sent from my phone</p>
<div class="gmail_quote">On Aug 22, 2012 7:21 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:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Hi again,<br>
<br>
Here is an updated webrev based on comments from John
Cuthbertson:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ebrutisso/7178363/webrev.02/"
target="_blank">http://cr.openjdk.java.net/~brutisso/7178363/webrev.02/</a><br>
<br>
Here is a diff compared to the previous webrev:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ebrutisso/7178363/webrev.01-02-diff/"
target="_blank">http://cr.openjdk.java.net/~brutisso/7178363/webrev.01-02-diff/</a><br>
<br>
Thanks,<br>
Bengt<br>
<br>
On 2012-07-20 14:17, Bengt Rutisson wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Hi all,<br>
<br>
Here is an updated webrev for this change:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ebrutisso/7178363/webrev.01/"
target="_blank">http://cr.openjdk.java.net/~brutisso/7178363/webrev.01/</a><br>
<br>
It turns out the the earlier change for 7178361 had
introduced two more issues: the heap transition information
for the PrintGC output and the output for evacuation
failures had both been moved in an unintended way. The above
webrev corrects both of these chagne too. Thanks John
Cuthbertson for pointing me to the evacuation failure
output.<br>
<br>
Bengt<br>
<br>
<br>
On 2012-07-19 11:01, Bengt Rutisson wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Hi again,<br>
<br>
Just in case anybody already started looking at this: I
have updated the webrev since I had to make some changes
to make it compile with the NMT fixes that have just made
it into the hotspot-gc repository. Those updates were just
to make it compile and not really related to my change, so
I just overwrote the existing webrev. Just use the same
link as before:<br>
<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ebrutisso/7178363/webrev.00/"
target="_blank">http://cr.openjdk.java.net/~brutisso/7178363/webrev.00/</a><br>
<br>
Also, if you want to see what the new output looks like I
am attaching a file called new.txt with an example from
running SpecJBB2005 with this command line:<br>
<br>
-XX:+UseG1GC -XX:ParallelGCThreads=4 -XX:+UnlockExperimentalVMOptions
-XX:G1LogLevel=finest -XX:+TraceGen0Time -Xms256m -Xmx2G<br>
<br>
I am also attaching a file called old.txt with what the
output, using the same command line, looked like before my
change. As you can see the differences are what I listed
in my earlier email. You will also notice that the "old"
version has an entry for the SATB filtering, even though
all the entries are 0 (we didn't do a concurrent cycle so
there has been no SATB filtering). This was a bug I just
introduced with my last change (for 7178361), so the new
example is more correct.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
On 2012-07-18 15:55, Bengt Rutisson wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Hi everyone,<br>
<br>
I would like some reviews of this change:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ebrutisso/7178363/webrev.00/"
target="_blank">http://cr.openjdk.java.net/~brutisso/7178363/webrev.00/</a><br>
<br>
This removes the special treatment for
ParallelGCThreads=0 from the G1 logging. I did keep the
log output unchanged for that case. Basically it just
has one indentation level less and skips some output. I
am not sure this is really necessary since it is really
a special case. I'm open to change that special
treatment too and just have the same output as for
ParallelGCThreads=1.<br>
<br>
The PrintGCDetails log output should be the same as
before with three minor adjustments:<br>
<br>
- The "Sum" is now not printed for the start and end
values for GC workers. This sum does not really make
sense to me.<br>
<br>
- The "(ms)" unit was removed from output that aren't in
milliseconds (termination attempts for example).<br>
<br>
- The average value is now printed as a double for all
types.<br>
<br>
I tried to clean up the code a bit and introduced a
separate class, Snippet WorkerDataArray, to keep track
of the per thread logging. I also introduced getters and
setters to avoid having to make G1CollectorPolicy and
TraceGen0TimeData friend classes to G1GCPhaseTimes.<br>
<br>
Thanks,<br>
Bengt<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>