RFR: 7164841: Improvements to the GC log file rotation

Yumin Qi yumin.qi at oracle.com
Wed Sep 4 13:02:26 PDT 2013


Thanks. I will use the name you came up with: dump_loggc_header as a 
member function.

Yumin
On 9/4/2013 10:30 AM, Lois Foltan wrote:
>
> On 9/4/2013 12:27 PM, Yumin Qi wrote:
>> Thanks, Lois
>>
>>> arguments.cpp -
>>>     - line # 1867, please add more information on the context in 
>>> which this function can be called
>>>
>> I change the comments to:
>>
>> // This function is called for -Xloggc:<filename>, it can be used
>> // to check if a given file name(or string) conform to following
>> // request:
>> // A valid string only contains "[A-Z][a-z][0-9].-_%[p|t]"
>> // %p and %t only allowed once.
>>
>> Is this OK?
>
> Hi Yumin,
> Yes, thank you, just a nit, instead of "conform to following request:" 
> clearer to state "conforms to the following specification:"
>
>>>     - line #  1868, // A valid file name only contains 
>>> "[A-Z][a-z][0-9].-_%[p|t]"
>>>       I really like the feature that allows a user to tailor the log 
>>> file name to include or not include the pid and timestamp.
>>>       I think this is a big improvement.  However, this does 
>>> represent a change over current accepted behavior.  By accepting
>>>       only "[A-Z][a-z][0-9] and "." in the file name, a user, who 
>>> once specified -Xloggc:foo%.log for example, will now receive
>>>       an error.  Or for that matter any special character like ?, *, 
>>> +.   So, I don't know if this is change in behavior is
>>>       allowable.
>>>
>> I think we can constrain the special chars for file name usage here 
>> --- there will be problem when user upgrade to new version which 
>> contains this changeset so they need to change their scripts to 
>> follow the requirement in this change.
>>
>> Wish to get more input for this change.
>>> ostream.hpp - looks good, no comments
>>>
>>> ostream.cpp -
>>>     - Remove extra lines (#372 & #446) around extend_file_name() 
>>> function
>>>
>> will
>>>    - It would be good to have a couple more comments in 
>>> extend_file_name(),
>>>      like for example, between line #384 and #385 something like:
>>>             // No user specified request for pid & timestamp in 
>>> -Xloggc file name.
>>>      Between 431 & 432, maybe // Only requested pid
>>>      Between 438 & 439, maybe // Only requested timestamp
>>>
>> Will add
>>>    - In the situation where NumberOfGCLogFiles > 1, how does the 
>>> very first file opened
>>>      in the rotation obtain the additional GC log file header info 
>>> you have added?  The first
>>>      file is opened in the constructor so does not have the benefit 
>>> of being opened at line
>>>      #647 and then having the header info written to it in line 
>>> #658-663.
>>>
>>>     - I think the additional GC log file header info would be 
>>> beneficial to a user even in the situation
>>>       where no UseGCLogFileRotation options are specified on the 
>>> command line.  A user may
>>>       be confused why when just -Xloggc:<filename> is specified no 
>>> additional header info is provided.
>>>       One possible solution to this and the previous comment is to 
>>> add similar code as #658-663 to the
>>>       constructor.
>>>
>>>     - In rotatingFileStream::rotate_log(), I still have questions 
>>> about the error situation where the next file
>>>       in the rotation can not be opened.  The current file was 
>>> closed at line #619, UseGCLogFileRotation is
>>>       turned off at line #675, so how will e current file be 
>>> reopened to accommodate for the next write in
>>>       rotatingFileStream::write()?
>>>
>> Good catch.  I thought of this, and think rotation will override that 
>> info. Will change to add this when the first time file is created for 
>> both non-rotate and rotate settings.
> My original suggestion of just duplicating lines #658-663 in the 
> constructor can be improved upon.  I think it would be worthwhile to 
> consider adding a function named something like, dump_xloggc_header(), 
> for example.  This allows for future additional pertinent information 
> to be included in the header very easily without having to visit all 
> the different places where the header info is generated.
>
> Thanks,
> Lois
>>
>> Thanks
>> Yumin
>>
>>>
>>> Thanks,
>>> Lois
>>>
>>> On 8/31/2013 2:36 AM, Yumin Qi wrote:
>>>> Hi, all
>>>>
>>>>
>>>>   With more feedback on the second version, the third version make 
>>>> following changes:
>>>>
>>>>   1) take parametrized filename after -Xloggc:, the filename will 
>>>> be forced to follow following rules:
>>>>       can only contain [A-Z][a-z][0-9].-_%[p|t], any other 
>>>> character used for file name will be rejected. %p or %t can only 
>>>> appear once in file name.
>>>>       example:  -Xloggc:test.log-%t-%p           OK
>>>>       example: -Xloggc:test.log-%p-%p           FAIL
>>>>
>>>>    2) removed unused rotatingFileStream constructors
>>>>
>>>>    3) log more information at head of rotation file:
>>>>        vm version, os version, build id etc
>>>>        memory usage
>>>>        commandline  flags
>>>>
>>>>    Tested on Linux/Windows
>>>>
>>>>    URL: http://cr.openjdk.java.net/~minqi/7164841/webrev0 
>>>> <http://cr.openjdk.java.net/%7Eminqi/7164841/webrev0>2
>>>>
>>>>   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: http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20130904/5ccdcaac/attachment-0001.html 


More information about the hotspot-gc-dev mailing list