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

[Vladimir Ivanov]

On Mon, 22 Jul 2024 08:49:12 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 three commits:
> 
>  - 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/runtime/globals.hpp line 1997:

> 1995:           "Use a terrible hash function in order to generate many collisions.") \
> 1996:                                                                             \
> 1997:   develop(bool, RestrictStable, true,                                       \

What's the use case for the flag? Solely for testing purposes (since it's develop)? 
Alternatively, you could place the test classes on boot class path and enable test execution with product binaries.

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

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