RFR: 8367013: Add Atomic<T> to package/replace idiom of volatile var plus AtomicAccess:: operations [v5]
Kim Barrett
kbarrett at openjdk.org
Tue Nov 11 06:26:03 UTC 2025
On Tue, 21 Oct 2025 14:58:30 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
>> Please review this change that adds the type Atomic<T>, to use as the type
>> of a variable that is accessed (including writes) concurrently by multiple
>> threads. This is intended to replace (most) uses of the current HotSpot idiom
>> of declaring a variable volatile and accessing that variable using functions
>> from the AtomicAccess class.
>> https://github.com/openjdk/jdk/blame/528f93f8cb9f1fb9c19f31ab80c8a546f47beed2/doc/hotspot-style.md#L138-L147
>>
>> This change replaces https://github.com/openjdk/jdk/pull/27462. Differences are
>>
>> * Substantially restructured `Atomic<T>`, to be IDE friendly. It's
>> operationally the same, with the same API, hence uses and gtests didn't need
>> to change in that respect. Thanks to @stefank for raising this issue, and for
>> some suggestions toward improvements.
>>
>> * Changed how fetch_then_set for atomic translated types is handled, to avoid
>> having the function there at all if it isn't usable, rather than just removing
>> it via SFINAE, leaving an empty overload set.
>>
>> * Added more gtests.
>>
>> Testing: mach5 tier1-6, GHA sanity tests
>
> Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
>
> rename relaxed_store => store_relaxed
More naming discussions have led to more name changes.
| AtomicAccess | old PR | new PR |
+=====================+=====================+=====================+
| store | store_relaxed | store_relaxed |
| release_store | release_store | release_store |
| release_store_fence | release_store_fence | release_store_fence |
| | | |
| load | load_relaxed | load_relaxed |
| load_acquire | load_acquire | load_acquire |
| | | |
| add | add_then_fetch | add_then_fetch |
| fetch_then_add | fetch_then_add | fetch_then_add |
| | | |
| sub | sub_then_fetch | sub_then_fetch |
| | fetch_then_sub | fetch_then_sub |
| | | |
| inc | atomic_inc | <removed> [1] | new PR change
| dec | atomic_dec | <removed> [1] | new PR change
| | | |
| xchg | fetch_then_set | exchange | new PR change
| cmpxchg | cmpxchg | compare_exchange | new PR change
| | | |
| replace_if_null | replace_if_null | <removed> [2] | new PR change
| | clear_if_equal | <removed> [2] | new PR change
| | | |
| fetch_then_and | fetch_then_and | fetch_then_and |
| fetch_then_or | fetch_then_or | fetch_then_or |
| fetch_then_xor | fetch_then_xor | fetch_then_xor |
| | | |
| and_then_fetch | and_then_fetch | and_then_fetch |
| or_then_fetch | or_then_fetch | or_then_fetch |
| xor_then_fetch | xor_then_fetch | xor_then_fetch |
[1] The `inc` and `dec` operations originally existed to provide better
codegen on some platforms for some cases where the value isn't needed. That
got dropped somewhere along the line. For example, `AtomicAccess::inc` just
calls `AtomicAccess::add` with an addend of 1. The assumption is that if the
result isn't used then the implementation (perhaps with help from the C++
compiler) can take care of appropriate optimization. They enabled the use of
x86 locked inc/dec instructions, which doesn't happen with the current code.
We could probably get better codegen for x86 by using compiler intrinsics,
instead of using inline assembler.
[2] `AtomicAccess::replace_if_null` was added because of complaints about
`cmpxchg` with a `NULL` (now `nullptr`) argument needing a cast, in order to
deal with argument type deduction. I added `Atomic<T>::clear_if_equal` because
I've wanted `AtomicAccess::clear_if_equal` in several places, but never got
around to adding it. But it turns out that not only don't we need casts in the
Atomic<T> layer, but there are a couple of ways to avoid the need for them in
the AtomicAccess layer too. So these seem of questionable utility.
I've also removed the new `AtomicNextAccess` class and backed out it's uses.
This is to reduce the size and scope of this PR to simplify its review. My
intent is to propose `AtomicNextAccess` in a followup.
The purpose of `AtomicNextAccess` is to permit conversions of clients of
`LockFreeStack` and `NonblockingQueue` to be done incrementally, rather than
requiring them to all be done in one change. It's useful to know how that
could be done, but doesn't have to be part of the initial `Atomic<T>` change.
Once all those clients have been converted, `AtomicNextAccess` can be removed.
Note that `NonblockingQueue` no longer has any clients. When I wrote
`AtomicNextAccess` it was being used by the G1 post-barrier. But with the
recent overhaul of the post-barrier, that use is gone. It was originally
internal to G1, but the Google folks wanted to use it for something else
(JDK-8236485: "Epoch synchronization protocol for G1 concurrent refinement",
which is also rendered moot by the recent G1 post-barrier changes. But I think
they had some other use in mind too?). So it got moved from gc/g1 to
utilities, renamed, and improved in various ways. Other Google uses don't seem
to have materialized though. But maybe it should just be deleted now?
-------------
PR Comment: https://git.openjdk.org/jdk/pull/27539#issuecomment-3515172882
More information about the hotspot-dev
mailing list