hi Marcus,
I’m still going through the webrev, but I thought I would report my feedback so
far.
------------------------------------------------
#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
-----------------------------------------------
#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));
+}
-----------------------------------------------
#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")
-----------------------------------------------
#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
> On Mar 11, 2016, at 8:21 AM, Marcus Larsson <[email protected]> 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
>>>
>>
>
