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

[Axel Boldt-Christmas]

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

I played around with this a bit. I think the implementation and API looks good, but have a couple of questions and I found one issue with certain types which unexpectedly do not compile.

So in the following code snippet neither `Atomic<EnumClass16>` nor `Atomic<uint16_t>` can be compiled. 

```C++
enum class EnumClass8 : uint8_t {};
enum class EnumClass16 : uint16_t {};
enum class EnumClass32 : uint32_t {};

enum class SpecialEnumClass8 : uint8_t {};
enum class SpecialEnumClass16 : uint16_t {};
enum class SpecialEnumClass32 : uint32_t {};

template<> struct PrimitiveConversions::Translate<SpecialEnumClass8> : public std::true_type {
  using Value = SpecialEnumClass8;
  using Decayed = uint32_t;

  static Decayed decay(Value x) { return (Decayed)(uint16_t)x; }
  static Value recover(Decayed x) { return Value(x); }
};

template<> struct PrimitiveConversions::Translate<SpecialEnumClass16> : public std::true_type {
  using Value = SpecialEnumClass16;
  using Decayed = uint32_t;

  static Decayed decay(Value x) { return (Decayed)(uint16_t)x; }
  static Value recover(Decayed x) { return Value(x); }
};

template<> struct PrimitiveConversions::Translate<SpecialEnumClass32> : public std::true_type {
  using Value = SpecialEnumClass32;
  using Decayed = uint32_t;

  static Decayed decay(Value x) { return (Decayed)(uint16_t)x; }
  static Value recover(Decayed x) { return Value(x); }
};

TEST_VM(AtomicValueAccessTest, type_test) {
  // Commented lines do no compile

  Atomic<EnumClass8> aec8;
  // Atomic<EnumClass16> aec16; // Unexpected
  Atomic<EnumClass32> aec32;

  // aec8.fetch_then_set(EnumClass8{}); // Expected
  // aec16.fetch_then_set(EnumClass16{}); // Expected
  aec32.fetch_then_set(EnumClass32{});

  aec8.cmpxchg(EnumClass8{}, EnumClass8{});
  // aec16.cmpxchg(EnumClass16{}, EnumClass16{});
  aec32.cmpxchg(EnumClass32{}, EnumClass32{});

  Atomic<uint8_t> a8;
  // Atomic<uint16_t> a16;  // Unexpected
  Atomic<uint32_t> a32;

  // a8.fetch_then_set(uint8_t{}); // Expected
  // a16.fetch_then_set(uint16_t{}); // Expected
  a32.fetch_then_set(uint32_t{});

  a8.cmpxchg(uint8_t{}, uint8_t{});
  // a16.cmpxchg(uint16_t{}, uint16_t{});
  a32.cmpxchg(uint32_t{}, uint32_t{});

  Atomic<SpecialEnumClass8> asec8;
  Atomic<SpecialEnumClass16> asec16;
  Atomic<SpecialEnumClass32> asec32;

  asec8.fetch_then_set(SpecialEnumClass8{});
  asec16.fetch_then_set(SpecialEnumClass16{});
  asec32.fetch_then_set(SpecialEnumClass32{});

  asec8.cmpxchg(SpecialEnumClass8{}, SpecialEnumClass8{});
  asec16.cmpxchg(SpecialEnumClass16{}, SpecialEnumClass16{});
  asec32.cmpxchg(SpecialEnumClass32{}, SpecialEnumClass32{});
}

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

> 297:                    atomic_memory_order order = memory_order_conservative) {
> 298:     return AtomicAccess::xchg(this->value_ptr(), new_value);
> 299:   }

I am not sure I understood the motivation behind the changing of the `xchg` name, nor why `cmpxchg` did not get a similar treatment. Could you provide some elaboration here?

Is there any plan to change the naming in the AtomicAccess layer as well?

test/hotspot/gtest/runtime/test_atomic.cpp line 604:

> 602: // Verify that explicit instantiation doesn't attempt to reference the
> 603: // non-existent fetch_then_set of the atomic decayed type.
> 604: template class AtomicImpl::Atomic<TranslatedAtomicByteObject>;

Preexisting: 
Why do we not suport exchange on 8 and 16 byte sized types? 

Especially given that we have `AtomicAccess::XchgUsingCmpxchg` and do support `cmpxchg`.

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

PR Review: https://git.openjdk.org/jdk/pull/27539#pullrequestreview-3363818171
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2450522649
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2450586436