RFR: 8323546: Clarify docs for Compiler.perfmap filename parameter, and other misc related jcmd doc cleanups [v4]

[Ioi Lam]

On Mon, 15 Jan 2024 01:29:47 GMT, David Holmes <dholmes at openjdk.org> 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