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
