On Mon, 21 Jul 2025 12:03:11 GMT, David Beaumont <duke at openjdk.org> 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:
>
> Feedback changes.
The updates to SystemModuleFinder look okay, just need some cleanup.
src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 259:
> 257: ImageReader reader = SystemImage.reader();
> 258: try {
> 259: return reader.findNode("/modules").getChildNames().map(mn -> readModuleAttributes(reader, mn));
Can you put the line over 3 lines so that it's easier to see the pipeline.
src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 265:
> 263: }
> 264:
> 265: // Every module is required to have a valid module-info.class.
The methods in this class use use /** ..*/ in the method descriptions so would prefer if the changes change that. Here the method should really say that it returns the module's module-info, returning a holder for its class file attributes.
src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 276:
> 274: err = e;
> 275: }
> 276: throw new Error("Missing or invalid module-info.class for module: " + moduleName, err);
I assume this can move into the catch block.
src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 457:
> 455: * a non-resource node, then {@code null} is returned.
> 456: */
> 457: private ImageReader.Node findResourceNode(ImageReader reader, String name) throws IOException {
I think the usage would be clear if this were renamed to findResource.
src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 464:
> 462: String nodeName = "/modules/" + module + "/" + name;
> 463: ImageReader.Node node = reader.findNode(nodeName);
> 464: return node != null && node.isResource() ? node : null;
Style wise, can you put ( ) around the expression to avoid having to pause and check precedence.
src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 470:
> 468: public Optional<ByteBuffer> read(String name) throws IOException {
> 469: ImageReader reader = SystemImage.reader();
> 470: return Optional.ofNullable(findResourceNode(reader, name)).map(reader::getResourceBuffer);
Style wise, can you put the .map(...) on the next line so this is easier to read.
-------------
PR Review: https://git.openjdk.org/jdk/pull/26054#pullrequestreview-3045867656
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2224555704
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2224563056
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2224557574
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2224553155
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2224549636
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2224546104