<!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 John,<br>
<br>
Thanks for looking at this!<br>
<br>
On 2012-05-14 21:00, John Cuthbertson wrote:
<blockquote cite="mid:4FB1563A.4070708@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
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>
</blockquote>
<br>
Fixed.<br>
<br>
<blockquote cite="mid:4FB1563A.4070708@oracle.com" type="cite">
<br>
gcCause.hpp - fields in the new class should have a leading
underscore.<br>
</blockquote>
<br>
Of course! I keep missing that when I create new classes. Thanks for
finding it! Fixed.<br>
<br>
<blockquote cite="mid:4FB1563A.4070708@oracle.com" type="cite">
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>
</blockquote>
<br>
Fixed.<br>
<br>
<blockquote cite="mid:4FB1563A.4070708@oracle.com" type="cite">
<br>
g1CollectorPolicy.cpp - @ line 888. If you have a log level ==
finer,
where is the print of the date/timestamp prefix now?<br>
</blockquote>
<br>
Good catch! I clearly removed one line too much. Added it back.<br>
<br>
Here is an updated webrev:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/7166894/webrev.06/">http://cr.openjdk.java.net/~brutisso/7166894/webrev.06/</a><br>
<br>
Thanks again for looking at this!<br>
Bengt<br>
<br>
<br>
<br>
<blockquote cite="mid:4FB1563A.4070708@oracle.com" type="cite">
<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>
</blockquote>
<br>
</body>
</html>