8366093: [lworld] Add preview mode to C++ classloader #1618

david-beaumont · 2025-09-23T19:11:41Z

C++ 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 being zero (which matches location flags for "normal" entries).

Progress

Change must not contain extraneous whitespace

Issue

JDK-8366093: [lworld] Add preview mode to C++ classloader (Sub-task - P4)

Reviewers

Paul Hübner (@Arraying - Author)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1618

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

Using diff file

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

Using Webrev

Link to Webrev Comment

david-beaumont · 2025-09-23T19:12:35Z

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

bridgekeeper · 2025-09-23T19:13:28Z

👋 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:14:39Z

@david-beaumont This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8366093: [lworld] Add preview mode to C++ classloader

Reviewed-by: phubner

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 654 new commits pushed to the lworld branch:

84c7c0a: DEFAULT_PROMOTED_VERSION_PRE=jep401ea2
693a507: 8369446: [lworld] Problem list LoggingDeadlock2
dd39de4: 8368795: [lworld] javac rejects assignments to components of array fields with initializers in the prologue
... and 651 more: https://git.openjdk.org/valhalla/compare/b22f6a476ec1f8d52845ba416ca74e5f573d7ae4...lworld

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

mlbridge · 2025-09-23T19:18:39Z

Webrevs

00: Full (9ca6c473)

RogerRiggs · 2025-09-29T01:41:02Z

src/java.base/share/native/libjimage/imageFile.hpp

+    enum {
+        // Set on a "normal" (non-preview) location if a preview version of
+        // it exists in the same module.
+        FLAGS_HAS_PREVIEW_VERSION = 0x1,


To avoid confusion, the value and spelling of the flag should agree across all modules.
In ImageLocation and jimage/ModulesReference.

ModulesReference are different flags, with different semantics. That's why I made sure they were private and provided helper methods for reading/writing the package directory offsets (because I did trip over using the wrong constants at one point).

I agree it's weird/annoying to have this similar-but-different set of flags, but the ModuleReference flags are completely encapsulated in one place, so cannot be accidentally confused with, or misused in place of the ImageLocation flags.

These flags have a requirement of being zero for almost all entries to reduce image file size, while the ModulesReference flags need to additive for merging (and are not stored in the same part of the jimage file).

I could change flag ordering to make the values of the "has preview version" flags match, but since these sets of flags must never be mistaken for each other, I'm not sure that's beneficial (if anything it might foster the idea that they can be used interchangeably in some way).

If they have the same spelling, they will cause confusion if they have different semantics and usages.
All of imageFile, jimage, and the file system provider and classloader are in a single scope of operation.

I've done this now (but I don't really agree with the argument that it will be likely to cause confusion however, since the flags are stored in different data structures and their use is carefully encapsulated).

RogerRiggs · 2025-09-29T01:50:01Z

src/java.base/share/native/libjimage/imageFile.hpp

+    // Flag masks for the ATTRIBUTE_PREVIEW_FLAGS attribute. Defined so
+    // that zero is the overwhelmingly common case for normal resources.
+    enum {
+        // Set on a "normal" (non-preview) location if a preview version of
+        // it exists in the same module.
+        FLAGS_HAS_PREVIEW_VERSION = 0x1,
+        // Set on all preview locations in "/modules/xxx/META-INF/preview/..."
+        FLAGS_IS_PREVIEW_VERSION = 0x2,
+        // Set on a preview location if no normal (non-preview) version of
+        // it exists in the same module.
+        FLAGS_IS_PREVIEW_ONLY = 0x4
+    };


A refactoring of the flags can avoid conflicting states.
FLAGS_IS_PREVIEW_VERSION = 2; // set for preview version
FLAGS_NO_NORMAL_VERSION = 1; // preserving the zero value for normal and no preview.

Create static methods in ImageLocation to perform the desired tests on flags.
The utility methods make the code easier to read.
Also define macros/function in the native code in ImageFile and jimage.

isPreviewVersion(flags) { return (flags & FLAGS_IS_PREVIEW_VERSION) != 0)}
isPreviewOnly(flags) { return (flags & FLAGS_IS_REVIEW_VERSION | FLAGS_NO_NORMAL_VERSION) == FLAGS_NO_NORMAL_VERSION}

Reprise:

Your name FLAGS_IS_PREVIEW_ONLY probably better than FLAGS_NO_NORMAL_VERSION and has the same semantics.

FLAGS_HAS_PREVIEW_VERSION means that there is a preview version and it will be found in the <META-INF/Preview> hierarchy. if the flag is in the preview node then this is it. If found in the normal hierarchy, the preview class is found in the <META-INF/Preview> hierarchy.

The FLAGS_HAS_PREVIEW_VERSION flag is only set on non-preview locations. It's what indicates that you may need to look for a preview version in preview mode (rare but possible).

For C++ I think this occasional 2nd hash calculation and lookup shouldn't be an issue because it's for class loading, so each successful lookup triggers a lot of additional work (and as I understand it, failed lookups for the preview-only cases should not be common in this code -- as opposed to the JRT file system code).

There are several possible approaches to address this if it does turn out to be a performance issue though (e.g. making preview string hashes a simple modification of non-preview string hashes).

RogerRiggs · 2025-10-09T19:41:20Z

src/java.base/share/native/libjimage/jimage.cpp

+    size_t name_len = strlen(name);
+    size_t preview_infix_len = strlen(preview_infix);
+
+    // TBD:   assert(module_name_len > 0 && "module name must be non-empty");


TBD: is obsolete?

I don't use multiple white-space, so this wasn't me. It's just line 113 in the old code moved.
I don't know enough to change it confidently.

RogerRiggs · 2025-10-09T19:42:51Z

src/java.base/share/native/libjimage/jimage.cpp

+    // Regardless of preview mode, don't return resources requested directly
+    // via their preview path.
+    if ((flags & ImageLocation::FLAGS_IS_PREVIEW_VERSION) != 0) {
        return 0L;
    }


How can this occur?
classloader is the only client and does not pass arbitrary paths.

Hopefully it can't, but I'm not taking chances.

RogerRiggs · 2025-10-09T19:45:00Z

src/java.base/share/native/libjimage/jimage.cpp

+    u4 flags = loc.get_preview_flags();
+    // No preview flags means "a normal resource, without a preview version".
+    // This is the overwhelmingly common case, with or without preview mode.
+    if (flags == 0) {


Should test for defined flags, ignoring bits outside of the defined bits.

No, this really needs to be zero in this case, because otherwise it will be adding a lot of space to the jimage file for all the attributes that need to be encoded (see comment on line 248 in imageFile.hpp).

RogerRiggs · 2025-10-09T19:51:54Z

src/java.base/share/native/libjimage/imageFile.hpp

+    enum {
+        // Set on a "normal" (non-preview) location if a preview version of
+        // it exists in the same module.
+        FLAGS_HAS_PREVIEW_VERSION = 0x1,


If they have the same spelling, they will cause confusion if they have different semantics and usages.
All of imageFile, jimage, and the file system provider and classloader are in a single scope of operation.

Arraying · 2025-10-10T08:54:21Z

src/hotspot/share/runtime/arguments.cpp

+#define DISABLE_PREVIEW_PATCHING_DEFAULT false
+
+bool Arguments::disable_preview_patching() {
+  const char* prop = get_property("DISABLE_PREVIEW_PATCHING");


Nitpick, feel free to ignore, esp. since this is temporary. I think it would make the code easier to follow if this was enable preview patching, defaulting to true. It stands in contrast toEnableValhalla and enable_preview(). But as said, not a huge thing.

The naming was Roger's, I'm mostly happy with anything really if it's temporary.

I have a similar preference for positive naming ("enable" rather than "disable") to avoid double negatives, but in this case it's probably more work to do that (since it'll cause a rebase of 3 other PRs in progress) than just live with it for a few weeks.

Arraying

Disclaimer: I can only speak for the runtime portions of the code.

Arraying · 2025-10-10T08:56:59Z

src/hotspot/share/classfile/classLoader.cpp

+
+// Closes and clears the JImage file reference (this will only be called during shutdown).
+static void jimage_close() {
+  if (JImage_file != nullptr) {


I'm not familiar with jimage at all, is there as reason we could close a non-open jimage? And following from that, can we guarantee to not call jimage_open on an open jimage? (Maybe an assert should guard it?)

I honestly don't know, so this is the old behaviour, so I'm keeping it for now.
Alan might know more.

Arraying · 2025-10-10T08:58:34Z

src/hotspot/share/classfile/classLoader.cpp

+// Looks up the location of a named JImage resource. This "raw" lookup function allows
+// the preview mode to be manually specified, so must not be accessible outside this
+// class. ClassPathImageEntry manages all calls for resources after startup is complete.
+static JImageLocationRef jimage_find_resource(const char* module_name,


This function makes the rest of the code more legible. Thanks!

Arraying · 2025-10-10T08:59:38Z

src/java.base/share/native/libjimage/imageFile.cpp

+        // be read so this code can be committed before image writing changes
+        // for preview mode. Preview mode changes do not modify any structure,
+        // so a 1.0 file will look like a jimage without any preview resources.
+        // TODO: Restore equality check for MINOR_VERSION.


Is this something you still have to address in this PR?

I need to address it later, when the image writing code is changed.

C++ preparation for preview mode

9ca6c47

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

david-beaumont mentioned this pull request Sep 23, 2025

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

Open

1 task

RogerRiggs reviewed Sep 29, 2025

View reviewed changes

RogerRiggs reviewed Oct 9, 2025

View reviewed changes

Arraying reviewed Oct 10, 2025

View reviewed changes

Arraying approved these changes Oct 10, 2025

View reviewed changes

8366093: [lworld] Add preview mode to C++ classloader #1618

Are you sure you want to change the base?

8366093: [lworld] Add preview mode to C++ classloader #1618

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

Issue

Reviewers

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

Webrevs

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!

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 9, 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!

Arraying left a comment

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

david-beaumont commented Sep 23, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Sep 23, 2025 •

edited

Loading

david-beaumont Oct 8, 2025 •

edited

Loading

david-beaumont Oct 8, 2025 •

edited

Loading

david-beaumont Oct 9, 2025 •

edited

Loading