RFR: 8333791: Fix memory barriers for @Stable fields [v3]

[Aleksey Shipilev]

On Mon, 12 Aug 2024 10:04:50 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> See bug for more discussion.
>> 
>> Currently, C2 puts a `Release` barrier at exit of _every_ method that writes a `@Stable` field. This is a problem for high-performance code that initializes the stable field like this: https://github.com/openjdk/jdk/blob/79a23017fc7154738c375fbb12a997525c3bf9e7/src/java.base/share/classes/java/lang/Enum.java#L182-L193
>> 
>> A more egregious example is here, which means that every `String` constructor actually does `Release` barrier for `@Stable` field write, while only a `StoreStore` for `final` field store would suffice:
>> https://github.com/openjdk/jdk/blob/79a23017fc7154738c375fbb12a997525c3bf9e7/src/java.base/share/classes/java/lang/String.java#L159-L160
>> 
>> AFAICS, the original intent for Release barrier in constructor for stable fields was to match the memory semantics of final fields better. `@Stable` are in some sense "super-finals": they are foldable like static finals or non-static trusted finals, but can be written anywhere. The `@Stable` machinery is intrinsically safe under races: either a compiler sees a component of stable subgraph in initialized state and folds it, or it sees a default value for the component and leaves it alone.
>> 
>> I [performed an audit](https://bugs.openjdk.org/browse/JDK-8333791?focusedId=14688000&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14688000) of current `@Stable` uses for fields that are not currently `final` or `volatile`, and there are cases where we write into `@Stable` fields in constructors. AFAICS, they are covered by final-field-like semantics by accident of having adjacent `final` fields.
>> 
>> Current PR implements Variant 2 from the discussion: makes sure stable fields are as memory-safe as finals, and that's it. I believe this is all-around a good compromise for both mainline and the backports: the performance is improved in one the path that matter, and we still have some safety margin in face of accidental removals of adjacent `final`-s, or in case I missed some spots during the audit.
>> 
>> C1 did not do anything special for `@Stable` fields at all, fixed those to match C2. Both Zero and template interpreters for non-TSO arches put barriers at every `return` (with notable exception of [ARM32](https://bugs.openjdk.org/browse/JDK-8333957)), which handles everything in an overkill manner.
>> 
>> Additional testing:
>>  - [x] New IR tests
>>  - [x] Linux x86_64 server fastdebug, `all`
>>  - [x] Linux AArch64 server fastdebug, `all`
>>  - [x...
>
> Aleksey Shipilev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits:
> 
>  - Use TestFramework bootclasspath instead of develop option
>  - Merge branch 'master' into JDK-8333791-stable-field-barrier
>  - Merge branch 'master' into JDK-8333791-stable-field-barrier
>  - Merge branch 'master' into JDK-8333791-stable-field-barrier
>  - Merge branch 'master' into JDK-8333791-stable-field-barrier
>  - Variant 2: Only final-field like semantics for stable inits
>  - Variant 3: Handle everything, including reads by compilers

Awesome, thanks! Here it goes, then.

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

PR Comment: https://git.openjdk.org/jdk/pull/19635#issuecomment-2291101221