JDK-7090324: gclog rotation via external tool

[Yasumasa Suenaga]

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