RFR: 8366671: Refactor Thread::SpinAcquire and Thread::SpinRelease [v6]
Kim Barrett
kbarrett at openjdk.org
Mon Nov 17 22:52:18 UTC 2025
On Mon, 17 Nov 2025 11:59:29 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 lambda function by only one thread.
>>
>> Tested in tiers 1 - 5.
>
> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
>
> 8366671: Removed redundant include.
Changes requested by kbarrett (Reviewer).
src/hotspot/share/runtime/park.cpp line 66:
> 64: {
> 65: SpinCriticalSection scs(&ListLock);
> 66: {
This extra level of scoping doesn't seem needed.
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"?
src/hotspot/share/utilities/spinCriticalSection.hpp line 30:
> 28: #include "runtime/javaThread.hpp"
> 29:
> 30: class SpinCriticalSectionHelper {
Derive from `AllStatic` ("memory/allStatic.hpp").
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.
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`.
src/hotspot/share/utilities/spinCriticalSection.hpp line 53:
> 51: SpinCriticalSectionHelper::spin_release(_lock);
> 52: }
> 53: };
Should be noncopyable.
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.
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.
src/hotspot/share/utilities/spinCriticalSection.hpp line 56:
> 54:
> 55: template<class Lambda, class...Args>
> 56: class SpinSingleSection {
Although I've made some suggestions for possibly improving SpinSingleSection,
I'm not sure it's a good idea as a concept. It seems to be attempting to
provide a conditional critical section, but is doing so in what seems to me to
be a weird way.
As provided, it first conditionally executes a funarg under the
lock, if it can acquire the lock. It then permits an external body (the scope
of the section) to execute either under or not under the lock (depending on
whether it was successfully acquired), with no way to know which state we're
in.
I think an API more similar to `std::unique_lock` for SpinCriticalSection
would be better. `std::unique_lock` provides a `owns_lock()` function and a
constructor overload taking a `std::try_to_lock_t` value. This controls
whether the locking should be conditional or not, and a way for the using code
to detect success/failure to lock in the conditional case. This doesn't have
to be in one class though. There could be two critical section classes, one
unconditional and one conditional, with only the latter providing the
success/failure info.
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.
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.
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`.
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.
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.
src/hotspot/share/utilities/spinCriticalSection.hpp line 75:
> 73: SpinCriticalSectionHelper::spin_release(_lock);
> 74: }
> 75: }
Should be noncopyable.
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 `//`.
-------------
PR Review: https://git.openjdk.org/jdk/pull/28264#pullrequestreview-3474550894
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535649044
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535596087
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535672539
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535572951
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535511031
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535682627
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535516604
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535520250
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535744012
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535609870
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535528890
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535535461
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535634603
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535741931
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535683931
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2535668579
More information about the hotspot-dev
mailing list