RFR: 8360037: Refactor ImageReader in preparation for Valhalla support

[Alan Bateman]

On Mon, 30 Jun 2025 23:27:49 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 referred to as the directory being "incomple...

src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 62:

> 60: import jdk.internal.module.ModuleHashes.HashSupplier;
> 61: 
> 62: import static java.util.Objects.requireNonNull;

The existing Objects.requireNonNull are okay, no need to change them all as it is nothing to do with the changes.

src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 251:

> 249:     }
> 250: 
> 251:     private static Stream<ModuleInfo.Attributes> getModuleAttributes() {

Can you rename this to allModuleAttributes and add a method description to match the other methods.

src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 257:

> 255:                 .getChildNames()
> 256:                 .map(mn -> getNode(reader, mn + "/module-info.class"))
> 257:                 // This fails with ISE if the node isn't a resource (corrupt JImage).

Can you drop this comment and check getNode to thrown an Error (ISE isn't right when we have a broken image).

test/micro/org/openjdk/bench/jdk/internal/jrtfs/ImageReaderBenchmark.java line 60:

> 58: @OutputTimeUnit(TimeUnit.MILLISECONDS)
> 59: @Fork(value = 1, jvmArgs = {"--add-exports", "java.base/jdk.internal.jimage=ALL-UNNAMED"})
> 60: public class ImageReaderBenchmark {

This is the benchmark from the other PR, did you mean to include it?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2177761299
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2177768203
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2177772042
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2177762265