RFR: 7164841: Improvements to the GC log file rotation

Tao Mao tao.mao at oracle.com
Fri Aug 23 23:46:28 UTC 2013


Big thanks again for improving this.

Tao

On 8/23/13 4:05 PM, Yumin Qi wrote:
> Tao,
>
>   Thanks for your review.
>
> On 8/23/2013 3:33 PM, Tao Mao wrote:
>> Hi,
>>
>> I reviewed most of the code and test-ran a build from it. It's a very 
>> cool and important improvement.
>>
>> Thank you for putting together these on our wishlist for long.
>>
>> Below are some comments.
>>
>> 1. src/share/vm/runtime/arguments.cpp
>>
>> - 1853 "To enable GC log rotation, use -Xloggc:<filename> 
>> -XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=<num_of_files> 
>> -XX:GCLogFileSize=<num_of_size>[k|K|m|M]\n"
>> + 1853 "To enable GC log rotation, use -Xloggc:<filename> 
>> -XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=<num_of_files> 
>> -XX:GCLogFileSize=<num_of_size>[k|K|m|M|g|G]\n"
>>
>> Please consider adding [g|G] to GCLogFileSize suggestion.
>>
>> I worked on a problem of enabling gc log limit over 2G (JDK-7122222). 
>> So it seems that customers sometimes want gc logs to be very large.
>>
> Sure, will add.
Thank you.
>> 2. src/share/vm/runtime/arguments.hpp
>>
>> (1) with the current changeset, we only append date&time to file_name 
>> w/ +UseGCLogFileRotation; if not, we won't have date&time info.
>>
>> I think it would be useful to let both cases (w/ and w/o 
>> UseGCLogFileRotation) have date&time in order to solve the 
>> overwritten problem (e.g. JDK-8020667). In fact, having that, we 
>> actually solve JDK-8020667.
>>
>> If you want to have that, basically you need to work on the 
>> FileStream constructor methods fileStream().
>>
> I can change that, if no objection from others. This also will 
> simplify the setting of file name here.
Thank you. Let's see if anyone disagrees with a good reason.
>
>> (2) Would it be better to rename method name reformat_filename() to 
>> extend_filename()?
>>
>> (3) Typos and suggestion
>> 537 // rotate file in names filename.0, filename.1, ..., 
>> filename.<NumberOfGCLogFiles - 1>
>> *=> extended_filename.0*
>>
>> 538 // filename contains pid and time when the fist file created. The 
>> current filename is
>> *=> *the*first *file created.
>>
>> 539 // gc_log_file_name + pid<pid> + YYYY-MM-DD_HH-MM-SS.<i>.current, 
>> where i is current
>> 540 // rotation file number. After it reaches max file size, the file 
>> will be saved and
>> 541 // renamed with .current removed from its tail.
>>
> Will change that.
Thank you.
>> 3. For your point 5), I don't quite get it. In my test-run, I tried 
>> to change file permission to unreadable and unwritable, but VM would 
>> later change the permission back anyway.
>>
>> So could you bring up some use cases of that functionality to give 
>> more details?
>>
> Changing file permission will not stop the file create, in this 
> rotation, the file opened/saved/removed/renamed -> then repeat. What I 
> have done is change the while directory to read only, then the create 
> failed so rotation stopped.
>
So, when VM is already running and gc logs are rotating, how should I 
make the rotation stop? Let me give one more try.
>> 4. Will you write jtreg tests for this functionality? It looks 
>> possible to write some tests, at least checking the format of log names.
>>
> Good suggestion, I will add one.
You may also want to check the number of gc logs created is what we 
desire to have, when the Java process is completed.

The example in the link 
(http://stackoverflow.com/questions/5694385/getting-the-filenames-of-all-files-in-a-folder) 
basically describes a routine to list all filenames in a directory. Hope 
that helps.
>
>> Thanks.
>> Tao
>>
>> On 8/23/13 7:53 AM, Yumin Qi wrote:
>>> Could I get  a GC staff review the change? Need more reviewers to 
>>> push this in.
>>>
>>> Thanks
>>> Yumin
>>>
>>> On 8/21/2013 3:43 PM, Yumin Qi wrote:
>>>> Hi, all
>>>>
>>>>   This is second version after feedback from first round.
>>>>   The changes are:
>>>>
>>>>   1) file name will be based on gc log file name 
>>>> (-Xloggc:filename), pid (process id) and time when the first 
>>>> rotation file created:
>>>> <filename>-pid<pid>-YYYY-MM-DD_HH-MM-SS
>>>>   2) If rotate in same file, file name is as in 1), if rotate in 
>>>> multiple files, .<i> will append to above file name.
>>>>   3) every time file rotated, file name and time when file created 
>>>> will be logged to head/tail, this is same as first version.
>>>>   4) current file has name 
>>>> <filename>-pid<pid>-YYYY-MM-DD_HH-MM-SS.<i>.current
>>>>        This is similar to first version.
>>>>        By adapting such name format we will never loss logs in case 
>>>> apps stops and restart, the log files will not be overwritten since 
>>>> time stamp in file names.
>>>>    5) If open file failed, turn off gc log rotation.
>>>>         If due to some reason, file operation failed (such as 
>>>> permission changed etc), with log file opened, logging still works, 
>>>> but at
>>>>         saving and renaming, the file operation will fail, this 
>>>> will lead not all files saved.
>>>>
>>>> http://cr.openjdk.java.net/~minqi/7164841/webrev01
>>>>
>>>>      Tested with jtreg, JPRT.
>>>>
>>>> Thanks
>>>> Yumin
>>>>
>>>> On 8/15/2013 8:35 AM, Yumin Qi wrote:
>>>>> Hi,
>>>>>
>>>>>   Can I have your review for this small changes?
>>>>> http://cr.openjdk.java.net/~minqi/7164841/webrev00/ 
>>>>> <http://cr.openjdk.java.net/%7Eminqi/7164841/webrev00/>
>>>>>
>>>>>    This is for a enhancement to add head/tail message to the 
>>>>> logging files to assist reading GC output.
>>>>>    1. modified prompt message if invalid arguments used for log 
>>>>> rotating;
>>>>>    2. add time and file name message to log file head/tail.
>>>>>    3. for easily identify which log file is current, use file name 
>>>>> like <filename>.n.current, after it reaches maximum size, rename 
>>>>> it to <filename>.n
>>>>>         On Windows, there is no F_OK (existing test) definition, 
>>>>> F_OK is defined as "0" and for _access of VC++, it just describes:
>>>>>
>>>>> modevalue
>>>>>
>>>>> 	
>>>>>
>>>>> Checks file for
>>>>>
>>>>> 00
>>>>>
>>>>> 	
>>>>>
>>>>> Existence only
>>>>>
>>>>> 02
>>>>>
>>>>> 	
>>>>>
>>>>> Write-only
>>>>>
>>>>> 04
>>>>>
>>>>> 	
>>>>>
>>>>> Read-only
>>>>>
>>>>> 06
>>>>>
>>>>> 	
>>>>>
>>>>> Read and write
>>>>>
>>>>>
>>>>> http://msdn.microsoft.com/en-us/library/1w06ktdy.aspx
>>>>> The definition are consistent with unistd.h.
>>>>>
>>>>>     Test: JPRT and jtreg.
>>>>>
>>>>>    Thanks
>>>>>    Yumin
>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130823/b459d8a7/attachment.htm>


More information about the hotspot-gc-dev mailing list