On Thu, 14 Mar 2024 13:43:09 GMT, Matthias Baesken <[email protected]> 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:
>
> add test HeapDumpJcmdPresetPathTest
Changes requested by cjplummer (Reviewer).
src/hotspot/share/runtime/globals.hpp line 565:
> 563: "triggered by jcmd GC.heap_dump without specifying a path, "
> \
> 564: "the path (filename or directory) of the dump file "
> \
> 565: "(defaults to java_pid<pid>.hprof in the working directory)")
> \
This incorrectly leads one to conclude that if HeapDumpPath is not specified
and GC.heap_dump is used without specifying a path, the default will be
java_pid<pid>.hprof in the working directory. That's not the case. The jcmd
will produce an error because it requires that either HeapDumpPath be specified
or a filename be specified as a jcmd argument (I'm not sure why the jcmd does
not default to java_pid<pid>.hprof)
Also, if you are cleaning up this text, I would suggest changing "is on" to "is
enabled". Same for HeapDumpGzipLevel below.
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,
> "-XX:HeapDumpPath"),
I can't say I like using the "default" in this manner, but I understand why it
was done. I wish there was a better way. I ran into a similar problem with
Compiler.perfmap in #17359, but this case is a bit worse. For Compiler.perfmap
the default had to be specified as `/tmp/perf-<pid>.map`, but at least it
somewhat resembled the actual default.
Maybe it would help if you used something a bit more descriptive, like "The
path specified by "-XX:HeapDumpPath".
You also should update jcmd.l to describe this argument much like I did for
Compiler.perfmap. You especially need to call out that if -XX:HeapDumpPath is
not used then the "filename" argument must be speified.
-------------
PR Review: https://git.openjdk.org/jdk/pull/18190#pullrequestreview-1937265589
PR Review Comment: https://git.openjdk.org/jdk/pull/18190#discussion_r1525211803
PR Review Comment: https://git.openjdk.org/jdk/pull/18190#discussion_r1525223292
