SpinPause() should be inlined?

[Yasumasa Suenaga]

Thanks Dan for your information!

At a glance, I think JDK-8200697 which has been mentioned in the comment will resolve the consideration about refactoring.
I wonder why SpinYield which has been intriduced in JDK-8200697 has not been applied all of the use of SpinPause(). This RFE seems to aim to allow future work without needing to rewrite all of SpinPause() use - it helps much for inlining!

OTOH I can't explain performance benefits clearly now...


Thanks,

Yasumasa


On 2021/06/23 0:55, daniel.daugherty at oracle.com wrote:
> Here's the RFE where that was discussed back in 2018:
> 
>      JDK-8208458 Simplify and inline os::SpinPause() for non-Windows OS on X86
>      https://bugs.openjdk.java.net/browse/JDK-8208458
> 
> Dan
> 
> On 6/22/21 10:51 AM, Yasumasa Suenaga wrote:
>> Hi all,
>>
>> I saw lock contention in SecureRandom. When I was analyzing it, I had a question.
>>
>> ObjectMonitor::TrySpin() calls SpinPause(). SpinPause() would issue PAUSE, but it is not inlined as following:
>>
>> ```
>>  1587e90:       f0 4d 0f b1 2e          lock cmpxchg %r13,(%r14)
>>  1587e95:       48 85 c0                test   %rax,%rax
>>  1587e98:       74 16                   je     1587eb0 <ObjectMonitor::TrySpin(JavaThread*)+0x60>
>>  1587e9a:       e8 31 7b d4 ff          callq  12cf9d0 <SpinPause>
>>  1587e9f:       83 eb 01                sub    $0x1,%ebx
>>  1587ea2:       72 da                   jb     1587e7e <ObjectMonitor::TrySpin(JavaThread*)+0x2e>
>> ```
>>
>> I found following comment about it in os.hpp. It says SpinPause() should be inlined.
>>
>> ```
>> // Note that "PAUSE" is almost always used with synchronization
>> // so arguably we should provide Atomic::SpinPause() instead
>> // of the global SpinPause() with C linkage.
>> // It'd also be eligible for inlining on many platforms.
>> ```
>>
>> According to Intel Software Developer's Manual, PAUSE seems to need to be inlined, but I'm not sure it can be allow through function call.
>> I've fixed it to do so for Linux x64 [1] and I benchmarked with [2], then I saw some advantage in PAUSE on my Core i3-8145U, but it may be within the margin of error.
>>
>> * original
>> Benchmark                       (algo)  (bytes)   Mode  Cnt Score       Error  Units
>> RandomBenchmark.fillRandomInMT    DRBG       16  thrpt    3 510141.578 ± 138543.261  ops/s
>>
>> * with inlined PAUSE
>> Benchmark                       (algo)  (bytes)   Mode  Cnt Score      Error  Units
>> RandomBenchmark.fillRandomInMT    DRBG       16  thrpt    3 531589.958 ± 66942.549  ops/s
>>
>>
>> Should be inlined PAUSE as `Atomic::SpinPause()`?
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> [1] https://github.com/YaSuenag/jdk/commit/66b31d35fac6fb0537bb9d85957157342fec564d
>> [2] https://github.com/YaSuenag/hwrand/blob/master/benchmark/src/main/java/com/yasuenag/hwrand/benchmark/RandomBenchmark.java#L59-L64
>