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

[Emanuel Peter]

On Thu, 16 Oct 2025 14:45:45 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:
> 
>   default construct translated atomic without SFINAE

Drive-by comments, feel free to ignore/resolve without much fuss ;)

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

> 82: // Default construction of an atomic integer or atomic byte initializes the
> 83: // value to zero. Default construction of an atomic pointer initializes the
> 84: // value to null.

Suggestion:

// value to nullptr.

Optional, and total nit-pick. Reason: we are in C++ and not Java.

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

> 158: //
> 159: // * Rather than load/store operations with a memory order parameter,
> 160: // Atomic<T> provides load_{relaxed,acquire}() and {relaxed,release}_store()

Optional, and totally nit-picky: consider writing them out, so we can grep easily.

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

> 31: 
> 32: // These tests of Atomic<T> only verify functionality.  They don't verify
> 33: // atomicity.

Would that be desirable? Do we have tests, or an issue for it to do it in the future?

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

PR Review: https://git.openjdk.org/jdk/pull/27539#pullrequestreview-3360752030
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2448291763
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2448316965
PR Review Comment: https://git.openjdk.org/jdk/pull/27539#discussion_r2448368295