On Tue, 5 Dec 2023 16:14:31 GMT, Evgeny Astigeevich <[email protected]>
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
