On Fri, 15 Mar 2024 11:24:53 GMT, Matthias Baesken <mbaes...@openjdk.org> wrote:
>> Currently jcmd command GC.heap_dump only works with an additionally provided
>> file name.
>> Syntax : GC.heap_dump [options] <filename>
>>
>> In case the JVM has the XX - flag HeapDumpPath set, we should support an
>> additional mode where the <filename> is optional.
>> In case the filename is NOT set, we take the HeapDumpPath (file or
>> directory);
>>
>> new syntax :
>> GC.heap_dump [options] <filename> .. has precedence over second option
>> GC.heap_dump [options] …in case -XX: HeapDumpPath=p is set
>>
>> This would be a simplification e.g. for support cases where a filename or
>> directory is set at JVM startup with -XX: HeapDumpPath=p and writing to the
>> path is intended/recommended for usage also in the jcmd case.
>
> 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.
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18190#discussion_r1526734113
PR Review Comment: https://git.openjdk.org/jdk/pull/18190#discussion_r1526736892
