JDK-7090324: gclog rotation via external tool

[Yasumasa Suenaga]

Hi Erik, Staffan,

I've uploaded new webrev. Could you review this ?
http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.02/

This patch includes fixes from comments of Staffan and Erik.

And I created new test of this patch as Test7090324 .
This test works fine with jtreg.


Thanks,

Yasumasa

On 2014/01/30 0:55, Yasumasa Suenaga wrote:
> 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 at 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<yasu at 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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>