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

[Marcus Larsson]

Hi,

On 03/11/2016 05:40 PM, Gerard Ziemski wrote:
> hi Marcus,
>
> I’m still going through the webrev, but I thought I would report my feedback so far.

Thanks for looking at this!

>
> ------------------------------------------------
> #1 File src/os/posix/vm/os_posix.cpp
>
> How about instead of:
>
> +int os::compare_file_modified_times(const char* file1, const char* file2) {
> +  struct stat st[2];
> +  struct timespec filetime[2];
> +
> +  for (int i = 0; i < 2; i++) {
> +    const char* file = (i == 0 ? file1 : file2);
> +    int ret = os::stat(file, &st[i]);
> +    assert(ret == 0, "failed to stat() file '%s': %s", file, strerror(errno));
> +#ifdef __APPLE__
> +    filetime[i] = st[i].st_mtimespec;
> +#else
> +    filetime[i] = st[i].st_mtim;
> +#endif
> +  }
> +
> +  int diff = filetime[0].tv_sec - filetime[1].tv_sec;
> +  if (diff == 0) {
> +    return filetime[0].tv_nsec - filetime[1].tv_nsec;
> +  }
> +  return diff;
> +}
>
> we have something like this, which doesn’t use arrays or a loop mimicking inline function:
>
> static inline struct timespec get_timespec(const char* file) {
>    struct stat st;
>    int ret = stat(file, &st);
>    assert(ret == 0, "failed to stat() file '%s': %s", file, strerror(errno));
>    #ifdef __APPLE__
>      return st.st_mtimespec;
>    #else
>      return st.st_mtim;
>    #endif
> }
>
> int compare_file_modified_times(const char* file1, const char* file2) {
>    struct timespec filetime1 = get_timespec(file1);
>    struct timespec filetime2 = get_timespec(file2);
>    int diff = filetime1.tv_sec - filetime2.tv_sec;
>    if (diff == 0) {
>       return filetime1.tv_nsec - filetime2.tv_nsec;
>    }
>    return diff;
> }
>
> Similarly we should use static inline function instead of a loop in src/os/windows/vm/os_windows.cpp

Good suggestions, will update.

>
> -----------------------------------------------
> #2 File src/share/vm/logging/log.cpp
>
> Doesn’t this function produce an error or at least a warning on a product build without asserts?
>
> +static void delete_file(const char* filename) {
> +  if (!file_exists(filename)) {
> +    return;
> +  }
> +  int ret = remove(filename);
> +  assert(ret == 0, "failed to remove file '%s': %s", filename, strerror(errno));
> +}

I haven't run in to any problems with this. I'm guessing I should've 
failed to build in JPRT if this was a problem, given that we treat 
warnings as errors.

>
> -----------------------------------------------
> #3 File src/share/vm/logging/log.cpp
>
> The number_of_lines_with_substring_in_file() function will not count the substrings that happen to lay across the boundary at sizeof(buf). For example with:
>
>   char buf[16];
>
> and file consisting of “12345678901234gerard1234567890” it will return 0 for number_of_lines_with_substring_in_file(file, “gerard")

Yeah I'm aware of this limitation. It's only used in the test though, 
where I'm hoping it won't be a problem. Would a comment about this be 
enough?

>
> -----------------------------------------------
> #4 File src/share/vm/logging/log.cpp
>
> Should we clarify that we are rounding the results of log10 down by explicitly using floor and explicitly casting it to uint?
>
> static uint number_of_digits_new(uint number) {
>    double d = static_cast<double>(number);
>    uint res = 1 + static_cast<uint>(floor(log10(d)));
>    return res;
> }
>
> cheers

Seems like a good idea to me.

Thanks,
Marcus

>
>
>> On Mar 11, 2016, at 8:21 AM, Marcus Larsson <marcus.larsson at oracle.com> wrote:
>>
>> Third time's the charm.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~mlarsson/8146879/webrev.03/
>>
>> 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