perf: accelerate EVM word byte swap on ARM64 - #12798
Conversation
|
Claude finished @kamilchodola's task in 5m 28s —— View job Review: ARM64 AdvSimd byte swap
No Critical or High findings. The intrinsic sequence is correct — I traced the byte algebra rather than taking the PR description at its word, and the composition is a genuine full 32-byte reversal. Details in the inline comment. What's right
Findings
One thing worth checking when the benchmark runs: SecurityNo concerns. No new public surface, no untrusted-input handling, no allocation, no unsafe blocks. VerdictCorrectness is sound and I'd be comfortable with the code as written once the doc comment is updated. The gate is the missing measurement — for a perf-only change, the Graviton number is the deliverable.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
EXPB Benchmark ComparisonRun: View workflow run superblocksScenario: Client Processing (SSE)
K6 TTFB
realblocksScenario: Client Processing (SSE)
K6 TTFB
fusakaScenario: Client Processing (SSE)
K6 TTFB
|
|
@claude re-review |
|
Claude finished @kamilchodola's task in 5m 25s —— View job Re-review: ARM64 AdvSimd byte swap (commit
|
| Prior | Status |
|---|---|
| Medium — benchmark won't yield a trustworthy number | ✅ Resolved. OperationsPerInvoke = 4 + serial word = word.ByteSwap() chain. Loop-carried dependency blocks hoisting across BDN's unroll; the return value blocks DCE. Detail → |
| Medium — perf claim unmeasured | |
Low — XML <summary> incomplete |
✅ Resolved. EvmWordExtensions.cs:19 now documents the ARM64 path alongside the other three. |
| Low — test placement | ✅ Resolved. Moved to Nethermind.Core.Test/EvmWordExtensionsTests.cs. Root placement matches the sibling convention there (EncodingExtensionsTests.cs, SizeExtensionsTests.cs, TransactionExtensionsTests.cs). Verified Nethermind.Core.Test is in the ubuntu-24.04-arm matrix (nethermind-tests.yml:52-63). |
| Low — test strength | ✅ Resolved, and better than what I suggested. |
On the rewritten test
EvmWordExtensionsTests.cs is a genuine improvement over both the original and my own suggestion:
MemoryMarshal.Read<EvmWord>+Array.Reverseis a cleaner oracle than the hand-writtenulongliterals. The old test encoded the expected result as big-endian-packedulongs, which required the reader to do the endianness reasoning by hand. Reading both input and expected through the same byte-order path makes the assertion independent of that.- Deterministic ramps beat my seeded-
Randomsuggestion.input[i] = (byte)(seed + i)yields 32 distinct byte values for every one of the four seeds, and 32 distinct bytes is a complete permutation oracle — any index-mapping error is caught by a single case.Randomwould add non-reproducibility on failure for no extra detection power.[TestCase(0xff)]also exercises theuncheckedwrap. Good call. - The double-swap round-trip catches half-swap errors that a symmetric input could mask, and
Assert.EnterMultipleScope()has precedent in the repo (Nethermind.Xdc.Test).
EvmWord resolves via the project-wide alias in src/Nethermind/Directory.Build.props:33 (Vector256<byte>), so it's in scope in Nethermind.Core.Test with no extra using — confirmed by the passing build.
CI verification (this commit)
| Check | Result |
|---|---|
Nethermind.Core.Test (ubuntu-24.04-arm) |
✅ success — the AdvSimd branch runs and the assertions hold on real ARM64 |
Nethermind.Core.Test (ubuntu-latest) |
✅ success — AVX path unregressed |
Build (release, Benchmarks) / (debug, Benchmarks) |
✅ success — benchmark compiles |
Code formatting, Code Lint, Spell check |
✅ success |
Full Nethermind/Ethereum tests |
✅ success, 0 failures |
Remaining gate: the number
This is an optimization-only change, so the measurement is the deliverable, and it hasn't landed.
The EXPB comparison posted above is not evidence either way, for two independent reasons: it reports no cached master baseline for any payload set, and unless that runner is arm64 it exercises the AVX2/AVX-512 paths, which this PR does not touch. Don't let those tables stand in for the Graviton result.
What's needed is a two-build A/B on the same Graviton host — master (scalar fallback) vs this branch (AdvSimd) — because the branch is selected at JIT time and so a single run cannot contain both arms. Methodology detail and a caveat about the Vector256-on-arm64 stack round-trip compressing the win are in the inline comment.
Findings
| Severity | Finding |
|---|---|
| Medium | Perf claim unmeasured. Author-acknowledged ("Target ARM64 disassembly/timing remains required before claiming a win") — but the acknowledgment defers the gate rather than clearing it. For a perf-only PR the Graviton delta has to land before merge, otherwise nothing justifies the added path. |
| Low | Scalar fallback now has zero CI coverage. x64 legs take AVX2/AVX-512, arm64 now takes AdvSimd, so no leg reaches lines 43-48. Pre-existing on x64, but this PR closes the last door. Not worth fixing here — exposing the fallback would add surface area, which AGENTS.md discourages. Possible follow-up: a DOTNET_EnableHWIntrinsic=0 leg in Nethermind extra test variants, which would cover this and every other ISA-gated fallback in the codebase at once. |
| — | Deferring the TBL variant to a benchmark-backed follow-up rather than swapping a CI-verified sequence on my untested suggestion is the right sequencing. No objection. |
Correctness, re-confirmed
The intrinsic block is byte-identical to the version I traced in the first pass; only the doc comment above it changed. REV64 (ReverseElement8 on Vector128<ulong>) reverses bytes within each 64-bit lane, ExtractVector128(x, x, 1) at ulong granularity is an 8-byte rotate — i.e. a lane swap — and Vector256.Create(lower, upper) places the reversed upper half into the result's lower half. Composition is exactly r[i] = b[31-i]. The arm64 CI leg now backs that up empirically. Guard style matches HexConverter.cs:163; all four IsSupported checks fold at JIT time, so x64 is untouched.
Security
No concerns. No new public surface, no untrusted-input handling, no allocations, no unsafe.
Verdict
The code is ready to merge as written — correct, ARM64-verified in CI, well-tested, documented, zero x64 blast radius, and every code-level comment addressed. The one thing standing between this and approval is the Graviton measurement.
Note:
dotnetis not permitted in this environment, so I verified compilation and test execution through the PR's CI runs rather than locally.
• branchperf/arm64-evm-word-byteswap
| } | ||
|
|
||
| [Benchmark(OperationsPerInvoke = 4)] | ||
| public EvmWord ByteSwap() |
There was a problem hiding this comment.
Resolved ✅ — OperationsPerInvoke = 4 plus the serial word = word.ByteSwap() chain fixes both problems from my earlier note: the per-invoke work is now ~4 dependent shuffles (well clear of BDN's overhead floor), and the loop-carried dependency means the JIT cannot hoist the computation out of BDN's 16x unroll. The returned value is consumed, so DCE is blocked too. ByteSwap is an involution, so the chain is semantically stable. This compiles clean — Build (release, Benchmarks) and Build (debug, Benchmarks) both passed on this commit.
One methodology point before quoting a number (Low, not a code defect): this benchmark has no baseline arm, and it can't have one — the AdvSimd branch is chosen at JIT time, so a single run only ever measures whichever path the host resolves to. The A/B therefore has to be two builds on the same Graviton host: master (scalar fallback) vs this branch (AdvSimd), same BDN config.
Note that trying to get the "before" arm by disabling the ISA via env var is not a valid substitute: AdvSimd is baseline on arm64, and turning hardware intrinsics off changes codegen for GetLower()/GetUpper()/Vector256.Create as well, so the baseline would be penalised for reasons unrelated to this method and the win would come out inflated.
Worth pairing with [DisassemblyDiagnoser] on the branch run to confirm REV64/EXT actually show up in the generated loop — that also tells you how much of the win the Vector256 stack round-trip is eating, since Vector256<T> is not hardware-accelerated on arm64.
1 participant
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
The ARM64 sequence uses two
REV64operations and twoEXT #8lane rotations, then exchanges the 128-bit halves. This preserves exact full-word reversal while avoiding the scalar four-limb fallback on ARM64.