RFR: 8366671: Refactor Thread::SpinAcquire and Thread::SpinRelease

[Coleen Phillimore]

On Wed, 12 Nov 2025 11:17:22 GMT, Anton Artemov <aartemov at openjdk.org> wrote:

> Hi, 
> 
> please consider the following changes:
> 
> In this PR `Thread::SpinAcquire()` and `Thread::SpinRelease()` methods are refactored into a utility class `SpinCriticalSection`. The motivation is to make it easier for developers to use this lightweight synchronization mechanism in the codebase. The two aforementioned methods were used in JFR to create short critical sections with a helper class, but that was not the case for the object monitor code. 
> 
> Additionally, `SpinSingleSection` class is added, which allows to execute a payload code inside of a functor by only one thread. 
> 
> Tested in tiers 1 - 5.

I have some initial comments and I haven't really figured out why we want the SpinSingleSection version vs. the CAS that is there now.  The CAS makes a lot more sense to me.

I like this refactoring!

src/hotspot/share/runtime/objectMonitor.hpp line 379:

> 377:     SetObjectStrongFunctor(OopHandle* object_strong, WeakHandle const* object);
> 378:     void operator()();
> 379:   };

Does this need to be in the header file?

src/hotspot/share/runtime/park.cpp line 95:

> 93:   {
> 94:     SpinCriticalSection scs(&ListLock);
> 95:     {

Do we need these extra {} here and at line 98?

src/hotspot/share/utilities/spinCriticalSection.cpp line 33:

> 31:  // short-duration critical sections where we're concerned
> 32:  // about native mutex_t or HotSpot Mutex:: latency.
> 33: void SpinCriticalSectionHelper::SpinAcquire(volatile int* adr) {

Now we can follow the coding style by calling this method spin_acquire.

src/hotspot/share/utilities/spinCriticalSection.cpp line 40:

> 38:   // Slow-path : We've encountered contention -- Spin/Yield/Block strategy.
> 39:   int ctr = 0;
> 40:   int Yields = 0;

lower case y.

src/hotspot/share/utilities/spinCriticalSection.hpp line 37:

> 35:   static void SpinAcquire(volatile int* Lock);
> 36:   static void SpinRelease(volatile int* Lock);
> 37:   static bool TrySpinAcquire(volatile int* Lock);

All these names can now follow the coding style (snake case).

src/hotspot/share/utilities/spinCriticalSection.hpp line 56:

> 54: // A short section which is to be executed by only one thread.
> 55: // The payload code is to be put into an object inherited from the Functor class.
> 56: class SpinSingleSection {

Could this instead be a template <typename Function> instead and the code can pass a lambda to it rather than this Functor type?

test/hotspot/gtest/jfr/test_adaptiveSampler.cpp line 43:

> 41: #include "runtime/atomicAccess.hpp"
> 42: #include "utilities/globalDefinitions.hpp"
> 43: #include "utilities/spinCriticalSection.hpp"

Why this include?

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

PR Review: https://git.openjdk.org/jdk/pull/28264#pullrequestreview-3461859596
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2525093915
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2525068401
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2525070046
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2525070847
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2525076533
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2525084252
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2525074123