On Wed, 30 Jul 2025 10:39:22 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: > > Convert non-visible markdown comments to JavaDoc for consistency. src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 189: > 187: private final Set<ImageReader> openers = new HashSet<>(); > 188: > 189: // Attributes of the .jimage file. The jimage file does not > contain Nit - (pre-existing) calling it a `.jimage` file makes it look like `jimage` is a (well known) extension for jimage files. As far as I know, it isn't (the default jimage file that we ship in the JDK is just named `modules`). Perhaps we should change this to "Attributes of the jimage file."? src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 283: > 281: * > 282: * <p>Note also that there is no reentrant calling back to this > method from within > 283: * the node handling code. This paragraph and the previous one feels like too much detail and I'm not sure they are necessary. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2245335120 PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2245442999
