On Fri, 11 Jul 2025 14:41:30 GMT, David Beaumont <[email protected]> wrote:
>> Refactoring `ImageReader` to make it easy to add preview mode functionality
>> for Valhalla.
>>
>> This PR is a large change to `ImageReader` (effectively a rewrite) but
>> reduces the surface area of the API significantly, reduces code complexity
>> and increases performance/memory efficiency. The need for this sort of
>> change comes from the need to support "preview mode" in Valhalla for classes
>> loaded from core modules.
>>
>> ### Preview Mode
>>
>> In the proposed preview mode support for Valhalla, a resource with the name
>> `/modules/<module>/<path>` may come from one of two locations in the
>> underlying jimage file; `/<module>/<path>` or
>> `/<module>/META-INF/preview/<path>`. Furthermore, directories (represented
>> as directory nodes via the API) will have a potentially different number of
>> child nodes depending on whether preview mode is in effect, and some
>> directories may only exist at all in preview mode.
>>
>> Furthermore, the directories and symbolic link nodes in `/packages/...` will
>> also be affected by the existence of new package directories. To retain
>> consistency and avoid "surprises" later, all of this needs to be taken into
>> account.
>>
>> Below is a summary of the main changes for mainline JDK to better support
>> preview mode later:
>>
>> ### 1: Improved encapsulation for preview mode
>>
>> The current `ImageReader` code exposes the data from the jimage file in
>> several ways; via the `Node` abstraction, but also with methods which return
>> an `ImageLocation` instance directly. In preview mode it would not be
>> acceptable to return the `ImageLocation`, since that would leak the
>> existence of resources in the `META-INF/preview` namespace and lets users
>> see resource nodes with names that don't match the underlying
>> `ImageLocation` from which their contents come.
>>
>> As such, the PR removes all methods which can leak `ImageLocation` or other
>> "underlying" semantics about the jimage file. Luckily most of these are
>> either used minimally and easily migrated to using nodes, or they were not
>> being used at all. This PR also removes any methods which were already
>> unused across the OpenJDK codebase (if I discover any issues with
>> over-trimming of the API during full CI testing, it will be easy to address).
>>
>> ### 2. Simplification of directory child node handling
>>
>> The current `ImageReader` code attempts to create parent directories "on
>> demand" for any child nodes it creates. This results in parent directories
>> having a non-empty but incomplete set of child nodes present. This is re...
>
> 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.
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.
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.
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.
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2205724093
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2205741054
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2205751547
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2205754037
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2205816781
