On Mon, 21 Jul 2025 12:03:11 GMT, David Beaumont <d...@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
