8368333: [lworld] Add preview mode to ImageReader and JRT file-system #1619

david-beaumont · 2025-09-23T19:15:43Z

Java changes for supporting preview mode when preview mode resources (with new location flags) are available.

At the moment, this code will operate on non-preview jimage files (1.0) and act as if no preview resources are available by virtue of the default value for missing attributes and package flags being zero (which matches jimage 1.0).

This should be reviewed on top of #1618

Progress

Change must not contain extraneous whitespace

Integration blocker

⚠️ Dependency #1618 must be integrated first

Issue

JDK-8368333: [lworld] Add preview mode to ImageReader and JRT file-system (Sub-task - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1619/head:pull/1619
$ git checkout pull/1619

Update a local copy of the PR:
$ git checkout pull/1619
$ git pull https://git.openjdk.org/valhalla.git pull/1619/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1619

View PR using the GUI difftool:
$ git pr show -t 1619

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1619.diff

Using Webrev

Link to Webrev Comment

david-beaumont · 2025-09-23T19:16:47Z

This is a new version of #1613, re-created so there's now a pr/xxx branch for use by dependent PRs.

bridgekeeper · 2025-09-23T19:17:55Z

👋 Welcome back david-beaumont! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-09-23T19:19:25Z

@david-beaumont This change is no longer ready for integration - check the PR body for details.

mlbridge · 2025-09-23T19:23:16Z

Webrevs

src/java.base/share/classes/jdk/internal/jimage/ImageHeader.java

RogerRiggs · 2025-09-24T18:22:48Z

src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java

+    /**
+     * Set on all locations in the {@code /modules/xxx/META-INF/preview/...}
+     * namespace.
+     *
+     * <p>This flag is mutually exclusive with {@link #FLAGS_HAS_PREVIEW_VERSION}.
+     */
+    public static final int FLAGS_IS_PREVIEW_VERSION = 0x2;


Seems to be redundant with HAS_PREVIEW_VERSION when seen in a preview location.

Ah, but HAS_PREVIEW_VERSION is never set on a preview location. It's only for non-preview locations to indicate that a (different) preview version exists.

HAS_PREVIEW_VERSION can also be set for /packages/xxx directory entries (where it means "some of the entries are modules in which preview versions of resources exist" rather than "there's a META-INF/preview version of this entry"). IS_PREVIEW_VERSION is kind of meaningless for the /packages/xxx directories though, and is never set on them.

I'm not sure I'm a fan of trying to merge the semantics of these into some sort of "HAS_OR_IS_PREVIEW_VERSION" flag though. We're not short on flag space (and are extremely unlikely to ever be).

I see the semantics of the flag as there-is-a-preview-version of the resource.
Where-ever it appears, it means that the resource/class is in the preview branch of the hierarchy.
If the flag appears in the normal hierarchy index, it still means the same, look in the preview hierarchy.
If it appears on a directory, it means there preview resources in/below.

There is no "the resource/class" in the jimage file, because both can be present. It depends on whether the path to the entry contains META-INF/preview or not. So where a normal resource has a preview version, the same flag would be set on both.

For example, I don't see how this works if the flags have the same value.

// Regardless of preview mode, don't return resources requested directly // via their preview path. if ((flags & ImageLocation::FLAGS_IS_PREVIEW_VERSION) != 0) { return 0L; } // Even if there is a preview version, we might not want to return it. if (!is_preview_mode || (flags & ImageLocation::FLAGS_HAS_PREVIEW_VERSION) == 0) { return locOffset; }

RogerRiggs · 2025-09-24T18:23:16Z

src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java

+    /**
+     * Indicates that a location only exists due to preview resources.
+     *
+     * <p>This can apply to both resources and directories in the
+     * {@code /modules/xxx/...} namespace, as well as {@code /packages/xxx}
+     * directories.
+     *
+     * <p>For {@code /packages/xxx} directories it indicates that, for every
+     * module in which the package exists, it is preview only.
+     *
+     * <p>This flag is mutually exclusive with {@link #FLAGS_HAS_PREVIEW_VERSION}
+     * and need not imply that {@link #FLAGS_IS_PREVIEW_VERSION} is set (i.e.
+     * for {@code /packages/xxx} directories).
+     */
+    public static final int FLAGS_IS_PREVIEW_ONLY = 0x4;


The preview-only case is expected to be the empty set. There seems to be a lot of code dedicated to it.

Well it's a small set, but vitally important. Preview only is the case where you can't lookup the normal resource first and check its flags to see if there's a preview version. Since you must correct find all preview-only resources, you are left with either:

always speculatively looking up all resources in their preview location when not found otherwise.

Doing something "cleverer" to reduce double lookups.

In the C++ class-loader (as I understand it) most lookups will be for things referenced in other class files, and thus expected to exist. This makes "if you can't find the normal version, look for the preview version" acceptable (I think) because this should be only in cases where you're looking for something that should exist at one of the two paths.

In the Java stuff, there's a lot of speculative lookup, especially for non-class resources. All modules are tested for any non-existent resource. So in this case the strategy needs to be cleverer or you'll basically (at least) double the cost of a lot of things.

So I settled on the easiest approach to be to just (efficiently) pre-scan for preview resources and put them in the node cache during init (this is only about the same work the old code did for link nodes etc.). Once they're cached, the lookup code no longer has to care about 2nd lookups or anything. Preview versions and preview-only versions are just treated the same, they're in the cache, so they're found and returned without looking at anything else.

This is partly why there's the "ensureCached" method which guarantees you never replace what's in the cache with anything else (so preview stuff put there during init of the reader is "safe" from accidental overwriting).

The new flag stuff and change of ordering of package entries is there mostly to support efficient pre-scanning of the small set of preview only resources.

I'm still going to say, there are no plans to have any Preview-only resources.

And yes, a preview-only flag would be useful to (mostly) avoid a second lookup.
If I understand the design, there is still a location in the normal tree with the preview-only flag but there is no resource at that location, only the flags. And a second lookup is needed to find the location in the preview heirarchy. The caching avoids repeated 2nd lookups.

Any new lambda or inner-class in a preview class is a new preview-only resource. It's definitely going to happen.
Additionally, if an entry is preview only, there's simply nothing in the jimage at the non-preview location. No flags, no nothing.

RogerRiggs · 2025-09-24T18:24:52Z

src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java

+     * Helper function to calculate preview flags (ATTRIBUTE_PREVIEW_FLAGS).
+     *


A summary of what gets what flags might be easier to read than the code.
Or list the flags and refer to their descriptions (that would describe the paths where the flags would be found)

Done in latest snapshot.

RogerRiggs · 2025-09-24T18:26:07Z

src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java

+     * Indicates that a non-preview location is associated with preview
+     * resources.


This comment would read better saying that preview resources exist related to this location.
More like FLAGS_HAS_PREVIEW_VERSION.

Ahh, but it's deliberately not worded like that because this flag can be set for "/packages/xxx" entries, for which "preview resources exist related to this location" is not a meaningful statement.

This is where you could add another flag (e.g. "FLAGS_PACKAGE_HAS_PREVIEW_ENTRIES"), but it felt better to use the same flag for this, since where it's calculated becomes a neat case of just propagating the flag (next PR):

// Calculate the package's ImageLocation flags. boolean hasPreviewVersion = moduleReferences.stream().anyMatch(ModuleReference::hasPreviewVersion); boolean isPreviewOnly = moduleReferences.stream().allMatch(ModuleReference::isPreviewOnly); return (hasPreviewVersion ? ImageLocation.FLAGS_HAS_PREVIEW_VERSION : 0) | (isPreviewOnly ? ImageLocation.FLAGS_IS_PREVIEW_ONLY : 0);

And the flag/method names in this code just match across this logic.

src/java.base/share/classes/jdk/internal/jimage/ImageReader.java

src/java.base/share/classes/jdk/internal/jrtfs/JrtFileSystemProvider.java

src/java.base/share/classes/jdk/internal/jrtfs/SystemImage.java

test/jdk/jdk/internal/jimage/ModuleReferenceTest.java

* Renaming slightly confusing "testEncoder" method. * Fixing unit tests to use new constructor.

…values.

* Removing package root flag based on feedback. * Changing existing package flags during writing to match altered flag values. * Feedback changes, and fixing some comments. * Renaming slightly confusing "testEncoder" method. * Fixing unit tests to use new constructor. * Word smithing flags definitions. * Add workaround until new image writing code is in * Clarifying flag docs for /packages/xxx case * Java ImageReader changes for preview mode

david-beaumont · 2025-10-14T18:32:17Z

test/jdk/tools/jimage/ImageReaderDuplicateChildNodesTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.


Restoring the copyright year accidentally removed in the last change.

RogerRiggs · 2025-10-09T15:54:24Z

src/java.base/share/classes/jdk/internal/jimage/ModuleReference.java

+    /**
+     * Returns whether the package associated with this reference contains
+     * resources in this reference's module.
+     *
+     * <p>An invariant of the module system is that while a package may exist
+     * under many modules, it is only non-empty in one.
+     */
+    public boolean hasContent() {
+        return ((flags & FLAGS_HAS_CONTENT) != 0);
+    }


Where is the presumed hierarchy of package non-emptyness used?

RogerRiggs · 2025-10-09T15:56:39Z

src/java.base/share/classes/jdk/internal/jimage/ModuleReference.java

+    }
+
+    /** Returns whether this reference exists only in preview mode. */
+    public boolean isPreviewOnly() {


Pick either PreviewOnly or hasNormal and use consistently. (Even if one is the opposite of the other).

They are NOT opposites. It is not possible to use only one or these flags and continue to be logically correct.

Hmm, the implementation of isPreviewOnly() is exactly !hasNormalVersion(flags)

RogerRiggs · 2025-10-09T16:03:53Z

src/java.base/share/classes/jdk/internal/jrtfs/SystemImage.java

+        // TODO: Support preview mode in ExplodedImage and remove this check.
+        if (mode.isPreviewModeEnabled())
+            throw new UnsupportedOperationException(
+                    "Preview mode not yet supported for exploded images");


I'd remove "yet", it implies some planned work.

Well I do plan to do it.

RogerRiggs · 2025-10-21T18:52:03Z

src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java

    public static final int ATTRIBUTE_COMPRESSED = 6;
    public static final int ATTRIBUTE_UNCOMPRESSED = 7;
-    public static final int ATTRIBUTE_COUNT = 8;
+    public static final int ATTRIBUTE_PREVIEW_FLAGS = 8;


Please add a comment above these constants that the values are defined in ImageFile.hpp.

RogerRiggs · 2025-10-21T18:55:07Z

src/java.base/share/classes/jdk/internal/jimage/ModuleReference.java

+    /** If set, this package exists in preview mode. */
+    private static final int FLAGS_HAS_PREVIEW_VERSION = 0x1;
+    /** If set, this package exists in non-preview mode. */
+    private static final int FLAGS_HAS_NORMAL_VERSION = 0x2;
+    /** If set, the associated module has resources (in normal or preview mode). */
+    // TODO: Make this private again when image writer code is updated.
+    public static final int FLAGS_HAS_CONTENT = 0x4;


Please change the prefix of these to distinguish them from the ImageLocation flags.
For example, "PKG_" or "MR_".

RogerRiggs · 2025-10-21T18:57:11Z

src/java.base/share/classes/jdk/internal/jimage/ImageHeader.java

    public static final int MAJOR_VERSION = 1;
-    public static final int MINOR_VERSION = 0;
+    public static final int MINOR_VERSION = 1;
    private static final int HEADER_SLOTS = 7;


Please add a comment before these constants connecting them to imageFile.hpp.

RogerRiggs · 2025-10-21T19:07:15Z

src/java.base/share/classes/jdk/internal/jimage/ImageStrings.java

 public interface ImageStrings {
+    // String offset constants are useful for efficient classification
+    // of location entries without string comparison. These may change
+    // between jimage versions (they are checked during initialization).


The checking seem only to be done when ImageStringsWriter is loaded/initialized.
There may be a small risk that a mismatch of the image with the ImageStringsReader could occur.
Can they be checked also by the reader?

Hmm, I don't see a nice way to do this at runtime since you can't lookup string offsets by their value (as far as I can see).

It's no more fragile than the old code (which had hard-coded constants for "class" and "" (and wasn't self-checking their validity). At least now it's tested at build time.

If the version number matches it should be safe. I'll add a comment about how changing existing entries is problematic.

RogerRiggs · 2025-10-21T19:32:55Z

src/java.base/share/classes/jdk/internal/jimage/ModuleReference.java

+     * <p>An invariant of the module system is that while a package may exist
+     * under many modules, it only has content in one.
+     */
+    public boolean hasContent() {


As suggested, hasResoruces would be consistent with the lookups for resources.

RogerRiggs · 2025-10-21T19:43:04Z

src/java.base/share/classes/jdk/internal/jimage/ImageReader.java

+        // A relativized mapping from non-preview name to directories containing
+        // preview-only nodes. This is used to add preview-only content to
+        // directories as they are completed.
+        private final HashMap<String, Directory> previewDirectoriesToMerge;


Can this be cleared or freed after the ImageReader is open. Its no longer needed.

It's used when directories are completed (as stated in the comment):

List<Node> previewOnlyNodes = getPreviewNodesToMerge(dir); ... children.addAll(previewOnlyNodes);

This is so that a preview only directory (and all its content) appears correctly in its parent's child list.

RogerRiggs · 2025-10-21T19:53:29Z

src/java.base/share/classes/jdk/internal/jimage/ImageReader.java

+            }
            ImageLocation loc = findLocation(moduleName, resourcePath);
-            return loc != null && isResource(loc);
+            return loc != null && loc.getType() == RESOURCE;


Would there be any benefit to caching the resource here, since it has been found and will likely be opened again.

Very unlikely to be worth it, and outside the scope of this work (since nothing I'm doing changes this behaviour).

ImageReader deals with small nodes and has a surprisingly low footprint. Caching content would require managing a size bounded cache, since it's well over 100MB of content.

david-beaumont added 4 commits September 23, 2025 21:02

Java ImageReader changes for preview mode

cc8d109

Clarifying flag docs for /packages/xxx case

96ee008

Add workaround until new image writing code is in

924698e

Word smithing flags definitions.

629ba8c

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 23, 2025

david-beaumont changed the base branch from lworld to pr/1618 September 23, 2025 19:22

openjdk bot removed the ready Pull request is ready to be integrated label Sep 23, 2025

david-beaumont mentioned this pull request Sep 23, 2025

8368467: [lworld] Add new flag generation for jimage to support preview mode #1621

Open

1 task

Manually deleting ImageReaderFactory (it returned somehow)

936b22d

RogerRiggs reviewed Sep 29, 2025

View reviewed changes

david-beaumont added 7 commits October 8, 2025 15:20

Test fixes and feedback changes.

c9989bd

* Renaming slightly confusing "testEncoder" method. * Fixing unit tests to use new constructor.

Feedback changes, and fixing some comments.

8838ad2

Changing existing package flags during writing to match altered flag …

2c0db3d

…values.

Removing package root flag based on feedback.

a230aa5

[[RESET BRANCH FOR MERGE]]

3197061

Merge branch 'jdk_8366093_cpp/squashed' into jdk_8368333_java/squashed

a1a9ac3

david-beaumont commented Oct 14, 2025

View reviewed changes

RogerRiggs reviewed Oct 21, 2025

View reviewed changes

		* Helper function to calculate preview flags (ATTRIBUTE_PREVIEW_FLAGS).
		*

		* Indicates that a non-preview location is associated with preview
		* resources.

		@@ -1,5 +1,5 @@
		/*
		* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.

8368333: [lworld] Add preview mode to ImageReader and JRT file-system #1619

Are you sure you want to change the base?

8368333: [lworld] Add preview mode to ImageReader and JRT file-system #1619

Uh oh!

Conversation

david-beaumont commented Sep 23, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Integration blocker

Issue

Reviewing

Uh oh!

david-beaumont commented Sep 23, 2025

Uh oh!

bridgekeeper bot commented Sep 23, 2025

Uh oh!

openjdk bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-beaumont Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-beaumont Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

david-beaumont commented Sep 23, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Sep 23, 2025 •

edited

Loading

mlbridge bot commented Sep 23, 2025 •

edited

Loading

david-beaumont Oct 8, 2025 •

edited

Loading

david-beaumont Oct 8, 2025 •

edited

Loading

david-beaumont Oct 22, 2025 •

edited

Loading