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 498:
> 496: // in case no specific filename was passed to heap_dump command,
> 497: // attempt to use the HeapDumpPath if it was set
> 498: const char* filename = HeapDumpPath;
Why is this here? It is confusing because it makes it look like the
HeapDumpPath will be used in place of specifying a filename with the jcmd, but
in fact HeapDumpPath can be null, and logic below relies on that fact. Why not
just check if HeapDumpPath is null below. You don't really need `filename`.
See suggested changes below.
src/hotspot/share/services/diagnosticCommand.cpp line 531:
> 529: use_heapdump_path = true;
> 530: }
> 531:
Suggestion:
if (!_filename.is_set()) {
if (HeapDumpPath != nullptr) {
// use HeapDumpPath (file or directory is possible)
use_heapdump_path = true;
} else {
output()->print_cr("Filename or -XX:HeapDumpPath must be set!");
return;
}
}
src/hotspot/share/services/diagnosticCommand.cpp line 540:
> 538: dumper.dump_to(output(), (int)level, _overwrite.value(),
> (uint)parallel);
> 539: } else {
> 540: dumper.dump(filename, output(), (int)level, _overwrite.value(),
> (uint)parallel);
Suggestion:
dumper.dump(_filename.value(), output(), (int)level, _overwrite.value(),
(uint)parallel);
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18190#discussion_r1538535068
PR Review Comment: https://git.openjdk.org/jdk/pull/18190#discussion_r1538535317
PR Review Comment: https://git.openjdk.org/jdk/pull/18190#discussion_r1538535458
