<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><br>
Hi Dima,<br>
<br>
On 27/04/15 18:32, Dmitry Fazunenko wrote:<br>
</div>
<blockquote cite="mid:553E6492.3060804@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
Hi Bengt,<br>
<br>
Thanks for catching this.<br>
I think, that giving -XX:G1LogLevel="" should cause the VM start
error, because empty string is none of legal values.<br>
</blockquote>
<br>
Agreed. But since G1LogLevel will be removed soon I don't think it
is worth introducing stricter checking for that flag now.<br>
<br>
<blockquote cite="mid:553E6492.3060804@oracle.com" type="cite"> <br>
If you want to treat -XX:G1LogLevel="" as if unset, I can fix as
you said. <br>
I think it's not very important, because you are going to rewrite
this code soon.<br>
</blockquote>
<br>
I don't think it s very important. But if you want to be on the safe
side I would suggest making the change to keep the old behavior.
I'll leave it up to you to decide.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:553E6492.3060804@oracle.com" type="cite"> <br>
Thanks,<br>
Dima<br>
<br>
<br>
<br>
<br>
<div class="moz-cite-prefix">On 27.04.2015 18:55, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:553E5BE5.4000400@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
Hi Dima,<br>
<br>
<div class="moz-cite-prefix">On 2015-04-27 15:32, Dmitry
Fazunenko wrote:<br>
</div>
<blockquote cite="mid:553E3A7A.4040008@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi Bengt,<br>
<br>
Updated variant of fix with all your comments addressed:<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edfazunen/8073476/webrev.08/">http://cr.openjdk.java.net/~dfazunen/8073476/webrev.08/</a><br>
</blockquote>
<br>
Thanks! Looks much better.<br>
<br>
One small detail. You have changed the behavior of this command
line:<br>
<br>
-XX:+UnlockExperimentalVMOptions -XX:G1LogLevel=""
-XX:+PrintGCDetails<br>
<br>
Before your change this would result in the log level being
"finer" due to the PrintGCDetails flag. But after your change we
will get log level None.<br>
<br>
It is not an interesting use case in my mind, but an easy fix
would be to introduce a state variable in G1Log to track whether
we should look at PrintGC* flags.<br>
<br>
So, in G1Log::init() you could do something like:<br>
<br>
_log_level_flag_used = G1LogLevel != NULL &&
G1LogLevel[0] != '\0';<br>
if (_log_level_flag_used) {<br>
<br>
} else <br>
update_level();<br>
}<br>
<br>
<br>
And update_level() would also look at the stat flag instead of
checking FLAG_IS_DEFAULT(G1LogLevel). That way init() and
update_level() would be consistent and we would keep the same
behavior as before your change.<br>
<br>
What do you think?<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<br>
<br>
<blockquote cite="mid:553E3A7A.4040008@oracle.com" type="cite">
<br>
Thanks,<br>
Dima<br>
<br>
<br>
<div class="moz-cite-prefix">On 21.04.2015 16:18, Dmitry
Fazunenko wrote:<br>
</div>
<blockquote cite="mid:55364E19.8010207@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Bengt,<br>
<br>
Thanks for your time!<br>
A have a question inline:<br>
<br>
<div class="moz-cite-prefix">On 21.04.2015 15:19, Bengt
Rutisson wrote:<br>
</div>
</blockquote>
<blockquote cite="mid:55364E19.8010207@oracle.com" type="cite">
<div class="moz-cite-prefix"> </div>
<blockquote cite="mid:5536405F.10104@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
Hi Dima,<br>
<br>
<div class="moz-cite-prefix">On 2015-04-21 13:33, Dmitry
Fazunenko wrote:<br>
</div>
<blockquote cite="mid:55363597.3040006@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<tt>Hi Bengt,<br>
<br>
This is modified fixed based on your comments:<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edfazunen/8073476/webrev.07/">http://cr.openjdk.java.net/~dfazunen/8073476/webrev.07/</a><br>
<br>
Thanks for your assistance and good ideas!<br>
</tt></blockquote>
<br>
Thanks for making these change. Looks much better to me.<br>
<br>
A couple of comments:<br>
<br>
G1CollectedHeap::do_collection() is only used for full
GCs. You probably want to add a call to
G1Log::update_level() in
G1CollectedHeap::do_collection_pause_at_safepoint() too to
make sure that it is called for young GCs as well.<br>
</blockquote>
<br>
agree. good catch!<br>
<br>
<blockquote cite="mid:5536405F.10104@oracle.com" type="cite">
<br>
<br>
I think it looks odd that G1Log::init() and
G1Log::update_level() use different guards for when to
take PrintGC and PrintGCDetails into account.<br>
<br>
51 void G1Log::init() {<br>
52 if (G1LogLevel != NULL && G1LogLevel[0] !=
'\0')<br>
<br>
37 void G1Log::update_level() {<br>
38 if (FLAG_IS_DEFAULT(G1LogLevel)) {<br>
<br>
Is there a difference between (G1LogLevel != NULL
&& G1LogLevel[0] != '\0') and
(FLAG_IS_DEFAULT(G1LogLevel)) ? Can we use the same guard
in both places?<br>
</blockquote>
<br>
I though about using the same expression in both cases. The
difference in handling the empty value: -XX:G1LogLevel= <br>
So, the question, how this case should be handled:<br>
- a warning to be printed out and level set to 'none'<br>
- treated in the same way as 'none'<br>
<br>
<br>
<blockquote cite="mid:5536405F.10104@oracle.com" type="cite">
<br>
<br>
g1Log.hpp:<br>
<br>
I don't think this comment helps much:<br>
<br>
+ // to be invoked on VM start <br>
static void init();<br>
<br>
The name init() makes it pretty clear to me what the
method is expected to do. I would just leave the comment
out.<br>
</blockquote>
I have a bit different vision, but I'm okay to remove the
comment<br>
<br>
<br>
<blockquote cite="mid:5536405F.10104@oracle.com" type="cite">
<br>
The comment for update_level() talks about where the
method should be called instead of what it does. That is
normally not such a good thing for comments. Better to
explain what it does so it can be called in all places
where it is needed in the future.<br>
<br>
So instead of:<br>
<br>
+ // to be invoked on GC start or flag values change<br>
+ static void update_level();<br>
<br>
I would prefer:<br>
<br>
// Update the log level to reflect runtime changes to
manageable flags.<br>
</blockquote>
agree.<br>
<br>
Thanks,<br>
Dima<br>
<br>
<br>
<blockquote cite="mid:5536405F.10104@oracle.com" type="cite">
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:55363597.3040006@oracle.com"
type="cite"><tt> <br>
-- Dima<br>
<br>
<br>
</tt>
<div class="moz-cite-prefix">On 20.04.2015 11:34, Bengt
Rutisson wrote:<br>
</div>
<blockquote cite="mid:5534BA0B.5010906@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
Hi Dima,<br>
<br>
<div class="moz-cite-prefix">On 2015-04-17 15:51,
Dmitry Fazunenko wrote:<br>
</div>
<blockquote cite="mid:55310FCF.5030109@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<tt>Bengt,<br>
<br>
Please find comments inline.<br>
<br>
</tt>
<div class="moz-cite-prefix">On 17.04.2015 9:43,
Bengt Rutisson wrote:<br>
</div>
<blockquote cite="mid:5530AB92.2050509@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
Hi Dima,<br>
<br>
<div class="moz-cite-prefix">On 2015-04-16 15:36,
Dmitry Fazunenko wrote:<br>
</div>
<blockquote cite="mid:552FBAF5.1060908@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi Bengt,<br>
<br>
thanks for looking.<br>
<br>
<div class="moz-cite-prefix">On 16.04.2015
15:42, Bengt Rutisson wrote:<br>
</div>
<blockquote
cite="mid:552FAE22.4020009@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<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>
</blockquote>
<br>
May it's not clear, but the change will have
effect only if PrintGC or PrintGCDetails has
been changed during execution.<br>
Otherwise, the level will not change.<br>
</blockquote>
<br>
I understand that, but what I was saying with the
example above is that you will not get the desired
effect if you change PrintGC or PrintGCDetails at
runtime. If they are disabled, and then turn them
on at runtime you will not enter the update code
because of this check at the start of the
log_gc_footer() method:<br>
<br>
if (!G1Log::fine()) {<br>
return;<br>
}<br>
<br>
<br>
Similarly turning them off at runtime will start
to have an effect in the middle of a GC. Which is
probably not the effect you are looking for.<br>
</blockquote>
fine() invokes level(), level() invokes update(), so
G1::fine() will return the up to date value:<br>
<br>
<pre> inline static bool fine() {
<span class="removed">- return _level >= LevelFine;</span>
<span class="new">+ return level() >= LevelFine;</span>
}</pre>
<br>
<pre> static LogLevel level() {
<span class="new">+ // PringGC and PrintGCDetails are dynamic flags</span>
<span class="new">+ // _level should be in sync with the latest values</span>
<span class="new">+ update_level();</span>
return _level;
}</pre>
<br>
</blockquote>
Oh, I missed that you had changed fine() to call
level(). This means that all calls to G1Log interface
has side effects. I don't like that at all.<br>
<br>
<blockquote cite="mid:55310FCF.5030109@oracle.com"
type="cite">
<blockquote cite="mid:5530AB92.2050509@oracle.com"
type="cite"> <br>
This is why making the level() getter having the
side effect of changing the log level is a bad
idea. You have no control over when that happens.<br>
</blockquote>
<br>
It must be up to those who change flags at runtime.<br>
But the following code should work:<br>
<br>
MXBean.set(PrintGC, false);<br>
System.gc();<br>
MXBean.set(PrintGC, true);<br>
<br>
<blockquote cite="mid:5530AB92.2050509@oracle.com"
type="cite"> <br>
<blockquote cite="mid:552FBAF5.1060908@oracle.com"
type="cite"> <br>
To me, it's strange, that when I disable PrintGC
in G1 I still seeing gc log messages... <br>
Or, I can't turn logging on during execution
with G1.<br>
This works well for other collectors.<br>
<br>
Would it make sense to make PrintGC and
PrintGCDetails unmanageable in case of g1?<br>
</blockquote>
<br>
I think they can be manageble but their changed
states need to be handled more explicitly.<br>
<br>
<blockquote cite="mid:552FBAF5.1060908@oracle.com"
type="cite"> <br>
<br>
<blockquote
cite="mid:552FAE22.4020009@oracle.com"
type="cite"> <br>
<br>
I would prefer to have a hook when the MXBean
changes the value and only update the level at
that point.<br>
</blockquote>
<br>
Yes, it would be ideal solution. But I haven't
found such a mechanism supported. And it's
unlikely worth adding just for this case.<br>
In my version, the sync will be done more
frequently. <br>
</blockquote>
<br>
The sync is done very frequently. Actually way too
often in my view.<br>
</blockquote>
<blockquote cite="mid:5530AB92.2050509@oracle.com"
type="cite"> <br>
I would prefer that you investigate the hook in
the MXBean change so we know how much work that
would be. </blockquote>
<br>
It was my first intention, but I haven't found a way
how to implement it.<br>
</blockquote>
<br>
I think what you need to do is to extend the Flag
class with an update() method and call the method in
WriteableFlags::set_flag().<br>
<br>
<blockquote cite="mid:55310FCF.5030109@oracle.com"
type="cite"> <br>
<blockquote cite="mid:5530AB92.2050509@oracle.com"
type="cite">If that is not possible I think a
better solution is to call G1Log::update_level()
*once* at the start of a GC. That way the log
level is always consistent throughout the GC.<br>
</blockquote>
<br>
Yes, this is a good idea!<br>
</blockquote>
<br>
Yes, it is much cleaner.<br>
<br>
<blockquote cite="mid:55310FCF.5030109@oracle.com"
type="cite"> <br>
<blockquote cite="mid:5530AB92.2050509@oracle.com"
type="cite"> <br>
<blockquote cite="mid:552FBAF5.1060908@oracle.com"
type="cite"> <br>
<blockquote
cite="mid:552FAE22.4020009@oracle.com"
type="cite"> <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>
</blockquote>
<br>
Yes, I'm aware of unified logging and expect
that G1Log class will go away someday.<br>
But I would like to be able to disable logging
in JDK9, to not see GC events occurred at the
certain period of time.<br>
</blockquote>
<br>
Can you explain more why this is important to fix
right now? Unified logging is targeted for JDK 9.<br>
</blockquote>
<br>
What I heard about unified logging is that there is
no guarantee that it will happen in JDK9 time frame.<br>
I need the feature now to develop tests based on
analysis of logs: I want to start logging not from
begging, but after some steps.<br>
<br>
Will you be okay if G1Log::update_level() is invoked
once per GC?<br>
</blockquote>
<br>
I would be ok with it, but I'm pretty convinced
unified logging will make it in to JDK9 so if you can
wait for it that might be better.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:55310FCF.5030109@oracle.com"
type="cite"> <br>
Thanks,<br>
Dima<br>
<br>
<blockquote cite="mid:5530AB92.2050509@oracle.com"
type="cite"> <br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:552FBAF5.1060908@oracle.com"
type="cite"> <br>
Thanks,<br>
Dima<br>
<br>
<br>
<blockquote
cite="mid:552FAE22.4020009@oracle.com"
type="cite"> <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>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>