Hi Erik,
On 2014/01/30 0:13, Erik Helin wrote:
Hi Yasumasa,
(have to use HTML email to get a width of more than 78 chars,
sorry)
why did you change the code in arguments.cpp in the method
check_gc_log_consistency?
In current implementation, check_gclog_consistency() checks three
parameters:
- GC log filename
- NumberOfGCLogFiles
- GCLogFileSize
My customer uses external trigger "ONLY" for rotating logs.
If they want to do that, GCLogFileSize does not need.
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)?
This was, the code would read:
bool gcLogFileStream::should_rotate(bool force) {
return force || _bytes_writen>= GCLogFileSize;
}
void gcLogFileStream::rotate_log(bool force) {
char time_msg[FILENAMEBUFLEN];
char time_str[EXTRACHARLEN];
char current_file_name[FILENAMEBUFLEN];
char renamed_file_name[FILENAMEBUFLEN];
if (!should_rotate(force)) {
return;
}
...
}
Could you please update your patch?
I will do that.
There is a new empty line in the rotate_log method:
}
+
#ifdef ASSERT
could you please remove it?
I will do that.
The logging change in rotate_log uses a different kind of if/else
syntax
than the rest of the file:
if (force) {
...
}
else {
...
}
The other if/else statements in the file uses:
if (cond) {
...
} else {
...
}
Could you please update your change to use the same if/else syntax?
I will do that.
This part of the change duplicates the code:
+ jio_snprintf(time_msg, sizeof(time_msg), "%s GC log rotation
request has been received. Saved as %s\n",
+ os::local_time_string((char *)time_str, sizeof(time_str)),
+ renamed_file_name);
+ }
+ else {
+ jio_snprintf(time_msg, sizeof(time_msg), "%s GC log file has
reached the"
" maximum size. Saved as %s\n",
- os::local_time_string((char *)time_str, sizeof(time_str)),
+ os::local_time_string((char *)time_str, sizeof(time_str)),
renamed_file_name);
Could you instead just change the message, as in:
const char* msg = forced ? "%s GC log rotation request has been
received. Saved as %s\n" :
"%s GC log file has reached the maximum size. Saved as %s\n";
jio_snprintf(msg, os::local_time_string((char *)time_str,
sizeof(time_str)), renamed_file_name);
I will do that.
The declaration of rotate_log in ostream.hpp still uses the old
variable name is_force, it should use force,
just as the definition.
Sorry, I will fix it.
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
(you might want to use some functionality from
hotspot/test/testlibrary).
I found three tests as following:
[ysuenaga@xelvis test]$ find . -iname "*jcmd*"
./runtime/NMT/JcmdWithNMTDisabled.java
./runtime/NMT/JcmdScale.java
./gc/TestG1ZeroPGCTJcmdThreadPrint.java
I understand that these tests checks output (stdout/stderr) with
OutputAnalyzer.
However, my patch affects target VM. So I guess current test cannot
check
that GC log rotation is succeeded.
Should I make test which checks exit value of jcmd ?
Thanks,
Yasumasa
Thanks,
Erik
On 2014-01-29 15:28, Yasumasa Suenaga wrote:
Hi Staffan,
Thank you for reviewing!
I've uploaded new webrev.
http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.01/
On 2014/01/29 20:56, Staffan Larsen wrote:
Yasumasa,
src/share/vm/runtime/arguments.cpp
no comments
src/share/vm/runtime/safepoint.cpp
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.
Agree.
However, I think that PeriodicTask also is not appropriate for
this.
Size of GC log file is increased when GC is occurred.
So I think rotate function should be called at entry of each GC
events
e.g. VM_GC_Operation::doit_prologue() etc...
src/share/vm/runtime/vm_operations.hpp
line 402: nit: missing space before {
Fixed.
line 405: I think ‘force’ is a better name than ‘is_force’
I removed "force" option from DCmd.
So I removed this from VMOperation.
src/share/vm/services/diagnosticCommand.cpp
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?
Indeed.
I removed "force" option.
line 677: “Target VM does not support GC log file rotation."
Fixed.
nits: some missing spaces before ‘{' and after ‘if'
Fixed.
src/share/vm/services/diagnosticCommand.hpp
I think RotateGCLogDCmd should require the “control” permission
when executed via JMX, so please add:
static const JavaPermission permission() {
JavaPermission p = {"java.lang.management.ManagementPermission",
"control", NULL};
return p;
}
Added.
line 394: Maybe “Force the GC log file to be rotated.” is a
better description?
Fixed.
src/share/vm/utilities/ostream.cpp
line 662: I think ‘force’ is a better name than ‘is_force’
line 668: The comment says exactly the same thing as the code so
I think it can be skipped
line 671: “GC log file rotation occurs by external trigger ONLY."
line 675: "not need” -> “no need”
line 718: "GC log rotation request has been received”
Fixed them.
Thanks,
Yasumasa
src/share/vm/utilities/ostream.hpp
no comments
Thanks,
/Staffan
On 24 jan 2014, at 14:50, Yasumasa
Suenaga<y...@ysfactory.dip.jp> wrote:
Hi all,
I've created webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.00/
This patch works fine on current jdk9/hs-rt .
Could you review this?
I am just an Author. So I need a sponsor.
Could you help me?
Please cooperate.
Thanks,
Yasumasa
On 2013/12/06 0:05, Yasumasa Suenaga wrote:
Hi all,
Did someone read my email?
I really hope to merge "JDK-7090324: gclog rotation via
external tool" .
I hear that someone need this RFE. So I want to discuss about
this.
Thanks,
Yasumasa
On 2013/11/08 21:47, Yasumasa Suenaga wrote:
Hi all,
Did someone read my mail?
I think that this RFE helps us to watch Java heap on
production system.
Also I think this RFE is able to be part of the JEP 158
(Unified JVM Logging) .
I want to update this RFE in JDK Bug System, but I don't have
account.
So I've posted email at first.
Thanks,
Yasumasa
On 2013/09/30 21:10, Yasumasa Suenaga wrote:
In previous email, I've attached new patch for this RFE.
It works fine with current hsx.
Yasumasa
On 2013/09/29 23:40, Yasu wrote:
Hi all,
We are using "logrotate" tool on RHEL for various log
rotation.
Current HotSpot has gclog rotation function for log size
base,
however I need to rotate gc log synchronizing with logrotate
tool.
So I've created RFE as "JDK-7090324: gclog rotation via
external tool" .
And Sr. Engineering Manager in Oracle said he use the
essence of my patch in one
of the jcmd subcommands.
http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2011-September/003274.html
2 years ago, I posted a patch for this RFE.
But this patch is too old to apply for current HotSpot.
In last month, a similar discussion was appeared in ML.
So I think it's time to discuss this RFE.
http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2013-August/008029.html
Please cooperate.
Best regards,
Yasumasa
