RFR: 8367013: Add Atomic<T> to package/replace idiom of volatile var plus AtomicAccess:: operations [v6]

[Stefan Karlsson]

On Tue, 11 Nov 2025 06:26:01 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 20 additional commits since the last revision:
> 
>  - Merge branch 'master' into atomic-template-tag-select
>  - remove AtomicNextAccess and uses
>  - use type_traits wrapper in new code
>  - Merge branch 'master' into atomic-template-tag-select
>  - more naming updates
>  - rename relaxed_store => store_relaxed
>  - default construct translated atomic without SFINAE
>  - Merge branch 'master' into atomic-template-tag-select
>  - Merge branch 'master' into atomic-template-tag-select
>  - add reference to gcc bug we're working around
>  - ... and 10 more: https://git.openjdk.org/jdk/compare/8e31dc62...da58d0d2

I think this looks very nice and readable. I'm approving this. I'm also leaving a few comments below on things that I reacted to and that you might want to address.

src/hotspot/share/runtime/atomic.hpp line 36:

> 34: // Atomic<T> is used to declare a variable of type T with atomic access.
> 35: //
> 36: // The following value types T are supported:

It would be nice to explain how enums fit into all this. In offline discussions that question was raised. It would be nice to get that clarified in this comment.

src/hotspot/share/runtime/atomic.hpp line 105:

> 103: // element arithmetic.
> 104: //
> 105: // (4) An atomic translated type additionally provides the exchange

It is a little odd that `exchange` and `compare_exchange` behave differently here. Is that only because of how `AtomicAccess` is currently implemented?

src/hotspot/share/runtime/atomic.hpp line 282:

> 280: template<typename T>
> 281: class AtomicImpl::SupportsExchange : public CommonCore<T> {
> 282:   using Base = CommonCore<T>;

I don't see how this `using Base` aids in the readability. It is only used in the constructor, and there it just becomes an extra level of indirection.

The same comment goes for the other `using Base` instances.

src/hotspot/share/runtime/atomic.hpp line 300:

> 298: 
> 299:   // Guarding the AtomicAccess calls with constexpr checking of I produces
> 300:   // better compile-time error messages.

It is unclear what `I` is meant to be a short name for. I don't think it is `integer` because we are also dealing with pointers. I don't think it can be `increment` because some functions pass the decrement amount.

Maybe `V` (for value) could be more suitable here? But that could conflict with the contained _value in Atomic.

In `check_i` you have this code and comment:

    } else if constexpr (std::is_signed_v<T>) {
      static_assert(std::is_signed_v<I>, "value is signed but offset is unsigned");


So maybe it could be `O` for offset? If `O` looks too much like `0` then maybe spell it out as `Offset`?

Is there a collective name for "add value" and "sub value"?

src/hotspot/share/runtime/atomic.hpp line 442:

> 440: public:
> 441:   static constexpr bool value = std::is_pointer_v<test_type>;
> 442: };

This little bit of time to understand and I tried to figure out why return `char*` or `char`, but then I see that it is only used in `is_pointer_v` test. Could this have been using `void*` and `void` instead, or does this need to use a type that can be instantiated?

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

Marked as reviewed by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27539#pullrequestreview-3447468889
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2513766104
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2513774450
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2513782743
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2513808761
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2513819494