On Tue, 10 Sep 2024 15:42:57 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 > > 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 A few notes, without reviewing the code further today, see if these all make sense and are settled: "%t" expanded to timestamp seems simple enough (I think % is not controversial, extending from our existing use of %p) But yes what is a timestamp? There could be debate on the format. The original issue came from a user request, but it was not fully specified, just a request for an automatic way of putting file creation time in an output filename. So should we have %t and %d ? Or adopt all the decorator options from unified logging? To me those seem extreme: if I just want output files to say when they were made, I don't really need options, or nanoseconds, and would be quite happy with the ostream.cpp style of "%t => YYYY-MM-DD_HH-MM-SS". Sonia your points above: (I haven't looked in detail at how this affects copy_expand_arguments, but if we can settle the jcmd options then that can follow...) 1. -filepattern Would really like to avoid adding a new jcmd option, in all the diagnostic commands that take an output filename, and just go with a filename that respects some substitutions. 2 %t and %p are "unique enough" that we don't need "%TIMESTAMP" ? We don't need to permit creating "my%perfect%test%file" without using "%%" to get a literal % included? 3. -XX:+AllowFileNamePattern I would think at this step, we get our jcmds that take an output FILE to respect the %t timestamp. There may be other XX options that take filenames which could be considered in future. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20568#issuecomment-2449759987
