RFR: 8204681: Option to include timestamp in hprof filename [v2]

[Kevin Walls]

On Tue, 10 Sep 2024 15:42:57 GMT, Sonia Zaldana Calles <szaldana at openjdk.org> wrote:

>> Hi all, 
>> 
>> This PR addresses [8204681](https://bugs.openjdk.org/browse/JDK-8204681) enabling support for timestamp expansion in filenames specified in `-XX:HeapDumpPath` using `%t`. 
>> 
>> As mentioned in this comments for this issue, this is somewhat related to [8334492](https://bugs.openjdk.org/browse/JDK-8334492) where we enabled support for `%p` for filenames specified in jcmd. 
>> 
>> With this patch, I propose: 
>> - Expanding the utility function `Arguments::copy_expand_pid` to `Arguments::copy_expand_arguments` to deal with `%p` expansions for pid and `%t` expansions for timestamps. 
>> - Leveraging the above utility function to enable argument expansion for both heap dump filenames and jcmd output commands. 
>> - Though the linked JBS issue only relates to heap dumps generated in case of OOM, I think we can edit it to more broadly support filename expansion to support `%t` for jcmd as well. 
>> 
>> Testing: 
>> - [x] Added test cases pass with all platforms (verified with a GHA job). 
>> - [x] Tier 1 passes with GHA. 
>> 
>> Looking forward to hearing your thoughts!
>> 
>> Thanks, 
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
> 
>  - Merge branch 'openjdk:master' into JDK-8204681
>  - 8204681: Option to include timestamp in hprof filename

Thanks Sonia --

Regarding timestamp format:

UL style got you a timestamp like: 2024-10-31T15:14:39.344-0400
make_log_name() would give:        YYYY-MM-DD_HH-MM-SS

Precision vs filename length?  The .001 of a second accuracy might not be that meaningful on a file name.  

I was liking the make_log_name() style above, but it's not really critical which.

But what does Unified Logging do when you log to a file?

file=filename uses %p and %t for pid and STARTUP timestamp.  Hmm.... For usage like heapdumps where you may want multiple, current time is much more useful than startup time.  But hey, it's another vote for the use of %t for timestamp in a filename, and a UnifiedLogging log to a file does not give you control over exactly how you want the timestamp in the filename.  You get what you get.  (The events logged in the file are what matter I suppose (and I agree)).

So what timestamp do you get?

java -Xlog:gc*:file%t.out -version
creates

file2024-11-01_12-01-10.out

..so that looks like the simpler version, like make_log_name()  mentioned earlier.  No milliseconds or time offsets.

I would do that, and forget multi-character patterns unless somebody really wants/needs it.  8-)


------

On changing Arguments::copy_expand_pid to Arguments::copy_expand_arguments

There are a few users, listed below.  I can't see any problem by starting to respect a new % token, but there is always a chance as I think Thomas was suggesting.


If anybody is concerned, we should keep Arguments::copy_expand_pid, and add a new function to do timestamps additionally.


------

Arguments::copy_expand_pid
I find usage by:

perfMemory.cpp:
perdata file is not normally going to have %t in it but -XX:PerfDataSaveFile= could set it.
Users would be really unwise to add specify %t here!  Maybe that means there should still be Arguments::copy_expand_pid which ONLY does %p expansion.


diagnosticArgument.cpp uses it, good, that's what we want.


vmError.cpp:
next_OnError_command uses it.
Having timestamp support in -XX:OnError command strings in addition to %p could be really useful.  Small chance of a clash with existing commands/scripts.


vmError.cpp:
expand_and_open / prepare_log_file /  VMError::report_and_die calls:  prepare_log_file(ErrorFile, "hs_err_pid%p.log",...
 
OK, being able to specify a timestamp in hs_err could be very useful.
Also the compiler replay data file would be affected (again not bad).

Also called by JVMCI::fatal_log() meaning -XX:JVMCINativeLibraryErrorFile= could set a filename using %p and now %t.



jfrEmergencyDump.cpp:
create_emergency_dump_path() uses it, with these patterns, which are not configurable, so not a problem:

44  static const char vm_error_filename_fmt[] = "hs_err_pid%p.jfr";
45  static const char vm_oom_filename_fmt[] = "hs_oom_pid%p.jfr";
46  static const char vm_soe_filename_fmt[] = "hs_soe_pid%p.jfr";


The changes look generally useful.  A release note and maybe other doc updates will be needed (which is fine).

If the impact looks too much, keeping Arguments::copy_expand_pid and creating Arguments::copy_expand_arguments which optionally does timestamp expansion, will mean the other changes (hs_err, replay, etc..) can be made separately.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/20568#issuecomment-2451776538