8379148: [lworld] Wrapper class caches should not be used when --enable-preview by Arraying · Pull Request #2249 · openjdk/valhalla

Arraying · 2026-03-20T13:06:37Z

Hi all,

This change eliminates more box cache usages when running with --enable-preview:

The majority of the change is contributed by @xmas92 who disabled CDS loading an archived cache.
There's an autobox cache cast in C2 that needed to be disabled.

Notably, this change does not prevent the XXXCache's cache variables from being CDS archived. This is due to the fact that the infrastructure is common to preview and non-preview mode, and the code complexity is not justifiable when the cost is storing five nulls. Instead, it is ensured that the cache is never read from.

Testing: tiers 1-4.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8379148: [lworld] Wrapper class caches should not be used when --enable-preview (Bug - P3)

Reviewers

Matias Saavedra Silva (@matias9927 - Committer) ⚠️ Review applies to d7197572
Tobias Hartmann (@TobiHartmann - Committer) ⚠️ Review applies to ed2acbd7
Chen Liang (@liach - Committer)
Ioi Lam (@iklam - Committer)

Contributors

Axel Boldt-Christmas <aboldtch@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2249

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

Using diff file

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

Using Webrev

Link to Webrev Comment

Arraying · 2026-03-20T13:07:05Z

/contributor add @xmas92

bridgekeeper · 2026-03-20T13:07:39Z

👋 Welcome back phubner! 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 · 2026-03-20T13:08:49Z

@Arraying 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:

8379148: [lworld] Wrapper class caches should not be used when --enable-preview

Co-authored-by: Axel Boldt-Christmas <aboldtch@openjdk.org>
Reviewed-by: matsaave, thartmann, liach, iklam

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 23 new commits pushed to the lworld branch:

c089179: 8380902: [lworld] Type confusion in jvm.cpp functions
74a690d: 8381477: [lworld] [BACKOUT] C2 should fold prototype_header_offset loads
dd106e5: 8380666: [lworld] C1: Substitutability check does not correctly load klass with -XX:+UseCompactObjectHeaders
... and 20 more: https://git.openjdk.org/valhalla/compare/9681478ebe95f33a3230f7cff6732155446c4e90...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.

➡️ To integrate this PR with the above commit message to the lworld branch, type /integrate in a new comment.

openjdk · 2026-03-20T13:09:34Z

@Arraying
Contributor Axel Boldt-Christmas <aboldtch@openjdk.org> successfully added.

liach · 2026-03-20T22:40:53Z

What does this really solve? This doesn't identify the sites that incorrectly initialized these caches. This doesn't remove the C2 recognition of special wrapper arrays. Why do we need this?

Arraying · 2026-03-27T10:24:15Z

Thanks for your comments @liach.

What does this really solve?

The purpose of this PR is to clean up the remaining box cache changes that are necessary. Once the scope of this task is finalized and the PR goes out of draft, I will update the description to be more representative.

This doesn't identify the sites that incorrectly initialized these caches.

There are likely other usage sites that I have not overhauled. This is still a draft.

This doesn't remove the C2 recognition of special wrapper arrays.

We cannot remove C2's recognition of these arrays. The box caches are VM symbols, we can only restrict their usages from the VM's side by ensuring control flow does not use them and verifying this through assertions. One prominent usage is during deoptimization. Axel has already changed the VM code for this in JDK-8378531.

We still had underlying test failures, for instance JDK-8379863, which was a missing piece for this PR. Now that this has been integrated, things will look a lot clearer.

Why do we need this?

When it comes to boxed caches, we should not initialize from (nor write to) the CDS archives.

src/java.base/share/classes/java/lang/Byte.java

matias9927

So far, the design makes sense to me! The caches will still exist in the CDS archive because they will be in the default classlist, but minimizing their use in preview mode will help transition to its deprecation.

src/java.base/share/classes/java/lang/Byte.java

Arraying · 2026-03-30T12:38:09Z

Thanks for the feedback folks, I've RFR'd this PR and I'm re-running testing. Further comments/feedback welcome!

mlbridge · 2026-03-30T12:41:14Z

Webrevs

TobiHartmann

JIT changes look good to me.

liach · 2026-03-30T14:09:22Z

src/java.base/share/classes/java/lang/Byte.java

+        if (ByteCache.isEnabled()) {
            final int offset = 128;
-            return ByteCache.cache[(int) b + offset];
+            return ByteCache.cache[(int)b + offset];


I think the introduction of XxxCache.isEnabled and all the changes to valueOf methods are redundant.

Thanks for the input. Axel raised a point regarding this in #2249 (comment)

I don't see how that comment relates to my suggestion. Axel says PreviewFeatures.isEnabled can already reliably constant fold, so there is no cost restoring the old calls.

I agree if someone can confirm that the static final field jdk.internal.misc.PreviewFeatures.ENABLED is constant folded. My guess is that it should be because we have had a lot of JEPs restricting access to our internals, and there is JEP-500, however I do not know the details.

The only reason this was in my original patch is because I know that @stable with an @stable sentinel check will be constant folded (unless changing the diagnostic flag FoldStableValues) .

Axel says PreviewFeatures.isEnabled can already reliably constant fold

What I am trying to say is that I do not know if that is the case, but if that was the case we should just use it directly.

My original patch was just defensive with what I know works.

Good to know. So @Stable is only useful for non final fields.

Is the reason that the box caches cache fields are not final, (and @Stable instead)? Some AOT / CDS interaction?

There are some inconsistencies. For instance, Integer sees its cache as non-final, so I think a @Stable makes sense there given that the setup is performed outside of the static initializer. It's not immediately clear to me why this decision was made, but this is also how it is in mainline.

Moving forwards, I propose:

Ensure all cache variables are consistent with mainline and either static final or @Stable static.

If in preview, static final caches with null and @Stable static caches with an empty array.

Remove isEnabled.

Revert changes in valueOf, i.e., call PreviewFeatures.

Thoughts?

On mainline Integer's cache is not final because the archived data may be invalid and need to be thrown away if a system property is changed. (This has always been the biggest headache with AOT)

@xmas92 I think only IntegerCache.cache is non-final @Stable. Others seem to be final @Stable. A final @Stable is meaningful for arrays because a read of such an array element is considered a compiler constant if it is non-zero/null.

If in preview, static final caches with null and @Stable static caches with an empty array.

If we do not touch the cache array at all in preview, we can leave that array null because it won't be touched by code and thus won't be dealt by JIT.

On mainline Integer's cache is not final because the archived data may be invalid and need to be thrown away if a system property is changed. (This has always been the biggest headache with AOT)

Right so the cache can have an unstable @stable non-null value. Scary.

Will be nice when Integer lose their identity and we do not have to worry about these sort of things.

@Stable is meaningful for arrays because a read of such an array element is considered a compiler constant if it is non-zero/null.

Did not know that the @Stable property was not shallow. Thanks for the information!

matias9927

Updates look good

matias9927 · 2026-03-30T17:10:19Z

src/hotspot/share/cds/heapShared.cpp

-  {"java/lang/Byte$ByteCache",                    "archivedCache"},
-  {"java/lang/Short$ShortCache",                  "archivedCache"},
-  {"java/lang/Character$CharacterCache",          "archivedCache"},
+  {"java/lang/Integer$IntegerCache",              _archived_cache_name},


I don't think this change is necessary, we should avoid unneeded diffs if possible.

It's used to re-use _archived_cache_name in the new assertion later on. Would you like me to revert this assertion?

Or should I use the "archivedCache" literal twice?

I think you should just use the string literal instead of _archived_cache_name to mimimize the diff to just the assert you add later.

matias9927 · 2026-03-31T14:34:25Z

src/hotspot/share/cds/heapShared.cpp

-  {"java/lang/Byte$ByteCache",                    "archivedCache"},
-  {"java/lang/Short$ShortCache",                  "archivedCache"},
-  {"java/lang/Character$CharacterCache",          "archivedCache"},
+  {"java/lang/Integer$IntegerCache",              _archived_cache_name},


I think you should just use the string literal instead of _archived_cache_name to mimimize the diff to just the assert you add later.

Arraying · 2026-03-31T16:55:54Z

Thanks for the feedback everyone, I've incorporated it and I'm re-running testing as we speak.

matias9927

Updates look good, thanks!

iklam · 2026-03-31T19:45:24Z

src/hotspot/share/cds/heapShared.cpp

+    // With preview mode enabled, the boxed class caches should all be null.
+    assert(strcmp("archivedCache", field_name) != 0 ||
+           !Arguments::is_valhalla_enabled(),
+           "field %s must be null when using Valhalla preview features", field_name);


I think this assert should be removed. Even though the field name archivedCache is currently used only for the number classes, this just happens to be a coincident.

iklam

LGTM

Arraying · 2026-04-02T08:06:25Z

Since HS RT & JIT is happy, @liach are you content enough with the Java changes?

liach

Oops, seems I forgot to leave an approval

Arraying · 2026-04-02T12:18:14Z

Thanks everyone for the extensive feedback!
/integrate

openjdk · 2026-04-02T12:19:48Z

Going to push as commit 897f6db.
Since your change was applied there have been 23 commits pushed to the lworld branch:

c089179: 8380902: [lworld] Type confusion in jvm.cpp functions
74a690d: 8381477: [lworld] [BACKOUT] C2 should fold prototype_header_offset loads
dd106e5: 8380666: [lworld] C1: Substitutability check does not correctly load klass with -XX:+UseCompactObjectHeaders
... and 20 more: https://git.openjdk.org/valhalla/compare/9681478ebe95f33a3230f7cff6732155446c4e90...lworld

Your commit was automatically rebased without conflicts.

openjdk · 2026-04-02T12:19:57Z

@Arraying Pushed as commit 897f6db.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Re-commit Axel's patch.

ee153d2

openjdk bot changed the title ~~8379148~~ 8379148: [lworld] Wrapper class caches should not be used when --enable-preview Mar 20, 2026

Arraying added 2 commits March 27, 2026 12:08

Merge branch 'lworld' into JDK-8379148

845cf15

Skip JVMCI autobox cache if preview is enabled.

0b9248d

iklam reviewed Mar 27, 2026

View reviewed changes

src/java.base/share/classes/java/lang/Byte.java Outdated Show resolved Hide resolved

matias9927 reviewed Mar 27, 2026

View reviewed changes

src/java.base/share/classes/java/lang/Byte.java Outdated Show resolved Hide resolved

Arraying added 4 commits March 30, 2026 10:32

Clean up autobox check coce.

dd0fbb6

Merge branch 'lworld' into JDK-8379148

addc49e

Reviewer feedback on Java code cleanup.

6b732f6

Harden CDS dumping with HotSpot assertion.

0441654

Arraying marked this pull request as ready for review March 30, 2026 12:35

openjdk bot added the rfr Pull request is ready for review label Mar 30, 2026

Move autobox check to ciField.

ed2acbd

TobiHartmann approved these changes Mar 30, 2026

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 30, 2026

liach suggested changes Mar 30, 2026

View reviewed changes

matias9927 suggested changes Mar 30, 2026

View reviewed changes

matias9927 suggested changes Mar 31, 2026

View reviewed changes

Reviewer feedback.

d719757

matias9927 approved these changes Mar 31, 2026

View reviewed changes

iklam suggested changes Mar 31, 2026

View reviewed changes

Remove assert.

838c6a7

iklam approved these changes Apr 1, 2026

View reviewed changes

liach approved these changes Apr 2, 2026

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 2, 2026

openjdk bot closed this Apr 2, 2026

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 2, 2026

Conversation

Arraying commented Mar 20, 2026 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

Arraying commented Mar 20, 2026

Uh oh!

bridgekeeper bot commented Mar 20, 2026

Uh oh!

openjdk bot commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 20, 2026

Uh oh!

liach commented Mar 20, 2026

Uh oh!

Arraying commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

matias9927 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Arraying commented Mar 30, 2026

Uh oh!

mlbridge bot commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TobiHartmann 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!

xmas92 Mar 30, 2026 • 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!

matias9927 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!

Arraying commented Mar 31, 2026

Uh oh!

matias9927 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iklam left a comment

Arraying commented Mar 20, 2026 •

edited by openjdk bot

Loading

openjdk bot commented Mar 20, 2026 •

edited

Loading

Arraying commented Mar 27, 2026 •

edited

Loading

mlbridge bot commented Mar 30, 2026 •

edited

Loading

xmas92 Mar 30, 2026 •

edited

Loading