8371357: [lworld] Remove EnableValhalla #1759

Arraying · 2025-11-25T13:37:46Z

Hi all,

This removes the EnableValhalla in favour of the --enable-preview flag. Concretely:

I've replaced most of the EnableValhalla checks with Arguments::is_valhalla_enabled().
Some checks were redundant and could be removed entirely.
I've made the EnableValhalla flag obsolete.
Some tests had to be updated.

This greatly changes the semantics of tests. I've refined some test groups to make it easier.

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-8371357: [lworld] Remove EnableValhalla (Bug - P4)

Reviewers

Coleen Phillimore (@coleenp - Committer) ⚠️ Review applies to 71f120ef
Stefan Karlsson (@stefank - Author)
Daniel D. Daugherty (@dcubed-ojdk - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1759

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-11-25T13:39:47Z

👋 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 · 2025-11-25T13:40:32Z

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

8371357: [lworld] Remove EnableValhalla

Reviewed-by: coleenp, stefank, dcubed

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

26b7640: 8369043: [lworld] -XX:AOTMode=create crashes with UseCompactObjectHeaders on aarch64
df1aa75: 8372740: [lworld] tools/javap/4870651/T4870651.java fails with --enable-preview
e82573e: 8373787: [lworld] InstanceKlass::size mixes bytes and words
... and 1 more: https://git.openjdk.org/valhalla/compare/ef301b763d03e612be7ad34b49b84f39fd7ce287...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 · 2025-12-11T13:42:08Z

@Arraying this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8371357
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

coleenp

Except for EnableValhalla legacy comments that I think should be removed, this looks reasonable on it's own. Does it pass our existing tier1-4 testing without updating problem lists ?

src/hotspot/cpu/aarch64/c1_MacroAssembler_aarch64.cpp

src/hotspot/share/cds/cdsConfig.hpp

test/hotspot/jtreg/serviceability/jvmti/valhalla/HeapDump/HeapDump.java

stefank

Not a full review, but I think this looks good. If this is accepted you'll have to add includes of arguments.hpp to all places where you have added a call to is_valhalla_enabled().

src/hotspot/share/runtime/arguments.cpp

mlbridge · 2025-12-12T16:21:36Z

Webrevs

dholmes-ora · 2025-12-15T06:42:45Z

is_valhalla_enabled

As Ioi suggested in the mainline argument.hpp PR shouldn't this be something more specific to what is actually being enabled in this preview release e.g. valueTypesConfig.hpp which defines value_types_enabled or some such? IIUC for this release we are talking about JEP-401 features that are opted into when --enable-preview is added. But for the next release I assume there could be additional JEPs with 401 still also in preview - so is_valhalla_enabled is somewhat vague and doesn't identify what aspect of Valhalla a piece of code relates to.

Arraying · 2025-12-15T08:54:19Z

is_valhalla_enabled

As Ioi suggested in the mainline argument.hpp PR shouldn't this be something more specific to what is actually being enabled in this preview release e.g. valueTypesConfig.hpp which defines value_types_enabled or some such? IIUC for this release we are talking about JEP-401 features that are opted into when --enable-preview is added. But for the next release I assume there could be additional JEPs with 401 still also in preview - so is_valhalla_enabled is somewhat vague and doesn't identify what aspect of Valhalla a piece of code relates to.

is_valhalla_enabled currently guards JEP 401 and JEP draft strict field init. It's supposed to be a drop-in replacement for --enable-preview such that when reading code, it becomes clear that this is a Valhalla-related feature (and not some special EP handling in e.g., AOT). I agree that this may become ambiguous if there are multiple Valhalla JEPs in preview that offer distinct language-level features. I am moving the function to its own config, I'll see what I can do.

coleenp

This seems fine to me with or without a one line include file for is_valhalla_enabled() to test Arguments::enable_preview() or any other change for future preview or valhalla related features. I think we can fix that for the specific situations when it happens.

stefank

Looks good minus one nit.

stefank · 2025-12-15T14:48:48Z

src/hotspot/share/opto/callnode.cpp

    Node* proto_adr = phase->transform(new AddPNode(klass_node, klass_node, phase->MakeConX(in_bytes(Klass::prototype_header_offset()))));
    mark_node = LoadNode::make(*phase, control, mem, proto_adr, TypeRawPtr::BOTTOM, TypeX_X, TypeX_X->basic_type(), MemNode::unordered);
-    if (EnableValhalla) {
+    // EnavleValhalla legacy


Suggested change

// EnavleValhalla legacy

Good shout, I missed that thanks.

Arraying · 2025-12-15T16:43:00Z

Thanks for the feedback, everyone. After a face-to-face discussion, we decided this shape (without explicit feature granularity or its own file) is sufficient for our current needs. Sole --enable-preview for Valhalla is a large change that will be delivered incrementally. This is only the start, and many subsequent changes depend on this change. I've filed a follow-up RFE https://bugs.openjdk.org/browse/JDK-8373694, where we can devote more (non-blocking) time and discussion towards how a better way of checking may look. Ideally, @stefank's mainline changes will have made it into lworld by then too.

dcubed-ojdk

Generally looks good. I posted one comment where I think a logical expression is backwards.

dcubed-ojdk · 2025-12-15T22:11:25Z

src/hotspot/share/cds/cdsConfig.hpp

-  static bool is_valhalla_preview() {
-    return Arguments::enable_preview() && EnableValhalla;
-  }
-


Does this file still need to include arguments.hpp? I'm not seeing anymore calls into Arguments::foo.

Nope, I'll get rid of it.

dcubed-ojdk · 2025-12-15T22:13:37Z

src/hotspot/share/cds/filemap.hpp

-  bool   _has_valhalla_patched_classes; // Is this archived dumped with --enable-preview -XX:+EnableValhalla?
+  bool   _has_valhalla_patched_classes; // Is this archived dumped with --enable-preview


Nit: dropped the ? at the end of the comment.

dcubed-ojdk · 2025-12-15T22:15:37Z

src/hotspot/share/classfile/classFileParser.cpp

  // Determining is the class allows tearing or not (default is not)
-  if (EnableValhalla && !_access_flags.is_identity_class()) {
+  if (Arguments::is_valhalla_enabled() && !_access_flags.is_identity_class()) {


Typo: L6147 (baseline): s/is the/if the/

dcubed-ojdk · 2025-12-15T22:30:27Z

src/hotspot/share/oops/arrayOop.hpp

  // Given a type, return true if elements of that type must be aligned to 64-bit.
  static bool element_type_should_be_aligned(BasicType type) {
-    if (EnableValhalla && type == T_FLAT_ELEMENT) {
+    if (type == T_FLAT_ELEMENT) {


You don't need EnableValhalla or Arguments::is_valhalla_enabled() as long as all callers
of element_type_should_be_aligned() only pass known valid BasicType values. If any caller
passes a non-valid BasicType value as part of a "probing call", then a call with a value that
happens to match T_FLAT_ELEMENT by a VM with Arguments::is_valhalla_enabled() == false
will return true unexpectedly.

I think this should be fine because T_FLAT_ELEMENT is hardcoded to 15 regardless of if Valhalla/preview is enabled.

Testing until tier 4 also shows no issue, so I think I'm confident enough to leave it as is. Unless someone from compiler has any insights?

FWIW, a follow-up could add an assert in this branch that we are indeed running with value classes.

dcubed-ojdk · 2025-12-15T22:45:46Z

src/hotspot/share/oops/markWord.hpp

  // instance state
  static const int age_bits                       = 4;
-  // EnableValhalla: static prototype header bits (fast path instead of klass layout_helper)
+  // prototype header bits (fast path instead of klass layout_helper)


Why drop the word static in the comment here?

Static prototypes were part of an older model that no longer exists.

dcubed-ojdk · 2025-12-15T23:00:21Z

src/hotspot/share/runtime/arguments.cpp

-  if (!EnableValhalla || (is_interpreter_only() && !CDSConfig::is_dumping_archive() && !UseSharedSpaces)) {
+  if (is_valhalla_enabled() || (is_interpreter_only() && !CDSConfig::is_dumping_archive() && !UseSharedSpaces)) {


Hmmm... The original logic is !EnableValhalla so shouldn't this be: !is_valhalla_enabled()?

You're right. Good spot!

jsikstro · 2025-12-16T09:25:37Z

Now that we're not using EnableValhalla, which was in runtime/globals.hpp, there are several includes of runtime/globals.hpp which have been added over the years in the lworld branch that don't need to be added anymore.

I don't see an issue with including runtime/globals.hpp unnecessarily, just pointing it out.

Arraying · 2025-12-16T10:13:49Z

Now that we're not using EnableValhalla, which was in runtime/globals.hpp, there are several includes of runtime/globals.hpp which have been added over the years in the lworld branch that don't need to be added anymore.

I don't see an issue with including runtime/globals.hpp unnecessarily, just pointing it out.

Good idea. I've added it to the follow up RFE where we are doing cleanups already.

dcubed-ojdk · 2025-12-16T18:47:54Z

I re-reviewed the changes from v02 -> v05 and they look good.

dcubed-ojdk

Thumbs up.

Arraying · 2025-12-17T10:48:15Z

Thanks all for the diligent feedback and reviews.
/integrate

openjdk · 2025-12-17T10:49:48Z

Going to push as commit 59a270d.
Since your change was applied there have been 7 commits pushed to the lworld branch:

1077e4f: 8373806: [lworld] Disable preview patching by default
d408d01: 8373779: [lworld] Type confusion with InlineKlassFixedBlock fields
5f0a81e: 8373701: [lworld] Revert Use Objects.equals in HashMap and ConcurrentHashmap
... and 4 more: https://git.openjdk.org/valhalla/compare/ef301b763d03e612be7ad34b49b84f39fd7ce287...lworld

Your commit was automatically rebased without conflicts.

openjdk · 2025-12-17T10:50:00Z

@Arraying Pushed as commit 59a270d.

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

openjdk bot changed the title ~~8371357~~ 8371357: [lworld] Remove EnableValhalla Nov 25, 2025

Arraying force-pushed the JDK-8371357 branch 3 times, most recently from a44154d to d671584 Compare November 28, 2025 16:14

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Dec 11, 2025

Arraying force-pushed the JDK-8371357 branch from 75f5532 to fb5641a Compare December 11, 2025 13:51

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Dec 11, 2025

coleenp reviewed Dec 11, 2025

View reviewed changes

src/hotspot/cpu/aarch64/c1_MacroAssembler_aarch64.cpp Outdated Show resolved Hide resolved

src/hotspot/share/cds/cdsConfig.hpp Outdated Show resolved Hide resolved

test/hotspot/jtreg/serviceability/jvmti/valhalla/HeapDump/HeapDump.java Show resolved Hide resolved

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Dec 12, 2025

Arraying added 15 commits December 12, 2025 15:16

Remove EnableValhalla from tests.

504213e

Runtime batch.

996f6cd

Compiler batch.

96ac8ee

Remove EnableValhalla flag.

e1b62c4

Add svc no_valhalla test group.

4ee0bcb

Add include for precompiled headers.

11f1a12

Add diagnostic flag to tests.

fe54fe5

Don't synchronize on Double.

cbf849c

New test groups.

0a32b56

Explode JDK groups and problemlist tests.

b89407b

Reset problem lists.

5198ae9

Remove unused file.

bc88eec

Use is_valhalla_enabled.

0ba0761

Change to arguments.

8296aad

Make heap dump test require debug VM.

624b2c6

Arraying force-pushed the JDK-8371357 branch from f5b6316 to 624b2c6 Compare December 12, 2025 15:17

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Dec 12, 2025

stefank reviewed Dec 12, 2025

View reviewed changes

src/hotspot/share/runtime/arguments.cpp Outdated Show resolved Hide resolved

stefank mentioned this pull request Dec 12, 2025

8373599: Cleanup arguments.hpp includes openjdk/jdk#28791

Closed

3 tasks

Fix imports.

f615a87

coleenp approved these changes Dec 15, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 15, 2025

stefank approved these changes Dec 15, 2025

View reviewed changes

Remove wrong comment.

71f120e

coleenp approved these changes Dec 15, 2025

View reviewed changes

Arraying added 2 commits December 15, 2025 20:18

Merge branch 'lworld' into JDK-8371357

32a4c59

Use the new function for the Parallel changes.

58fb031

stefank approved these changes Dec 15, 2025

View reviewed changes

dcubed-ojdk suggested changes Dec 15, 2025

View reviewed changes

Arraying added 3 commits December 16, 2025 11:14

Reviewer comments.

e6c091f

Merge branch 'lworld' into JDK-8371357

b5612ea

Merge branch 'lworld' into JDK-8371357

6eae244

stefank approved these changes Dec 16, 2025

View reviewed changes

dcubed-ojdk approved these changes Dec 16, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 17, 2025

openjdk bot closed this Dec 17, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 17, 2025

		bool _has_valhalla_patched_classes; // Is this archived dumped with --enable-preview -XX:+EnableValhalla?
		bool _has_valhalla_patched_classes; // Is this archived dumped with --enable-preview

		if (!EnableValhalla \|\| (is_interpreter_only() && !CDSConfig::is_dumping_archive() && !UseSharedSpaces)) {
		if (is_valhalla_enabled() \|\| (is_interpreter_only() && !CDSConfig::is_dumping_archive() && !UseSharedSpaces)) {

8371357: [lworld] Remove EnableValhalla #1759

8371357: [lworld] Remove EnableValhalla #1759

Uh oh!

Conversation

Arraying commented Nov 25, 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!

bridgekeeper bot commented Nov 25, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Dec 11, 2025

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefank left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

dholmes-ora commented Dec 15, 2025

Uh oh!

Arraying commented Dec 15, 2025

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

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

Arraying commented Dec 15, 2025

Uh oh!

dcubed-ojdk 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!

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!

jsikstro commented Dec 16, 2025

Uh oh!

Arraying commented Dec 16, 2025

Uh oh!

dcubed-ojdk commented Dec 16, 2025

Arraying commented Nov 25, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Nov 25, 2025 •

edited

Loading

mlbridge bot commented Dec 12, 2025 •

edited

Loading