RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v8]

[Matthias Baesken]

On Fri, 15 Mar 2024 19:22:58 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Adjust jcmd manpage, help and globals comment
>
> src/hotspot/share/services/diagnosticCommand.cpp line 475:
> 
>> 473: HeapDumpDCmd::HeapDumpDCmd(outputStream* output, bool heap) :
>> 474:                            DCmdWithParser(output, heap),
>> 475:   _filename("filename","Name of the dump file", "STRING", false, "if no filename was specified, but -XX:HeapDumpPath=hdp is set, path hdp is taken"),
> 
> This seems clumsy, but I'm having a hard time coming up with something better.
> 
> "the filename specified by -XX:HeapDumpPath, if specified"
> "If -XX:HeapDumpPath is specified, then it is used as the default"
> 
> Makes me wonder if this approach is wrong since it is hard to document clearly. Maybe we should go back to not having the jcmd use HeapDumpPath. I understand the argument for having the jcmd use the HeapDumpPath setting, but it might not be worth the documentation confusion it introduces. You can argue that HeapDumpPath really is just intended to help support HeapDumpOnOutOfMemoryError. Does the jcmd also use HeapDumpGzipLevel? I'm not sure, but we should be consistent with the application of HeapDumpPath and HeapDumpGzipLevel to the jcmd.

So far we only use the gzip level setting from jcmd, not HeapDumpGzipLevel .
See the `level` variable in  `HeapDumpDCmd::execute` . Yeah you are probably right we should make it consistent.

> src/jdk.jcmd/share/man/jcmd.1 line 340:
> 
>> 338: \f[I]filename\f[R]: The name of the dump file; in case no file is specified
>> 339: but -XX:HeapDumpPath=path is set, the path provided by HeapDumpPath is used
>> 340: (STRING, no default value)
> 
> Here we say "no default value" but the actual text of the help output says something different. But then what do we put in place of "no default value" here? Descriptive text (like in the help output) is not needed since we have the description here. I don't really have an answer for how to handle this.

Hi , 'no default' should be still correct. Both the old syntax and also the additional optional new one (evaluating -XX:HeapDumpPath) do not set some default string. The user has to specify something.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18190#discussion_r1527168801
PR Review Comment: https://git.openjdk.org/jdk/pull/18190#discussion_r1527167229