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

[Kim Barrett]

On Tue, 11 Nov 2025 10:54:57 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> 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/8ab17707...da58d0d2
>
> 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.

There's a PrimitiveConversions::Translate specialization for enums. I'm not
sure what more should be done than the existing reference to Translate
specializations. It's not appropriate to list here all the types for which
there are such specializations. Something like "such as enum types" could be
added, but that feels like treating enum as more special here than I think it is.

> 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?

As explained later in this big comment, `exchange` differs from
`compare_exchage` because `AtomicAccess` doesn't currently implement `xchg`
for 1-byte values. Both the description and the implementation here would be
simpler if it did. (That includes eliminating the whole conditional `exchange`
for atomic translated types.) And I know of one or two places where an
exchange of a bool value might be clearer than the current cmpxchg code.
(Though `fetch_then_set` seems clearer still, at least to me. :) ) So maybe
someone will add that support, but not in this PR.

> 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.

The `Base` types had more uses in earlier versions. Removed.

> 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"?

`I` _is_ short for `Integer`. That argument must be integral, even when the
atomic value type is a pointer.  (Atomic ptrdiff isn't a sensible operation.)
Though I somehow dropped the check that it is indeed integral! Fixed that.

Addition and subtraction have "addend" and "subtrahend" respectively, but
there doesn't seem to be a great generalization of that. A couple AIs
suggested "operand", but that's pretty uninformative. I decided to go with
"offset". That was already being used in the error messages. More importantly,
`Offset` is much easier to distinguish from `T` than is `I`. Updated to use
that nomenclature consistently, so, for example, `check_i` -> `check_offset_type`.

> 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?

This is a variation on the so-called "sizeof trick", but using a type
predicate rather than a sizeof comparison. I can never remember the syntax for
the size!=1 return type, and have to search for an example every time I use
that technique. I recently realized that distinguishing based on types avoided
the uncommon syntax, and `decltype` + `<type_traits>` makes that easy.

The use of `char` (and `char*`) was just a holdover from the typical usage in
examples of the "sizeof trick"; nothing special there. I changed it to use
obviously distinct types for the two cases - `void*` and `int`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2515693803
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2515695361
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2515716347
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2515717496
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2515718233