RFR: 8360037: Refactor ImageReader in preparation for Valhalla support [v7]

[David Beaumont]

On Mon, 14 Jul 2025 20:11:51 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> David Beaumont has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Linting of a few minor issues (nothing serious).
>>   
>>   * Linting of minor issues.
>>   * Factored out the module existence test, it's only a performance heuristic and could (should?) be removed.
>
> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 148:
> 
>> 146:     /**
>> 147:      * Returns the content of a resource node.
>> 148:      *
> 
> It may be helpful to mention that the byte[] returned is a copy and on return the caller is the owner.
> If/when we have immutable byte arrays, it would save allocation and copying to return a/the cached byte array.

Will do. Though I'd be very cautious about caching arrays here since these bytes are likely consumed and transformed by callers, and probably only looked-up once or twice at most.

> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 182:
> 
>> 180: 
>> 181:     private static final class SharedImageReader extends BasicImageReader {
>> 182:         // TODO: Should this be OPEN_FILES or openFiles (it's not constant).
> 
> All caps is good for final statics.

Even if mutable? I've seen both in the codebase so wasn't sure what the style should be. I'll remove the TODO.

> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 204:
> 
>> 202:             super(imagePath, byteOrder);
>> 203:             this.imageFileAttributes = Files.readAttributes(imagePath, BasicFileAttributes.class);
>> 204:             // TODO (review note): Given there are ~30,000 nodes in the image, is setting an initial capacity a good idea?
> 
> Likely yes, saving multiple (12) re-sizing's before reaching the 30k occupancy.

Any thoughts on a good-enough first guess. The benchmark has a list of 800 or so classes loaded just to get the simplest "hello world" working, so I'm thinking that something between 2000 and 5000 as an initial size would be justifiable (and we can always change it later). Ideally we'd be able to report/imply the value and figure it out empirically.

> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 207:
> 
>> 205:             this.nodes = new HashMap<>();
>> 206:             // TODO (review note): These should exist under all circumstances, but there's
>> 207:             //  probably a more robust way of getting at these offsets.
> 
> ok as is, its stable.

Cool. Will remove the TODO. Thanks.

> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 348:
> 
>> 346:             }
>> 347:             // Tests the implied module directory location "/modules/<module>" exists.
>> 348:             ImageLocation loc = findLocation(name.substring(0, moduleEnd));
> 
> There might be an opportunity to save some string creation if some functions took a (string, start, end) parameters. It would need to ripple down through ImageStringsReader.hashCode and would make the code less readable.

Yeah, I looked at making a hashcode from string parts, but the API gets messy fast. I still might do it as a followup (now we will have benchmarks to test performance with).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2207091408
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2207085022
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2207083384
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2207085176
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2207088339