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.
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 51:
> 49: * file system friendly way. The jimage entries (resources, module and
> package
> 50: * information) are mapped into a unified hierarchy of named nodes, which
> serve
> 51: * as the underlying structure for the {@code JrtFileSystem} and other
> utilities.
I think it's a bit confusing to document this class as an adapter for a
BasicImageReader because you obtain one with static open methods that specify
the file path to the jimage file. So I think better to start out in the class
description by saying that it provide an API that can be used to create file
system and other views over the contents of the jimage file.
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 106:
> 104: *
> 105: * <p>This is the expected was to open an {@code ImageReader}, and
> it should
> 106: * be rare for anything other than the native byte order to be
> needed.
I think the "Almost all callers" and "This is the expected was (typo)" comments
are confusing. Can we remove these?
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 138:
> 136: * @param name a JRT file system name (path) of the form
> 137: * {@code "/modules/<module>/...} or {@code
> "/packages/<package>/...}.
> 138: * @return a node representing a resource, directory or symbolic
> link.
This is the jimage reader rather than the jrt file system so it's very
confusing to see comments about "JRT file system" in this file.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2224593051
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2224578163
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2224596684
