On Mon, 14 Jul 2025 20:11:51 GMT, Roger Riggs <rri...@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