On Mon, 30 Jun 2025 23:27:49 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
> 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
