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

[Quan Anh Mai]

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

src/hotspot/share/c1/c1_GraphBuilder.cpp line 1752:

> 1750:       scope()->set_wrote_final();
> 1751:     }
> 1752:     if (field->is_stable()) {

What if the `field` is a field of another object, not the one in construction? For final fields the verifier ensures that we only write to it in the containing object constructor, but for final fields it is not guaranteed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19635#discussion_r1719282291