[lworld] RFR: 8371357: [lworld] Remove EnableValhalla
Coleen Phillimore
coleenp at openjdk.org
Fri Dec 12 16:21:40 UTC 2025
On Tue, 25 Nov 2025 13:37:46 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.
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 line 104:
> 102: assert_different_registers(obj, klass, len);
> 103:
> 104: // EnableValhalla legacy
I don't think this comment is necessary and kind of noisy everywhere, especially since there's Arguments::enable_preview() also tested. The weird thing is that EnableValhalla is true but Arguments::enable_preview() is not always true so I see why it's good to have testing with both settings, we were exercising valhalla code even with enable-preview off.
src/hotspot/share/cds/cdsConfig.hpp line 211:
> 209:
> 210: // EnablePreview legacy
> 211: static bool is_valhalla_preview() { return Arguments::enable_preview(); }
You might want to change all is_valhalla_preview() in CDS to just Arguments::enable_preview(). Or not. Not completely necessary I guess.
test/hotspot/jtreg/serviceability/jvmti/valhalla/HeapDump/HeapDump.java line 153:
> 151:
> 152: // -XX:+PrintInlineLayout is debug-only arg
> 153: LingeredApp.startApp(theApp, "--enable-preview", "-XX:+UnlockDiagnosticVMOptions",
Is there an if (debugvm) test that you need here?
-------------
PR Review: https://git.openjdk.org/valhalla/pull/1759#pullrequestreview-3568470576
PR Review Comment: https://git.openjdk.org/valhalla/pull/1759#discussion_r2611403488
PR Review Comment: https://git.openjdk.org/valhalla/pull/1759#discussion_r2611412940
PR Review Comment: https://git.openjdk.org/valhalla/pull/1759#discussion_r2611429239
More information about the valhalla-dev
mailing list