RFR: 8366671: Refactor Thread::SpinAcquire and Thread::SpinRelease [v6]

Wed Nov 19 14:10:10 UTC 2025

On Mon, 17 Nov 2025 22:04:30 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8366671: Removed redundant include.
>
> src/hotspot/share/runtime/park.cpp line 66:
> 
>> 64:   {
>> 65:     SpinCriticalSection scs(&ListLock);
>> 66:     {
> 
> This extra level of scoping doesn't seem needed.

Addressed.

> src/hotspot/share/utilities/spinCriticalSection.cpp line 38:
> 
>> 36:   }
>> 37: 
>> 38:   // Slow-path : We've encountered contention -- Spin/Yield/Block strategy.
> 
> Use "utilities/spinYield.hpp"?

This is a good suggestion,  `SpinYield` has slightly different behavior: it does not return to `SpinPause()` after exceeding the spin limit, whereas the current code does. It looks like the behavior `SpinYield` is what one needs, because if a thread failed to grab a lock while spinning, why to spin again? The performance testing is being done right now.

> src/hotspot/share/utilities/spinCriticalSection.hpp line 30:
> 
>> 28: #include "runtime/javaThread.hpp"
>> 29: 
>> 30: class SpinCriticalSectionHelper {
> 
> Derive from `AllStatic` ("memory/allStatic.hpp").

Done.

> src/hotspot/share/utilities/spinCriticalSection.hpp line 42:
> 
>> 40: 
>> 41: // Short critical section. To be used when having a
>> 42: // mutex is considered to be expensive.
> 
> I think this is a really poor description, as I think it will encourage the use of these facilities in
> inappropriate places.  Spin-lock usage ought to be pretty rare!  Note that the existing mechanism
> is described as "Not for general synchronization use." I think better motivation is needed. Note
> I'm not suggesting that doesn't exist, rather than motivation and usage guidelines should be
> documented here. The comment for SpinCriticalSectionHelper in the .cpp file is more the kind
> of thing I'm looking for. I shouldn't have to look at that internal helper's implementation to find
> such guidance.

Right. I moved that comment over to the header file and modified it a bit.

> src/hotspot/share/utilities/spinCriticalSection.hpp line 45:
> 
>> 43: class SpinCriticalSection {
>> 44: private:
>> 45:   volatile int* const _lock;
> 
> Use new `Atomic<T>` rather than introducing new direct uses of `AtomicAccess`.

This will require somewhat extensive changes in JFR, as the are using the same thing for their JfrTryLock.

> src/hotspot/share/utilities/spinCriticalSection.hpp line 53:
> 
>> 51:     SpinCriticalSectionHelper::spin_release(_lock);
>> 52:   }
>> 53: };
> 
> Should be noncopyable.

I made `SpinCriticalSection` non-copyable.

> src/hotspot/share/utilities/spinCriticalSection.hpp line 55:
> 
>> 53: };
>> 54: 
>> 55: template<class Lambda, class...Args>
> 
> I'd prefer not to name the first argument "Lambda", since it might not be one.
> I would prefer `F` or `Fn` or something like that. And there should be some documentation
> for this class, including a description of the template parameters and their requirements.

I removed SpinSingleSection completely, it is an overkill.

> src/hotspot/share/utilities/spinCriticalSection.hpp line 56:
> 
>> 54: 
>> 55: template<class Lambda, class...Args>
>> 56: class SpinSingleSection {
> 
> Consider giving this class template a deduction guide. That will likely make uses _much_ simpler,
> removing the explicit template parameters in variable declarations and just letting them be
> deduced from the constructor argument types.

I removed SpinSingleSection completely.

> src/hotspot/share/utilities/spinCriticalSection.hpp line 58:
> 
>> 56: class SpinSingleSection {
>> 57: private:
>> 58:   volatile int* const _lock;
> 
> Why an `int`-type value for the lock, rather than `bool`? I know why, but it should probably be
> stated explicitly, else someone might be tempted to change it in the future.

I believe it is for historical reasons?

> src/hotspot/share/utilities/spinCriticalSection.hpp line 61:
> 
>> 59:   Thread* _lock_owner;
>> 60: public:
>> 61:   SpinSingleSection(volatile int* lock, Lambda& F, Args&... args) : _lock(lock), _lock_owner(nullptr) {
> 
> `F` => `f` - variables have lower-case names.

I removed SpinSingleSection completely.

> src/hotspot/share/utilities/spinCriticalSection.hpp line 61:
> 
>> 59:   Thread* _lock_owner;
>> 60: public:
>> 61:   SpinSingleSection(volatile int* lock, Lambda& F, Args&... args) : _lock(lock), _lock_owner(nullptr) {
> 
> I really need to get on the ball and update the style guide regarding at least forwarding references
> and `std::forward`.

I removed SpinSingleSection completely.

> src/hotspot/share/utilities/spinCriticalSection.hpp line 61:
> 
>> 59:   Thread* _lock_owner;
>> 60: public:
>> 61:   SpinSingleSection(volatile int* lock, Lambda& F, Args&... args) : _lock(lock), _lock_owner(nullptr) {
> 
> Taking the function argument by reference prevents certain common use-cases, e.g. I think this
> prevents passing an anonymous lambda.

I removed SpinSingleSection completely.

> src/hotspot/share/utilities/spinCriticalSection.hpp line 63:
> 
>> 61:   SpinSingleSection(volatile int* lock, Lambda& F, Args&... args) : _lock(lock), _lock_owner(nullptr) {
>> 62:     if (SpinCriticalSectionHelper::try_spin_acquire(_lock)) {
>> 63:       _lock_owner = Thread::current();
> 
> Why do we need the owning thread here? It seems like a bool "lock acquired" value
> would be sufficient.

I removed SpinSingleSection.

> src/hotspot/share/utilities/spinCriticalSection.hpp line 75:
> 
>> 73:       SpinCriticalSectionHelper::spin_release(_lock);
>> 74:     }
>> 75:   }
> 
> Should be noncopyable.

Done.

> src/hotspot/share/utilities/spinCriticalSection.hpp line 77:
> 
>> 75:   }
>> 76: };
>> 77: #endif //SHARE_UTILITIES_SPINCRITICALSECTION_HPP
> 
> We usually put a blank line before the `#endif` of the include guard. Also a space after `//`.

Addressed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542153334
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542152204
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542154027
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542150954
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542148147
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542154310
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542149515
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542149897
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542152590
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542150246
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542150580
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542152983
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542154871
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542154548
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2542153781