RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v14]

[Kevin Walls]

On Thu, 25 Jul 2024 15:31:05 GMT, Sonia Zaldana Calles <szaldana at openjdk.org> wrote:

>> Hi all, 
>> 
>> This PR addresses [8334492](https://bugs.openjdk.org/browse/JDK-8334492) enabling jcmd diagnostic commands that issue an output file to accept the `%p` pattern in the file name and substitute it for the PID. 
>> 
>> This PR addresses the following diagnostic commands: 
>> - [x] Compiler.perfmap 
>> - [x] GC.heap_dump
>> - [x] System.dump_map
>> - [x] Thread.dump_to_file
>> - [x] VM.cds
>> 
>> Note that some jcmd diagnostic commands already enable this functionality (`JFR.configure, JFR.dump, JFR.start and JFR.stop`). 
>> 
>> I propose opening a separate issue to track updating the man page similarly to how it’s done for the JFR diagnostic commands. For example, 
>> 
>> 
>> filename         (Optional) Name of the file to which the flight recording data is
>>                    written when the recording is stopped. If no filename is given, a
>>                    filename is generated from the PID and the current date and is
>>                    placed in the directory where the process was started. The
>>                    filename may also be a directory in which case, the filename is
>>                    generated from the PID and the current date in the specified
>>                    directory. (STRING, no default value)
>> 
>>                    Note: If a filename is given, '%p' in the filename will be
>>                    replaced by the PID, and '%t' will be replaced by the time in
>>                    'yyyy_MM_dd_HH_mm_ss' format.
>> 
>> 
>> Unfortunately, per [8276265](https://bugs.openjdk.org/browse/JDK-8276265), sources for the jcmd manpage remain in Oracle internal repos so this PR can’t address that. 
>> 
>> Testing: 
>> 
>> - [x] Added test case passes. 
>> - [x] Modified existing VM.cds tests to also check for `%p` filenames. 
>> 
>> Looking forward to your comments and addressing any diagnostic commands I might have missed (if any). 
>> 
>> Cheers, 
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Adding FILE descriptor for help output

One more thing that's troubling me.  (Apologies it's now and not last week.)

I was looking at the _filename.value().get() usage and finding it uncomfortable, compared to the previous simple _filename.value() 8-)
Harder to remember and to read and understand.  Maybe we can avoid the two accessors, it really is just a char*.

These additional argument types look like part of the framework which never found an audience: MemorySizeArgument has one usage in CompilationMemoryStatisticDCmd, NanoTimeArgument looks unused -- so the two-accessor usage is only in once place until now?

Adding FileArgument as another of these might be the wrong direction, as these classes are so almost redundant.

What if we didn't add FileArgument, and kept using <char*> for _filename args/opts:

Then in DCmdArgument<char*>::parse_value(), recognise a "FILE" argument type and call Arguments::copy_expand_pid there, to set _value.

Just seeing if we can cut down some of the complexity here, as Thomas mentioned, it is already very complex for what it is!


(There is also the to_string method which seemed like it would be useful here, but it needs a buffer so is more complex than calling two accessors...  Another thing that seems to part of the framework that was never much adopted.)

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

PR Review: https://git.openjdk.org/jdk/pull/20198#pullrequestreview-2201623426