RFR: 8333791: Fix memory barriers for @Stable fields

[Hans Boehm]

> Message: 1
> Date: Thu, 11 Jul 2024 08:50:57 GMT
> From: John R Rose <jrose at openjdk.org>
> To: <hotspot-dev at openjdk.org>
> Subject: Re: RFR: 8333791: Fix memory barriers for @Stable fields
> Message-ID:
>         <pfFWmbs1q_M-WQIDyBw15ctVdRcAudSrdJ6BEQRx41E=.
762c100f-7650-47fd-bfe3-ac620913384f at github.com>
>
> Content-Type: text/plain; charset=utf-8
>
> On Mon, 10 Jun 2024 18:05:09 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`
>
> I like this compromise.  Let me see if I got it right:  A stable write in
a constructor is treated like a final write ? it triggers a barrier at the
end of the constructor.  That?s a cheap move.  No other barriers are added
automatically, for reads or other writes, saving us from doing less cheap
moves.  The burden would be on users of stable vars (in fancy access
patterns) to add more fences if needed, but we don?t see any important
cases of that, at the moment.
>

No opinion on the merits here. But IIUC, "as memory safe as finals" is a
slightly squishy notion here. The downside of not having the release fence
is that even with safe publication, a write to an @Stable field outside the
constructor can be seen by a read in the constructor, before the object is
published. That's arguably weirder than final field behavior, and not
something that can arise with final fields. But it still only happens in
the presence of data races, and thus probably not in code you should be
writing anyway.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-dev/attachments/20240712/c76ad150/attachment.htm>