RFR: 8314029: Add file name parameter to Compiler.perfmap [v5]

[Chris Plummer]

On Tue, 5 Dec 2023 16:14:31 GMT, Evgeny Astigeevich <eastigeevich at openjdk.org> wrote:

>> src/hotspot/share/code/codeCache.cpp line 1809:
>> 
>>> 1807: }
>>> 1808: 
>>> 1809: void CodeCache::write_perf_map(const char* filename) {
>> 
>> Why not have a `filename == nullptr` indicate that the default should be used. Then you don't need CodeCache::DefaultPerfMapFile. You can just have a private `CodeCache::defaultPerfmapFileName()` method.
>
> Hi Chris,
> The current design of `write_perf_map` provides a clean and explicit interface. The purpose of the function is evident from its signature: to write a perf map into a specified file. This explicitness makes the code more readable and self-documenting. It reduces the need for developers to go to the implementation to figure out: what is the meaning of `nullptr`; where a filename will be taken from. It also serves as a contract between the caller and the function itself. By explicitly requiring a filename, the function sets clear expectations for the caller.
> 
> I think `CodeCache::write_default_perf_map` hiding the filename of the default perf map might not be a good idea because it makes impossible to get the filename used in it.  I prefer either method `CodeCache::defaultPerfmapFileName()` or class `CodeCache::DefaultPerfmapFileName`. The class is simpler to implement than the method (like it was earlier).

The default filename was already "hidden" before these changes, so at the very least things are not being made any worse, but I don't see why any users `write_perf_map` would ever need the default filename.  I just felt that adding and exporting a class whose only purpose is to provide the default name seemed like unnecessary overkill. I'm not so sure having a public CodeCache::defaultPerfmapFileName() API and two `write_perf_map` APIs isn't overkill also. There is nothing wrong with a null filename argument signally to use some default name. You can also have the filename arg default to `nullptr`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15871#discussion_r1416228456