[lworld] RFR: 8361352: [lworld] Weird interaction between OSR and value class instances

[Tobias Hartmann]

On Fri, 22 Aug 2025 14:07:16 GMT, Marc Chevalier <mchevalier at openjdk.org> 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