RFR: 8362193: Re-work MacOS/AArch64 SpinPause to handle SB
Aleksey Shipilev
shade at openjdk.org
Sun Jul 20 13:46:23 UTC 2025
On Fri, 18 Jul 2025 13:11:26 GMT, Evgeny Astigeevich <eastigeevich at openjdk.org> wrote:
> ### Background
>
> With JDK-8359435 "AArch64: add support for SB instruction to MacroAssembler::spin_wait" we have an option to use the speculation barrier (SB) instruction for `j.l.Thread::onSpinWait` and `SpinPause`. On Linux AArch64 `SpinPause` uses a stub which is generated with `MacroAssembler::spin_wait`. `j.l.Thread::onSpinWait` uses it as well. As a result tests for `j.l.Thread::onSpinWait`, e.g.`compiler/onSpinWait/TestOnSpinWaitAArch64.java`, cover both `j.l.Thread::onSpinWait` and `SpinPause`. Also we don't need to update `SpinPause` when we add support for a new instruction to `MacroAssembler::spin_wait`.
>
> On Mac AArch64 `SpinPause` does not use the generated stub to avoid a costly call to `os::current_thread_enable_wx()`. It uses inline assembly instead. As a result we have `SpinWait` implementation details leaking into `SpinPause`. Besides asserts there are no tests covering Mac implementation of `SpinPause`. Testing on Apple M3 Pro showed that `compiler/onSpinWait/TestOnSpinWaitAArch64.java` could not reliably trigger those asserts. `ArchiveWorkers::run_task_multi` did not invoke `spin.wait()`. The inline assembly in Mac AArch64 `SpinPause` has another issue. It uses a jump table. An offset in the table is calculated based on the value of `enum SpinWait::Inst`. When a new value `SB` was added the offset became out of bounds. The jump goes out of the assembly code.
>
> ### Summary of changes
>
> * Rewrote Mac AArch64 `SpinPause` not to use a jump table.
> * Added `SpinWait::supports()` and `SpinWait::from_name()` methods to validate and convert instruction names into corresponding enum values.
> * Updated `SpinWait::Inst` enum to use bit flags for simplified assembly testing and added a constructor to initialize `SpinWait` using instruction names.
> * `src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp`: Added an assertion to ensure the `sb` instruction is supported by the CPU before generation.
> * Added `OnSpinWaitInstNameConstraintFunc` for the `OnSpinWaitInst` option to check option's value at parsing.
> * Added a gtest to verify the functionality of `SpinPause`.
> * Added a jtreg test to run the gtest for`SpinPause` with various instructions, including the `sb` instruction if supported by the CPU.
>
> ### Testing results: fastdebug, release
>
> - Linux, Graviton 2
> - tier1: Passed
> - `test/hotspot/jtreg/gtest/TestSpinPauseAArch64.java`: Passed
> - `test/hotspot/jtreg/compiler/onSpinWait`: Passed
> - MacOS, M3 Pro
> - tier1, OnSpinWaitInst=default: Passed
> ...
Overall, I think it is not very reasonable to pull things into shared parts, when that deviates significantly from the common Hotspot way of handling things. I don't think it even saves us any duplication to move implementations to `spin_wait_aarch64.(inline.)pp`? So I suggest we don't.
More reviews.
I think the PR drifted from my initial "oh, just assert range problem" to "Re-work MacOS/AArch64 spin-waits to handle SB". Rewrite the JBS/PR title accordingly?
src/hotspot/cpu/aarch64/globals_aarch64.hpp line 118:
> 116: "Value -1 means off.") \
> 117: range(-1, 4096) \
> 118: product(ccstr, OnSpinWaitInst, DEFAULT_SPIN_WAIT_INST, DIAGNOSTIC, \
It is somewhat weird to introduce references to macros here. At some point, this might lead to _interesting_ include circularities, as I think `globals.*` are supposed to be at the leaves of include trees. Other options do not do this, see for example `UseBranchProtection` below. So, keep this part intact?
src/hotspot/cpu/aarch64/vm_version_aarch64.cpp line 54:
> 52:
> 53: static SpinWait get_spin_wait_desc() {
> 54: if (!SpinWait::supports(OnSpinWaitInst)) {
So this is not about the actual spin-wait hints support, correct? This only checks the option string is in expected domain? A common way to deal with this is to use constraint functions, see for example:
product(ccstr, AOTCache, nullptr, \
"Cache for improving start up and warm up") \
constraint(AOTCacheConstraintFunc, AtParse) \
\
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 546:
> 544: assert(inst_id != SpinWait::SB || VM_Version::supports_sb(), "current CPU does not support SB instruction");
> 545: if (inst_id > SpinWait::NOP) {
> 546: warining("Unsupported type of SpinWait::Inst: %d", inst_id);
`warning`
Also, I think we want to minimize the amount of code we actually execute in `SpinPause`, since it likely sits in the hot loop. So checking this here is probably counter-productive.
test/hotspot/jtreg/gtest/TestSpinPauseAArch64.java line 24:
> 22: */
> 23:
> 24: /**
Suggestion:
/*
test/hotspot/jtreg/gtest/TestSpinPauseAArch64.java line 25:
> 23:
> 24: /*
> 25: * @test TestSpinPauseAArch64
A common way to tag the tests:
@test id=default
I assume all these are supported in ARMv8.0 (default? baseline?) profile. That's why I put `default`. Put something else if that is incorrect.
Same for the second `@test` block.
test/hotspot/jtreg/gtest/TestSpinPauseSBAArch64.java line 32:
> 30: * @requires os.arch=="aarch64"
> 31: * @requires vm.cpu.features ~= ".*sb.*"
> 32: * @run main/native GTestWrapper --gtest_filter=SpinPause* -XX:+UnlockDiagnosticVMOptions -XX:OnSpinWaitInst=sb
This configuration is for `@requires vm.cpu.features ~= ".*sb.*"`, correct?
If so, you can put this entire `@test` block in the `TestSpinPauseAArch64.java`, then jtreg would just execute them as subtests. Here is an inspiration: https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/gc/shenandoah/TestAllocObjects.java
-------------
PR Review: https://git.openjdk.org/jdk/pull/26387#pullrequestreview-3033656122
PR Review: https://git.openjdk.org/jdk/pull/26387#pullrequestreview-3034111169
PR Review Comment: https://git.openjdk.org/jdk/pull/26387#discussion_r2216124860
PR Review Comment: https://git.openjdk.org/jdk/pull/26387#discussion_r2216440371
PR Review Comment: https://git.openjdk.org/jdk/pull/26387#discussion_r2216466816
PR Review Comment: https://git.openjdk.org/jdk/pull/26387#discussion_r2216117779
PR Review Comment: https://git.openjdk.org/jdk/pull/26387#discussion_r2216427945
PR Review Comment: https://git.openjdk.org/jdk/pull/26387#discussion_r2216117555
More information about the hotspot-dev
mailing list