On Fri, 22 Aug 2025 14:07:16 GMT, Marc Chevalier <[email protected]> wrote:
> First, credit to @TobiHartmann for the diagnostic, and a lot of the solution. > > # Diagnostic > > According to [Strict Field Initialization > JEP](https://openjdk.org/jeps/8350458), when a strict field is being > initialized, it is not quite immutable, but observally immutable: at first, > the field can be only set (during the early larval phase), then it can be > only read (late larval or initialized phase), so the last set is the actual > value one can ever observe. > > The interesting part is that in early larval phase, a field can be subject to > some side effects. When applied to a value object, that means that until it > reaches the unrestricted state, it is not yet immutable. While being not > theoretically necessary, avoiding scalarization and keeping the value object > behind a reference is a convenient way to make sure that side effects are > correctly applied. This strategy means that we shouldn't scalarized before > reaching the unrestricted state. Normally, in C2, finding out what is early > larval or not is the job of bytecode parsing, but in OSR compilation, > everything about the StartOSR is not parsed, and thus some objects are > soundly assumed that they might be larval, when they actually aren't. In the > reported example, that leads to drastic performance difference between OSR > and non OSR compilation: the second one is able to eliminate allocations > since it knows more precisely when the value object can be scalarized. > > In the original example: > > public value class MyNumber { > private long d0; > private MyNumber(long d0) { this.d0 = d0; } > public MyNumber add(long v) { return new MyNumber(d0 + v); } > > private static void loop() { > MyNumber dec = new MyNumber(123); > for (int i = 0; i < 1_000_000_000; ++i) { > dec = dec.add(i); > } > } > > public static void main(String[] args) { > for (int i = 0; i < 10; ++i) { > loop(); > } > } > } > > OSR happens in the loop in `loop`, but here `dec` is not detected to be > unrestricted (so immutable, so scalarizable), so the allocation in inlined > `add` still needs to happen because we need the buffer for the new `dec`. The > first iteration traps at the exit of the loop (unstable if), OSR happens > again, followed by a non-OSR compilation, finding correctly that `dec` can be > scalarized in the loop, making the third iteration fast. > > # Solution > > Overall, the solution requires to improve our detection of early larval > values. Since we keep parsing as-is, let's do that in `ciTypeFlow`. Th... Thanks for working on this Marc. Analysis and fix look good. I just added a few minor comments. src/hotspot/share/ci/ciType.cpp line 151: > 149: assert(type->is_inlinetype() > 150: // An abstract value type is an instance_klass > 151: || (type->is_instance_klass() && > type->as_instance_klass()->flags().is_abstract() && > !type->as_instance_klass()->flags().is_identity()) Just wondering, is the `type->as_instance_klass()->flags().is_abstract()` condition even needed? Same in `ciTypeFlow::get_start_state()`. test/hotspot/jtreg/compiler/valhalla/inlinetypes/LarvalDetectionAboveOSR.java line 29: > 27: * @summary In OSR compilation, value objects coming from above the OSR > start > 28: * must be correctly found to be early larval so we know they are > 29: * immutable, and they can be scalarized. I suggest to rephrase this: "to be early larval so we know they are immutable" is confusing because early larvals are **mutable**. test/micro/org/openjdk/bench/valhalla/loops/osr/LarvalDetectionAboveOSR.java line 1: > 1: package org.openjdk.bench.valhalla.loops.osr; Copyright header is missing. ------------- Marked as reviewed by thartmann (Committer). PR Review: https://git.openjdk.org/valhalla/pull/1531#pullrequestreview-3150178417 PR Review Comment: https://git.openjdk.org/valhalla/pull/1531#discussion_r2297313229 PR Review Comment: https://git.openjdk.org/valhalla/pull/1531#discussion_r2297293768 PR Review Comment: https://git.openjdk.org/valhalla/pull/1531#discussion_r2297289457
