On Thu, 11 Sep 2025 17:22:27 GMT, David Beaumont <[email protected]> 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:
>
> Tweaked comment.
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 330:
> 328: // because those nodes are already in the nodes cache.
> 329: if (name.startsWith(MODULES_ROOT + "/")) {
> 330: // This may preform two lookups, one for a directory
> (in
I think this is a typo - should have been "perform"?
test/jdk/jdk/internal/jimage/ImageReaderTest.java line 166:
> 164: // Or module name to be empty.
> 165: ":modfoo/com/foo/Alpha.class"})
> 166: public void testFindResourceNode_absent(String modAndPath) throws
> IOException {
Hello David, for the newly introduced methods, could we also test for empty
resource name? I believe the new methods should not find such a resource.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27203#discussion_r2343733831
PR Review Comment: https://git.openjdk.org/jdk/pull/27203#discussion_r2343736919
