RFR: 8367005: ImageReader refactor caused performance regressions for startup and footprint [v2]

[Roger Riggs]

On Thu, 11 Sep 2025 00:34:00 GMT, David Beaumont <duke at openjdk.org> wrote:

>> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 379:
>> 
>>> 377:             // exist, so it skips checking the nodes cache and only checks for
>>> 378:             // an ImageLocation.
>>> 379:             if (moduleName.contains("/") || resourcePath.startsWith("/")) {
>> 
>> The fast-path guard here and above might improve if they were done before synchronizing, e.g. dropping the modifier and putting a `synchonized (this)` block around the rest of the method. 
>> 
>> Putting the likely cheaper `resourcePath.startsWith` first might be a small win, too, depending on input/benchmark.
>
> When called from SystemModuleFinders this really should never happen (arguably it's IAE) so it won't make a difference per-call if it's changed, and it won't really help reduce the time of the locked section.
> Both checks will basically always be done, so ordering isn't an issue (but a fair point to raise).
> 
> Module names are typically short, so while a string scan is not ideal, it's not awful (and we do string concatentaion later anyway). Ideally there would be a ModuleName type to use as the parameter.

While you are at it, both findNode and BuildModulesNode should have comments indicating that the node can be found in one of two places.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27203#discussion_r2338286808