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

[Chris Plummer]

On Thu, 14 Mar 2024 13:43:09 GMT, Matthias Baesken <mbaesken at 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:
> 
>   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