JDK-7090324: gclog rotation via external tool

Staffan Larsen staffan.larsen at oracle.com
Thu Jan 30 00:23:59 PST 2014 

Would it be possible for the Diagnostic Command to output the location of the rotated log? When invoking the command it would be good to get some kind of feedback.

test/gc/7090324/Test7090324.java:
- I think this needs to have the Oracle copyright notice as well.
- Tests should now use descriptive names, not bug numbers: https://wiki.openjdk.java.net/display/HotSpot/Naming+HotSpot+JTReg+Tests
- nits: lots of missing spaces before ‘{‘, and after ‘for’, ‘if’
- line 47: you don’t need to clean up old files, jtreg will give you a fresh scratch directory to run in


/Staffan
 


On 30 jan 2014, at 08:08, Yasumasa Suenaga <suenaga.yasumasa at lab.ntt.co.jp> wrote:

> 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
>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>>

More information about the serviceability-dev mailing list