<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<font face="monospace">Hi Yasumasa,<br>
<br>
(have to use HTML email to get a width of more than 78 chars,
sorry)<br>
<br>
why did you change the code in arguments.cpp in the method
check_gc_log_consistency?<br>
<br>
Next, the gcLogFileStream::rotate_log method now does a lot of
things. Could you separate out the first block into a new method,
gcLogFileStream::should_rotate(bool force)?<br>
<br>
This was, the code would read:<br>
<br>
> bool gcLogFileStream::should_rotate(bool force) {<br>
> return force || _bytes_writen >= GCLogFileSize;<br>
> }<br>
><br>
> void gcLogFileStream::rotate_log(bool force) {<br>
> char time_msg[FILENAMEBUFLEN];<br>
> char time_str[EXTRACHARLEN];<br>
> char current_file_name[FILENAMEBUFLEN];<br>
> char renamed_file_name[FILENAMEBUFLEN];<br>
><br>
> if (!should_rotate(force)) {<br>
> return;<br>
> }<br>
><br>
> ...<br>
> }<br>
<br>
Could you please update your patch?<br>
<br>
There is a new empty line in the rotate_log method:<br>
<br>
> }<br>
> +<br>
> #ifdef ASSERT<br>
<br>
could you please remove it?<br>
<br>
The logging change in rotate_log uses a different kind of if/else
syntax than the rest of the file:<br>
<br>
> if (force) {<br>
> ...<br>
> }<br>
> else {<br>
> ...<br>
> }<br>
<br>
The other if/else statements in the file uses:<br>
<br>
> if (cond) {<br>
> ...<br>
> } else {<br>
> ...<br>
> }<br>
<br>
Could you please update your change to use the same if/else
syntax?<br>
<br>
This part of the change duplicates the code:<br>
<br>
+ jio_snprintf(time_msg, sizeof(time_msg), "%s GC log
rotation request has been received. Saved as %s\n",<br>
+ os::local_time_string((char
*)time_str, sizeof(time_str)),<br>
+ renamed_file_name);<br>
+ }<br>
+ else {<br>
+ jio_snprintf(time_msg, sizeof(time_msg), "%s GC log file
has reached the"<br>
" maximum size. Saved as %s\n",<br>
- os::local_time_string((char
*)time_str, sizeof(time_str)),<br>
+ os::local_time_string((char
*)time_str, sizeof(time_str)),<br>
renamed_file_name);<br>
<br>
Could you instead just change the message, as in:<br>
<br>
> const char* msg = forced ? </font><font face="monospace"><font
face="monospace">"%s GC log rotation request has been received.
Saved as %s\n" :</font></font><font face="monospace"><font
face="monospace"><font face="monospace"><br>
> "%s GC log file has reached
the maximum size. Saved as %s\n";<br>
> jio_snprintf(msg, </font></font></font><font
face="monospace"><font face="monospace"><font face="monospace"><font
face="monospace">os::local_time_string((char *)time_str,
sizeof(time_str)), renamed_file_name);<br>
<br>
</font></font></font>The declaration of rotate_log in
ostream.hpp still uses the old variable name is_force, it should
use force, just as the definition.</font><br>
<br>
<font face="monospace">Finally, could you add a test that tests your
change? Have a look at the other tests in hotspot/test/gc to see
how you can do it<br>
(you might want to use some functionality from
hotspot/test/testlibrary).<br>
<br>
Thanks,<br>
Erik<br>
</font><br>
<div class="moz-cite-prefix">On 2014-01-29 15:28, Yasumasa Suenaga
wrote:<br>
</div>
<blockquote cite="mid:52E91000.9010600@ysfactory.dip.jp" type="cite">Hi
Staffan,
<br>
<br>
Thank you for reviewing!
<br>
I've uploaded new webrev.
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.01/">http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.01/</a>
<br>
<br>
On 2014/01/29 20:56, Staffan Larsen wrote:
<br>
<blockquote type="cite">Yasumasa,
<br>
<br>
src/share/vm/runtime/arguments.cpp
<br>
no comments
<br>
<br>
src/share/vm/runtime/safepoint.cpp
<br>
I was surprised that gc log size was checked after each safe
point. That seems an uneccssary burden to place on a safe point.
Instead we should switch to a periodic task that checks the gc
log size. However, this is unrelated to you patch, so please
ignore for now.
<br>
</blockquote>
<br>
Agree.
<br>
However, I think that PeriodicTask also is not appropriate for
this.
<br>
<br>
Size of GC log file is increased when GC is occurred.
<br>
So I think rotate function should be called at entry of each GC
events
<br>
e.g. VM_GC_Operation::doit_prologue() etc...
<br>
<br>
<br>
<blockquote type="cite">src/share/vm/runtime/vm_operations.hpp
<br>
line 402: nit: missing space before {
<br>
</blockquote>
<br>
Fixed.
<br>
<br>
<br>
<blockquote type="cite">line 405: I think ‘force’ is a better name
than ‘is_force’
<br>
</blockquote>
<br>
I removed "force" option from DCmd.
<br>
So I removed this from VMOperation.
<br>
<br>
<br>
<blockquote type="cite">src/share/vm/services/diagnosticCommand.cpp
<br>
line 666: What does this do without the -force option? It looks
to me that the non-force case will happen after each safe point
(see above) and that there is no need to ever do this from a
diagnostic command. Can we remove the option?
<br>
</blockquote>
<br>
Indeed.
<br>
I removed "force" option.
<br>
<br>
<br>
<blockquote type="cite">line 677: “Target VM does not support GC
log file rotation."
<br>
</blockquote>
<br>
Fixed.
<br>
<br>
<br>
<blockquote type="cite">nits: some missing spaces before ‘{' and
after ‘if'
<br>
</blockquote>
<br>
Fixed.
<br>
<br>
<br>
<blockquote type="cite">src/share/vm/services/diagnosticCommand.hpp
<br>
I think RotateGCLogDCmd should require the “control” permission
when executed via JMX, so please add:
<br>
static const JavaPermission permission() {
<br>
JavaPermission p =
{"java.lang.management.ManagementPermission",
<br>
"control", NULL};
<br>
return p;
<br>
}
<br>
</blockquote>
<br>
Added.
<br>
<br>
<br>
<blockquote type="cite">line 394: Maybe “Force the GC log file to
be rotated.” is a better description?
<br>
</blockquote>
<br>
Fixed.
<br>
<br>
<br>
<blockquote type="cite">src/share/vm/utilities/ostream.cpp
<br>
line 662: I think ‘force’ is a better name than ‘is_force’
<br>
line 668: The comment says exactly the same thing as the code so
I think it can be skipped
<br>
line 671: “GC log file rotation occurs by external trigger
ONLY."
<br>
line 675: "not need” -> “no need”
<br>
line 718: "GC log rotation request has been received”
<br>
</blockquote>
<br>
Fixed them.
<br>
<br>
<br>
Thanks,
<br>
<br>
Yasumasa
<br>
<br>
<br>
<blockquote type="cite">src/share/vm/utilities/ostream.hpp
<br>
no comments
<br>
<br>
<br>
Thanks,
<br>
/Staffan
<br>
<br>
On 24 jan 2014, at 14:50, Yasumasa
Suenaga<a class="moz-txt-link-rfc2396E" href="mailto:yasu@ysfactory.dip.jp"><yasu@ysfactory.dip.jp></a> wrote:
<br>
<br>
<blockquote type="cite">Hi all,
<br>
<br>
I've created webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.00/">http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.00/</a>
<br>
<br>
This patch works fine on current jdk9/hs-rt .
<br>
Could you review this?
<br>
<br>
<br>
I am just an Author. So I need a sponsor.
<br>
Could you help me?
<br>
<br>
<br>
Please cooperate.
<br>
<br>
<br>
Thanks,
<br>
<br>
Yasumasa
<br>
<br>
<br>
On 2013/12/06 0:05, Yasumasa Suenaga wrote:
<br>
<blockquote type="cite">Hi all,
<br>
<br>
Did someone read my email?
<br>
I really hope to merge "JDK-7090324: gclog rotation via
external tool" .
<br>
<br>
I hear that someone need this RFE. So I want to discuss
about this.
<br>
<br>
<br>
Thanks,
<br>
<br>
Yasumasa
<br>
<br>
On 2013/11/08 21:47, Yasumasa Suenaga wrote:
<br>
<blockquote type="cite">Hi all,
<br>
<br>
Did someone read my mail?
<br>
<br>
I think that this RFE helps us to watch Java heap on
production system.
<br>
Also I think this RFE is able to be part of the JEP 158
(Unified JVM Logging) .
<br>
<br>
I want to update this RFE in JDK Bug System, but I don't
have account.
<br>
So I've posted email at first.
<br>
<br>
<br>
Thanks,
<br>
<br>
Yasumasa
<br>
<br>
<br>
On 2013/09/30 21:10, Yasumasa Suenaga wrote:
<br>
<blockquote type="cite">In previous email, I've attached
new patch for this RFE.
<br>
It works fine with current hsx.
<br>
<br>
<br>
Yasumasa
<br>
<br>
On 2013/09/29 23:40, Yasu wrote:
<br>
<blockquote type="cite">Hi all,
<br>
<br>
We are using "logrotate" tool on RHEL for various log
rotation.
<br>
Current HotSpot has gclog rotation function for log
size base,
<br>
however I need to rotate gc log synchronizing with
logrotate tool.
<br>
<br>
So I've created RFE as "JDK-7090324: gclog rotation
via external tool" .
<br>
And Sr. Engineering Manager in Oracle said he use the
essence of my patch in one
<br>
of the jcmd subcommands.
<br>
<a class="moz-txt-link-freetext" href="http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2011-September/003274.html">http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2011-September/003274.html</a>
<br>
<br>
2 years ago, I posted a patch for this RFE.
<br>
But this patch is too old to apply for current
HotSpot.
<br>
<br>
In last month, a similar discussion was appeared in
ML.
<br>
So I think it's time to discuss this RFE.
<br>
<a class="moz-txt-link-freetext" href="http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2013-August/008029.html">http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2013-August/008029.html</a>
<br>
<br>
<br>
Please cooperate.
<br>
<br>
Best regards,
<br>
Yasumasa
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>