RFR: 8146879: Add option for handling existing log files in UL

[Dmitry Samersoff]

Marcus,

1. os_posix/os_windows.cpp

Windows version is expensive and change access time of a log file.

So it's better to create os::get_mtime() that uses stat() for both,
windows and posix and LogFileOutput::calc_mtime_diff().

Also it saves a bit of compiler power unrolling loop.

2. log.cpp

size_t number_of_lines_with_substring_in_file()

When you plan to use it ? Current implementation is rather expensive,
but if you plan to run it manually only, I'm OK with it.

3. logFileOutput.cpp

82.
Please, change file_exists to can_be_rotated() and check that log output
is a *regular file* and you have a write access to it.

87.
Please, don't use floating point math. Just allocate 10 extra bytes, it
fit all possible file length value.

93.
if we have log.01, log.03 etc this function creates log.02 instead of
log.04.

Sorry! I'm not follow logic of ll.118
What happens if some log parsing tool change log file mtime?

Please, limit the number of old logs (FileCountOption) to some
reasonable value (e.g. 999) and use binary search to find the name of a
next file.

Also please check full log file name against MAX_PATH constant.

-Dmitry




On 2016-03-11 17:39, Marcus Larsson wrote:
> Hi,
> 
> On 2016-03-11 15:35, Bengt Rutisson wrote:
>>
>> Hi Marcus,
>>
>> On 2016-03-11 15:21, Marcus Larsson wrote:
>>> Third time's the charm.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~mlarsson/8146879/webrev.03/
>>
>> I had a quick look at the code changes. It is not really my area of
>> the code, so I'll leave to someone else to formally review it.
>>
>> However, I downloaded the patch a played a bit with the logging. This
>> is much more like the way I would like it! Thanks!
>>
>> So, from a functional perspective this looks good to me.
>>
> 
> Thanks for the feedback!
> 
> Marcus
> 
>> Thanks,
>> Bengt
>>
>>>
>>> This patch makes log file rotation the default. Default thresholds
>>> are 5 rotated files with a target size of 20MiB. Truncating behavior
>>> can be achieved by setting filecount to 0
>>> (-Xlog::myfile.log::filecount=0).
>>>
>>> If a log file already exists during log file initialization it will
>>> be rotated. If any of the target file names (file.0 to file.4 in the
>>> default case) are available, that filename will be used for the
>>> existing log. If all names are taken the VM will attempt to overwrite
>>> the oldest file.
>>>
>>> This should prevent unlimited log file creations and avoid accidental
>>> loss of log files from previous runs. The default thresholds (5
>>> files, 20MiB each) is just a suggestion. If you think it should be
>>> higher/lower let me know.
>>>
>>> Tested with included internal VM tests through RBT.
>>>
>>> Thanks,
>>> Marcus
>>>
>>> On 2016-03-01 15:05, Marcus Larsson wrote:
>>>> Hi,
>>>>
>>>> After some offline discussions I'm withdrawing this patch. I will
>>>> instead investigate if I can achieve similar behavior using log
>>>> rotation as the default.
>>>>
>>>> Thanks,
>>>> Marcus
>>>>
>>>> On 03/01/2016 12:11 PM, Marcus Larsson wrote:
>>>>> Hi again,
>>>>>
>>>>> Taking a different approach to this.
>>>>>
>>>>> New webrev:
>>>>> http://cr.openjdk.java.net/~mlarsson/8146879/webrev.01/
>>>>>
>>>>> Existing files will now by default be renamed/archived with a .X
>>>>> suffix where X is the lowest number such that the resulting file
>>>>> name is available (jvm.log becomes jvm.log.0). A mode option for
>>>>> controlling this behavior has been added as well. It can be set to
>>>>> archive, append, or truncate (i.e. -Xlog::jvm.log::mode=truncate).
>>>>>
>>>>> Tested with included jtreg test through JPRT.
>>>>>
>>>>> Thanks,
>>>>> Marcus
>>>>>
>>>>> On 01/14/2016 04:00 PM, Marcus Larsson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review the following patch to make sure UL truncates
>>>>>> existing log files before writing to them. Since files are opened
>>>>>> in append mode, truncation isn't done automatically, so instead
>>>>>> the patch adds an attempt to remove the log file before opening it.
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~mlarsson/8146879/webrev.00/
>>>>>>
>>>>>> Issue:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8146879
>>>>>>
>>>>>> Testing:
>>>>>> Included test through JPRT
>>>>>>
>>>>>> Thanks,
>>>>>> Marcus
>>>>>
>>>>
>>>
>>
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.