<!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>
On 2012-08-22 16:12, Vitaly Davidovich wrote:
<blockquote
cite="mid:CAHjP37FrOkd2uWLWmpXyfAto6LLS0=PKEFOxjsofhkFLz=U1_A@mail.gmail.com"
type="cite">
<p>Bengt,</p>
<p>Sorry, one follow-up after looking at the code again. In the
guard case, if the VerifyXXX isn't set, then the value will
always be 0.0, even in prior GCs. Are you saying these flags
can be changed dynamically, perhaps by some management API? If
not, then these methods will always produce and store 0, which
seems a bit wasteful. Did I miss something?</p>
</blockquote>
<br>
Currently the Verify* flags can not be turned on or off at runtime.
There is however the VerifyGCStartAt to consider, so even when the
Verify* flags are on it is not sure that we do verification every
GC.<br>
<br>
It seems safer and simpler to me to always set the values. We only
do this once per GC so I think the potential performance effects are
very limited. Always setting the value will also be robust if we
decide to make the Verify* flags manageable in the future.<br>
<br>
Cheers,<br>
Bengt<br>
<br>
<blockquote
cite="mid:CAHjP37FrOkd2uWLWmpXyfAto6LLS0=PKEFOxjsofhkFLz=U1_A@mail.gmail.com"
type="cite">
<p>Sent from my phone</p>
<div class="gmail_quote">On Aug 22, 2012 9:50 AM, "Vitaly
Davidovich" <<a moz-do-not-send="true"
href="mailto:vitalyd@gmail.com" target="_blank">vitalyd@gmail.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;">
<p>Thanks for the explanation Bengt - all good to me.</p>
<p>Sent from my phone</p>
<div class="gmail_quote">On Aug 22, 2012 9:39 AM, "Bengt
Rutisson" <<a moz-do-not-send="true"
href="mailto:bengt.rutisson@oracle.com" target="_blank">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;">
<div bgcolor="#FFFFFF" text="#000000">
<div><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 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 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 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 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 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 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"
target="_blank">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 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: 0pt
0pt 0pt 0.8ex; border-left: 1px solid rgb(204,
204, 204); 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:
0pt 0pt 0pt 0.8ex; border-left: 1px solid
rgb(204, 204, 204); 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:
0pt 0pt 0pt 0.8ex; border-left: 1px solid
rgb(204, 204, 204); 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>
</div>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>