On Fri, 15 Mar 2024 19:22:58 GMT, Chris Plummer <[email protected]> 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
