<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
Hello Dima,<br>
<br>
<div class="moz-cite-prefix">On 2015-04-16 13:40, Dmitry Fazunenko
wrote:<br>
</div>
<blockquote cite="mid:552F9FA8.3030403@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
Hello,<br>
<br>
Would you review a simple fix in G1.<br>
<br>
Short description: <br>
after introduction G1Log - dynamic changes of PrintGC and
PrintGCDetails flag has no effect anymore, because G1Log looks for
these flags during initialization only. The fix: sync the log
level with the flags values.<br>
<br>
A huge thanks to Jesper who helped me a lot with my first product
fix.<br>
<br>
Bug: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8073476">https://bugs.openjdk.java.net/browse/JDK-8073476</a><br>
Webrev: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edfazunen/8073476/webrev.06/">http://cr.openjdk.java.net/~dfazunen/8073476/webrev.06/</a><br>
</blockquote>
<br>
Sorry, but I don't really like the way this is solved. With this
approach calling G1GCPhaseTimes::print() suddenly has the side
effect that it resets the log level. That's quite unexpected for me.
Especially if you consider the code path in
G1CollectedHeap::log_gc_footer() where we do this:<br>
<br>
void G1CollectedHeap::log_gc_footer(double pause_time_sec) {<br>
if (!G1Log::fine()) {<br>
return;<br>
}<br>
<br>
if (G1Log::finer()) {<br>
...<br>
g1_policy()->phase_times()->print(pause_time_sec);<br>
...<br>
}<br>
<br>
If we don't have G1Log::fine() (which is PrintGC) enabled we will
never call the print() method and will thus not detect any changes
made by the MXBean. If we have G1Log::finer() enabled we enter the
logging code, print other things at the "finer" level (which is
PrintGCDetails) and then do the call to the print() method where we
can suddenly decide that PrintGCDetails no longer is enabled and not
do the rest of the logging. So for the same GC we will print some
stuff at PrintGCDetails level and some things at another level.
Strange.<br>
<br>
<br>
I would prefer to have a hook when the MXBean changes the value and
only update the level at that point.<br>
<br>
Having said that I am not sure that this bug is worth fixing right
now. I am currently working on the JEP to make the GC logging use
the new unified logging format. That will change all of this and
most likely remove the G1Log class all together. So, my suggestion
would be to leave this as is for now and instead add the MXBean
requirement to the unified logging work.<br>
<br>
Thanks,<br>
Bengt<br>
<blockquote cite="mid:552F9FA8.3030403@oracle.com" type="cite"> <br>
Testing: <br>
I ran manually the test from the bug report to make sure the
change fixes the problem.<br>
A regression test will be delivered separately as a fix of <a
moz-do-not-send="true" id="key-val" rel="4774806"
href="https://bugs.openjdk.java.net/browse/JDK-8077056">JDK-8077056</a><br>
<br>
Thanks,<br>
Dima </blockquote>
<br>
</body>
</html>