On Tue, 5 May 2026 12:46:56 GMT, Paul Hübner <[email protected]> wrote:
> Hi all, > > This patch fixes some performance issues with AArch64's `NMethodEntryBarrier`. > > The purpose of a method entrypoint is to act as a barrier for potential GC > work. Entrypoints can be "armed", such that when the method is called, a > slow-path is triggered. This slow-path then does any necessary fixups. > > With the introduction of scalarized calling conventions in Valhalla, methods > have multiple entrypoints, depending on the calling conventions that are > used. This is due to the fact that `nmethod`s cannot be multiplexed when > scalarized. There can be up to three entrypoints for a given method. The > locations of these entrypoints is tracked via relocations. > > The problem lies within the usage of `RelocIterator`. When `static void > set_value` is called (such as when arming entrypoints), it creates a > `NativeNMethodBarrier` for each entrypoint. However, the `RelocIterator` only > finds the first relocation, meaning each embodiment of `NativeNMethodBarrier` > sees the same `_guard_addr`. Consequently, executions entering through the > other entrypoints will not see the up-to-date armed value, and be > unnecessarily slow-pathed. > > This patch introduces several improvements: > 1. Makes `NativeNMethodBarrier` use the `ldr` instructions to determine the > `_guard_addr`s, as opposed to relocations (contributed by @TobiHartmann). > This gets the correct addresses for each of the guards, and avoids having to > iterate through the `nmethod`'s relocations, which is expensive for large > methods. > 2. Consolidates all the entrypoints as first-class citizens of > `NativeNMethodBarrier`, instead of having an `NativeNMethodBarrier` per > entrypoint. This reduces object allocation, and makes the code much easier to > read for free! > 3. Stricter assertions. All entrypoints `ldr` instructions are verified (as > opposed to just the scalarized ones previously). All guard addresses are > verified to be within the `nmethod`'s instructions/stubs. > > Testing: tiers 1-4 and some adhoc testing with `--enable-preview`. > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Looks good. This is an interesting area! It's unfortunate that we now have (up to) three separate guard values, it would be nice if we only had one. I have some comments but we don't have to do them now if we want to get this in. src/hotspot/cpu/aarch64/gc/shared/barrierSetNMethod_aarch64.cpp line 131: > 129: // decoded from the literal in the instruction. Verification will > confirm > 130: // that this instruction corresponds to a load. > 131: _default_entry_instruction = nm->code_begin() + > nm->frame_complete_offset() + entry_barrier_offset(nm); Now that this is the only place calling `entry_barrier_offset` it would be really nice to make it return a positive value and subtract here instead to make this easier to read and understand. src/hotspot/cpu/aarch64/gc/shared/barrierSetNMethod_aarch64.cpp line 224: > 222: uint32_t* addr = (uint32_t*) instruction; > 223: uint32_t inst = *addr; > 224: if ((inst & 0xff000000) != 0x18000000) { Pre-existing: We should probably follow up on this in mainline later on, this is hard to read. src/hotspot/cpu/aarch64/gc/shared/barrierSetNMethod_aarch64.cpp line 293: > 291: > 292: NativeNMethodBarrier barrier(nm); > 293: barrier.set_values(value, bit_mask); I see, so this is where we initially only set the "default" guard value, and now we're setting all of them. Neat. ------------- Marked as reviewed by jsikstro (Committer). PR Review: https://git.openjdk.org/valhalla/pull/2389#pullrequestreview-4236541716 PR Review Comment: https://git.openjdk.org/valhalla/pull/2389#discussion_r3195871044 PR Review Comment: https://git.openjdk.org/valhalla/pull/2389#discussion_r3195861542 PR Review Comment: https://git.openjdk.org/valhalla/pull/2389#discussion_r3195926815
