JDK-7090324: gclog rotation via external tool
Erik Helin
erik.helin at oracle.com
Wed Jan 29 07:13:46 PST 2014
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?
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?
There is a new empty line in the rotate_log method:
> }
> +
> #ifdef ASSERT
could you please remove it?
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?
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);
The declaration of rotate_log in ostream.hpp still uses the old variable
name is_force, it should use force, just as the definition.
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).
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
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140129/ba9f6c77/attachment.html
More information about the serviceability-dev
mailing list