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.lars...@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
