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

[Coleen Phillimore]

On Fri, 14 Nov 2025 15:44:53 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: Fixed build problem.

I think the SpinSingleSection might be a bit of over-engineering for something that's done a lot in the code base.  Let's see what @pchilano  thinks.

patch.txt line 3:

> 1: diff --git a/src/hotspot/share/runtime/objectMonitor.cpp b/src/hotspot/share/runtime/objectMonitor.cpp
> 2: index ee7629ec6f5..b1c806308ff 100644
> 3: --- a/src/hotspot/share/runtime/objectMonitor.cpp

I don't think this was supposed to be added.

src/hotspot/share/runtime/objectMonitor.cpp line 320:

> 318:   check_object_context();
> 319:   if (_object_strong.is_empty()) {
> 320:     auto setObjectStrongLambda = [&](OopHandle& object_strong, const WeakHandle& object) {

Doesn't the lambda capture the _object and _object_strong values from the [&] ?
And maybe instead of a class SpinSingleSection it should be a template function that passes in the lambda?

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

> 34: #include "utilities/checkedCast.hpp"
> 35: #include "utilities/globalDefinitions.hpp"
> 36: #include "utilities/spinCriticalSection.hpp"

Include shouldn't be needed here.

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 is this include needed here?

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

Changes requested by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/28264#pullrequestreview-3466310630
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2528558216
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2528580597
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2528566456
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2528572083