On Wed, 10 Sep 2025 23:57:36 GMT, Claes Redestad <[email protected]> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Found additional place where new API can be used.
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27203#discussion_r2338229587
