[lworld] RFR: 8371357: [lworld] Remove EnableValhalla [v3]

[Daniel D. Daugherty]

On Mon, 15 Dec 2025 19:46:03 GMT, Paul Hübner <phubner at openjdk.org> wrote:

>> 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.
>
> Paul Hübner has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 23 commits:
> 
>  - Use the new function for the Parallel changes.
>  - Merge branch 'lworld' into JDK-8371357
>  - Remove wrong comment.
>  - Fix imports.
>  - Reviewer comments.
>  - Copyright.
>  - Revert "Don't synchronize on Double."
>    
>    This reverts commit cbf849c2ce6059af2b43b3dd8cb42445eab4df2b.
>  - Accidentally changed too many enable_preview.
>  - Make heap dump test require debug VM.
>  - Change to arguments.
>  - ... and 13 more: https://git.openjdk.org/valhalla/compare/8c0c190b...58fb031a

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

src/hotspot/share/cds/cdsConfig.hpp line 213:

> 211:     return Arguments::enable_preview() && EnableValhalla;
> 212:   }
> 213: 

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

src/hotspot/share/cds/filemap.hpp line 175:

> 173:   bool   _has_aot_linked_classes;       // Was the CDS archive created with -XX:+AOTClassLinking
> 174:   bool   _has_full_module_graph;        // Does this CDS archive contain the full archived module graph?
> 175:   bool   _has_valhalla_patched_classes; // Is this archived dumped with --enable-preview

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

src/hotspot/share/classfile/classFileParser.cpp line 6148:

> 6146: 
> 6147:   // Determining is the class allows tearing or not (default is not)
> 6148:   if (Arguments::is_valhalla_enabled() && !_access_flags.is_identity_class()) {

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

src/hotspot/share/oops/arrayOop.hpp line 59:

> 57:   // Given a type, return true if elements of that type must be aligned to 64-bit.
> 58:   static bool element_type_should_be_aligned(BasicType type) {
> 59:     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.

src/hotspot/share/oops/markWord.hpp line 134:

> 132:   // instance state
> 133:   static const int age_bits                       = 4;
> 134:   // prototype header bits (fast path instead of klass layout_helper)

Why drop the word `static` in the comment here?

src/hotspot/share/runtime/arguments.cpp line 4006:

> 4004:     FLAG_SET_DEFAULT(BytecodeVerificationRemote, true);
> 4005:   }
> 4006:   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()`?

-------------

Changes requested by dcubed (Committer).

PR Review: https://git.openjdk.org/valhalla/pull/1759#pullrequestreview-3580390993
PR Review Comment: https://git.openjdk.org/valhalla/pull/1759#discussion_r2621030493
PR Review Comment: https://git.openjdk.org/valhalla/pull/1759#discussion_r2621034967
PR Review Comment: https://git.openjdk.org/valhalla/pull/1759#discussion_r2621039257
PR Review Comment: https://git.openjdk.org/valhalla/pull/1759#discussion_r2621074984
PR Review Comment: https://git.openjdk.org/valhalla/pull/1759#discussion_r2621103988
PR Review Comment: https://git.openjdk.org/valhalla/pull/1759#discussion_r2621129557