RFR: 8321371: SpinPause() not implemented for bsd_aarch64/macOS

[Evgeny Astigeevich]

On Wed, 13 Dec 2023 11:01:42 GMT, Fredrik Bredberg <fbredberg at openjdk.org> wrote:

>> The better way to find out an instruction to use is to run microbenchmarks/benchmarks. See #6803.
>
> Hi @eastig,
> 
> I initially did the things you suggest, but after some internal discussions changed the implementation into a single yield instruction.
> 
> Here's some background info from JDK-8321371:
> 
> Fredrik: My thought was to implement SpinPause() for MacOS by copying the implementing from src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp to src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp.
> 
> @shipilev I remember trying the same thing along with JDK-8318986, but I realized SpinPause() is only used from the quite hot VM native code, and so it would probably affect GC and runtime performance. The actual Thread.onSpinWait from Java code should be already handled by intrinsics. I suspect the overhead of doing the stub call is already similar to whatever hint we finally emit in the stub, but the WX transition back and forth is likely to be quite bad to make often. SpinWait is quite likely used in busy loops, so this would add up.
> 
> So if we are doing this, we need to check how much does this actually cost.
> 
> Fredrik: I was running some performance tests after removing ObjectMonitor::NotRunnable() (see JDK-8320317).
> 
> The performance went up on Linux x86 and Windows x86 by approximately 12%, but went down with roughly the same amount on macOS AArch64. The performance decreased only slightly on Linux AArch64. So I stated to focus on the differences between macOS and Linux on AArch64 and found out that SpinPause() is implemented on Linux but not on macOS.
> 
> So I copied the source from Linux to macOS (or bsd_aarch64 if you'd like) and re-run the tests. This seemed to help bringing back macOS to the Linux level on AArch64.
> 
> I do agree that the overhead of doing the stub call is already similar to whatever hint we finally emit in the stub, and that the WX transition back and forth is likely to be quite bad.
> 
> My measurements showed that among the different OnSpinWaitInst options, "isb" generated the best result. If we could get rid of the OnSpinWaitInst options and just hard code an isb instruction (or any other instruction that people can agree upon) like it's done on x86, that would probably be best. For now I just wanted macOS AArch64 to be on par with Linux after the removal of NotRunnable().
> 
> About measuring the actual cost. Some of the performance tests show notoriously unstable values, when run multiple times. I've focused on the ones that I feel is stable.
> 
> After having some internal discussions, it seems like the most reasonable thing to do is to implement Spin...

@fbredber I read comments on JDK-8321371.
I see misunderstanding of the purpose of `SpinPause`:

> After having some internal discussions, it seems like the most reasonable thing to do is to implement SpinPause() using a single inline yield instruction. This way we get rid of both the call and the WX stuff.
>
> This solution also showed better performance figures than the OnSpinWaitInst options did.
>
> The reason for using the yield instruction instead of the the isb instruction (which showed slightly better performance figures) is that the yield instruction is meant for this kind of use cases. So even if isb is slightly better on today's silicon, yield is likely to be better in the long run.

`SpinPause` should introduce short delays ~30 - 100 clocks. It should have ARM yield or Intel pause semantics to reduce the power consumption. As it is unlikely `yield` to become a real instruction in the long run, not `nop`. We tried different approaches to simulate `yield`. We found that `isb` is the closest to the needed semantics. 

Also without reviewing your benchmarking results, it is difficult to say what you measured. Could you share them?

You might also try another implementation: https://github.com/openjdk/jdk/pull/5562#issuecomment-1122652981

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

PR Comment: https://git.openjdk.org/jdk/pull/16994#issuecomment-1853727286