On Mon, 15 Jan 2024 01:29:47 GMT, David Holmes <[email protected]> wrote:
>> The default value for the argument is what gets displayed in the help text.
>> For example:
>>
>>
>> ThreadDumpToFileDCmd::ThreadDumpToFileDCmd(outputStream* output, bool heap) :
>> DCmdWithParser(output, heap),
>> _overwrite("-overwrite", "May overwrite existing file", "BOOLEAN", false,
>> "false"),
>> _format("-format", "Output format ("plain" or "json")", "STRING", false,
>> "plain"),
>> _filepath("filepath", "The file path to the output file", "STRING", true) {
>> _dcmdparser.add_dcmd_option(&_overwrite);
>> _dcmdparser.add_dcmd_option(&_format);
>> _dcmdparser.add_dcmd_argument(&_filepath);
>> }
>>
>>
>> And the help text:
>>
>>
>> Options: (options must be specified using the <key> or <key>=<value> syntax)
>> -overwrite : [optional] May overwrite existing file (BOOLEAN, false)
>> -format : [optional] Output format ("plain" or "json") (STRING, plain)
>>
>>
>> The help output that indicates that "plain" is the default format comes from
>> the intialization of the _format argument. There is no separate help text.
>
> Ugghh! So the help text is an actual stringification of the actual default
> value of the "field", whereas in this case the real default value comes from
> passing null to `CodeCache::write_perf_map`. So we need this hack to deal
> with that. That is truly awful IMO. The only way to cleanly address that is
> to expand things so that you can set an actual help string to be used.
This check is problematic:
if (strncmp(filename, DEFAULT_PERFMAP_FILENAME,
strlen(DEFAULT_PERFMAP_FILENAME)) == 0)
Because it cannot tell whether `filename` was explicitly given by the user. As
a result, if the user specifies:
jcmd 1234 perfmap '/tmp/perf-<pid>.map'
the file will be written as `/tmp/perf-1234.map`, but if the user specifies
jcmd 1234 perfmap '/tmp/<pid>-perf.map'
then the file will be written as `/tmp/<pid>-perf.map`.
This gives the confusing impression that the string `<pid>` is sometimes
substituted and sometimes not.
I think it's better to do this (similarly for the `VM.cds` case):
void PerfMapDCmd::execute(DCmdSource source, TRAPS) {
CodeCache::write_perf_map(_filename.is_set() ? _filename.value() : nullptr);
}
This essentially makes the JVM behavior exactly the same as before, so I think
we can change the JBS issue title to something like "Clarify docs for filename
parameter to Compiler.perfmap and VM.cds".
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17359#discussion_r1451918737
