On Tue, 13 Aug 2024 15:07:17 GMT, Sonia Zaldana Calles <szald...@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 I think this could be very useful, but it needs more preparation and decisions. Possibly a CSR. - copy_expand_xxx is used in many places. While I think all of these places would benefit from more expansions than just %p, there is a potential backward compatibility issue if clients use %t for whatever reason today - Do we want the time of the dump or the JVM start? If the JVM runs for a week, then produces a JFR file, should the file be named by the JVM start date? I think in most cases the *current* time makes more sense - Do we want the printout as a human-readable date or as a numeric timestamp? Both makes sense depending on the post-processing clients want to do. - Do we want to improve this function further, potentially adding more replacement options? One possible way to solve this: - use different characters for timestamp (number) and datetime (human readable date) - use always the current time - If we want to add further replacements: - come up with a new replacement character that does not clash with libc sprintf (IMHO using percent was not a good idea in the first place). E.g. `$` - Add a new switch to guard this new replacement logic. By default off. If on, the contract is that any character following a `$` may be either now or in the future replaced with something different. Client must not use `$` as a normal character. - We probably should remove all non-matching `$` from the input. - The first replacements could be: `$p` for pid, `$t` for timestamp (numeric), `$d` for datetime - later replacements can be added later. Since we guard the new feature with a switch and forbid the use of `$`, we are then free to do so without breaking backward compatibility. I would like to hear @dholmes-ora take on this. We had a similar system at SAP in our proprietary JVM, which was really useful, so I like this idea in general. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20568#issuecomment-2301641288
