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

[David Beaumont]

On Thu, 11 Sep 2025 10:43:55 GMT, David Beaumont <duke at openjdk.org> wrote:

>> Summary: Add two new methods to ImageReader to make SystemModuleReader more performant.
>> 
>> Analysis of benchmarks shows that when the vast majority of resources (~11,000 in the Perfstartup-SwingSet-G1 benchmark) tested for in SystemModuleReader do NOT exist, performance is degraded compared to this code prior to the refactoring in JDK-8360037.
>> 
>> The current refactoring of ImageReader has everything going through a single "findNode()" method for simplest possible encapsulation, but while this is functionally correct, it's not tuned for testing for the non-existence of resources.
>> 
>> In particular:
>> 1. SystemModuleReader only requests resources (i.e. things in the jimage file with paths *not* starting /modules/ or /packages/). This means findNode() does two look-ups for the resource, the first of which will always fail.
>> 2. The containsResource() logic doesn't need to create and cache nodes in ImageReader, it can just check for the presence of an ImageLocation corresponding to a resource.
>> 
>> Thus two new methods are added to resolve these cases:
>> * findResourceNode(module, path)
>> * containsResource(module, path)
>> 
>> Their API takes module and path separately so as to not be confusable with findNode(nodename).
>> 
>> However care must be taken to prevent these methods being fooled into returning non-resource entries (this was possible before the refactoring by using module names like "modules" or "packages") so new tests have been added.
>
> David Beaumont has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Feedback.

src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 382:

> 380: 
> 381:         /** Returns whether a resource exists in the given module. */
> 382:         boolean containsResource(String moduleName, String resourcePath) {

I *think* I can drop synchronization here since I'm not using the nodes cache in this method. However I'm going to triple check that BasicImageReader is thread safe before going with this (it's not documented either way).

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

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