RFR: 8333791: Fix memory barriers for @Stable fields [v3]
Aleksey Shipilev
shade at openjdk.org
Mon Aug 12 10:04:50 UTC 2024
> 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] Linux AArch64 server fastdebug, jcstre...
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
-------------
Changes: https://git.openjdk.org/jdk/pull/19635/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19635&range=02
Stats: 1067 lines in 14 files changed: 1028 ins; 20 del; 19 mod
Patch: https://git.openjdk.org/jdk/pull/19635.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19635/head:pull/19635
PR: https://git.openjdk.org/jdk/pull/19635
More information about the hotspot-dev
mailing list