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

[Anton Artemov]

On Thu, 13 Nov 2025 22:11:06 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8366671: Addressed reviewer's comments.
>
> 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?

I removed the functor completely, now a lambda function is used.

> src/hotspot/share/runtime/park.cpp line 95:
> 
>> 93:   {
>> 94:     SpinCriticalSection scs(&ListLock);
>> 95:     {
> 
> Do we need these extra {} here and at line 98?

No, we don't need those. Removed.

> 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.

Adjusted in the latest commit.

> 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.

Adjusted in the latest commit.

> 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).

Adjusted in the latest commit.

> 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?

It can be done with a template and a lambda function. I changed the implementation in the latest commit. It does not look any better, however.

> 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?

I used to have a different layout, where it was necessary, not it is not. Removed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2527821629
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2527816701
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2527817406
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2527817745
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2527819831
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2527820396
PR Review Comment: https://git.openjdk.org/jdk/pull/28264#discussion_r2527819486