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

[Kevin Walls]

On Mon, 11 Mar 2024 11:57:04 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.

-XX:HeapDumpPath was introduced by -XX:+HeapDumpOnOutOfMemoryError for its use.

globals.hpp documents HeapDumpPath as relating to HeapDumpOnOutOfMemoryError (so that will need changing if this change is happening).

It does look useful to use HeapDumpPath as the default for jcmd, although maybe there's some confusion possible if you're already relying on it for OOM dumps.

jcmd GC.heap_dump has the overwrite flag.
OOM dumping in HeapDumper::dump_heap(bool oome) has a sequence number.

Can it be specified how these will interact? 8-)
Does overwrite mean you don't want to increase sequence number (but we already increased it last iteration)... 


I like splitting out alloc_and_create_heapdump_pathname() as this is already a large part of dump_heap.
Should the comment say, "caller must free the returned pointer".

HeapDumper::dump_to_heapdump_path
This new name makes less sense to me, as we already use heapdump path, but maybe you need it to avoid the dcmd having to work out the final filename. 8-)

Does dump_to_heapdump_path() not print the ("Dumping heap to %s ...", path) message?  That seems important when the user isn't specifying it directly.

But the policy questions probably important to confirm before worrying about those and any other code comments.

A test would not be a bad thing. 8-)

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

PR Review: https://git.openjdk.org/jdk/pull/18190#pullrequestreview-1930752683