<!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>
Overall this looks good to me, but I do have a couple of minor comments
and questions....<br>
<br>
arguments.cpp - Typo @ line 3096<br>
<br>
gcCause.hpp - fields in the new class should have a leading underscore.<br>
<br>
g1CollectedHeap.cpp - @ line 3600. IMO using a local to hold the GC
string and pass that into the TraceTime constructor would improve the
readability significantly.<br>
<br>
g1CollectorPolicy.cpp - @ line 888. If you have a log level == finer,
where is the print of the date/timestamp prefix now?<br>
<br>
Other than that, it looks good?<br>
<br>
JohnC<br>
<br>
On 05/14/12 00:46, Bengt Rutisson wrote:
<blockquote cite="mid:4FB0B85F.9060400@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<br>
Hi again, :-)<br>
<br>
Here is an updated webrev where PrintGCCause is off by default in JDK7
but on by default in JDK8:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebrutisso/7166894/webrev.05/">http://cr.openjdk.java.net/~brutisso/7166894/webrev.05/</a><br>
<br>
The relevant change is in arguments.cpp:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebrutisso/7166894/webrev.05/src/share/vm/runtime/arguments.cpp.udiff.html">http://cr.openjdk.java.net/~brutisso/7166894/webrev.05/src/share/vm/runtime/arguments.cpp.udiff.html</a><br>
<br>
With this proposal there is still one slight change in JDK7 logging.
For full GCs that were triggered by System.gc() calls we used to log
the cause. Now this will not happen unless you add -XX:+PrintGCCause.
This will not break any parsers, but it might make some parsers miss
System.gc() calls.<br>
<br>
I think we are getting closer to wrapping this change up. It is a
little unclear to me who would like to be listed as reviewers. Could
those that would like to be reviewers please take a look at the latest
webrev and let me know.<br>
<br>
Thanks,<br>
Bengt <br>
<br>
<br>
On 2012-05-11 19:51, Srinivas Ramakrishna wrote:
<blockquote
cite="mid:CABzyjyk7xwBUSWcEuh8FmqwJ_zRKcm6CnrwY7Af0DbzqizZAMg@mail.gmail.com"
type="cite">Mikael, thanks for sounding that note of caution... what
you say makes sense.<br>
<br>
-- ramki<br>
<br>
<div class="gmail_quote">On Fri, May 11, 2012 at 10:17 AM, Mikael
Vidstedt <span dir="ltr"><<a moz-do-not-send="true"
href="mailto:mikael.vidstedt@oracle.com" target="_blank">mikael.vidstedt@oracle.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote"
style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div bgcolor="#FFFFFF" text="#000000"> <br>
I'm all for improving the information in the log messages, great work!
However, I'm not sure I'm warm and fuzzy about potentially breaking
users' log parsers in a minor update. My preference would be to have
the PrintGCCause flag be default false in jdk7 and default true in
jdk8+. In general I'd prefer to only change the log messages in major
releases.<br>
<br>
Reasonable?<br>
<br>
Cheers,<br>
Mikael
<div>
<div class="h5"><br>
<br>
On 2012-05-11 07:30, Bengt Rutisson wrote:
<blockquote type="cite"> <br>
Hi Kris,<br>
<br>
Thanks again for looking at this.<br>
<br>
I had to make some minor changes make it compile on all platforms.
Mostly some explicit casts to const char*. Here is an updated webrev:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ebrutisso/7166894/webrev.04/"
target="_blank">http://cr.openjdk.java.net/~brutisso/7166894/webrev.04/</a><br>
<br>
More comments inline.<br>
<br>
On 2012-05-08 16:43, Krystal Mok wrote:
<blockquote type="cite">Hi Bengt,
<div><br>
</div>
<div>The current factoring looks nice and uniform. Thanks :-)</div>
<div><br>
</div>
<div>But for most minor GCs and both CMS pause phases, the
extra logging doesn't really give additional information.</div>
<div>Most minor GCs are going to say "Allocation Failure",
and the two CMS phases would change from, e.g.</div>
<div><br>
</div>
<div>[GC [1 CMS-initial-mark</div>
<div><br>
</div>
<div>to something like</div>
<div><br>
</div>
<div>[GC (CMS Initial Mark) [1 CMS-initial-mark</div>
<div><br>
</div>
<div>which is probably reasonable given the scope of the
change, but not really helpful.</div>
<div>The "real cause", such as which generation (or perhaps
System.gc() with ExplicitGCInvokesConcurrent, or even GC locker) is
triggering this collection cycle, may be more useful, but it's hard to
fit into the current form.</div>
</blockquote>
<br>
Yes, I think you are correct in both cases. The gc cause that we have
available does not always add a lot of information. This is relevant to
fix but it is a slightly different issue than what this patch sets out
to fix. Let's try to get this in first and then evaluate how the GC
causes should be set.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote type="cite">
<div><br>
</div>
<div>- Kris<br>
<br>
<div class="gmail_quote">On Tue, May 8, 2012 at 10:18 PM,
Bengt Rutisson <span dir="ltr"><<a moz-do-not-send="true"
href="mailto:bengt.rutisson@oracle.com" target="_blank">bengt.rutisson@oracle.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote"
style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div bgcolor="#FFFFFF" text="#000000"> <br>
Hi again everyone,<br>
<br>
It seems like the feedback on hotspot-gc-use is that we should add the
GC cause to all collectors but also provide a switch to turn this
logging off.<br>
<br>
Here is an updated webrev:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ebrutisso/7166894/webrev.03/"
target="_blank">http://cr.openjdk.java.net/~brutisso/7166894/webrev.03/</a><br>
<br>
Changes:<br>
* GC cause logged for all collectors<br>
* Added the flag -XX:-PrintGCCause to turn the new information off<br>
* Refactored the string concatenation code into a helper class<br>
<br>
I guess I will also have to update the CR to now reflect the fact that
this does not just concern full GCs anymore.<br>
<br>
Thanks,<br>
Bengt
<div>
<div><br>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</blockquote>
<br>
</blockquote>
<br>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>