On Mon, 15 Dec 2025 19:46:03 GMT, Paul Hübner <[email protected]> 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
