perf: encode a branch node in one pass - #13167
Conversation
|
Claude finished @LukaszRozmej's task in 5m 56s —— View job Review —
|
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | Medium | TrieNode.Decoder.cs:157 |
Missing [SkipLocalsInit] — the 528-byte address-exposed scratch is zeroed in the prologue on every branch encode, giving back a memset the size of the copy the change saves. Unsafe.SkipInit does not clear .locals init. Every sibling encoder in the class has the attribute. Safe here: all bytes in [0, childrenLength) are written before being copied out, so no uninitialized stack can leak into the RLP. Likely also the cause of the ~4.5 ns Encode_Extension regression — try it before [MethodImpl(NoInlining)]. |
| 2 | Low | TrieNode.Decoder.cs:176 |
UseParallel evaluated twice on non-AVX-512 hosts when it returns true; hoist to a local. |
| 3 | Low | TrieNode.Decoder.cs:168-175 |
The block comment restates the commit message, and "Measuring means walking all sixteen children twice" is inaccurate — measuring is one walk. Suggested trim inline. |
| 4 | Low | Nibbles.cs:17-24 |
Floating XML doc binds to private const int StackAllocLengthLimit instead of PackNibbles, putting <param> tags on a const and leaving the two <inheritdoc cref="Nibbles.PackNibbles"/> self-referential. Inherited from the stacked commit, not this one. |
Notes
- Behaviour change on a malformed node (informational, not a finding): a branch whose stored RLP holds an over-long child item was previously measured and re-encoded; it now throws
ArgumentOutOfRangeExceptionout ofSpan.Slice. That is the safe direction and the buffer cannot be overrun, but the escaping exception is not aTrieExceptionlike the rest of this layer. Unreachable for any hash-consistent trie, so I would not gate merge on it. - Tests: I agree the existing suites plus the byte-identical guest output cover the path densely; the one case not obviously pinned is the 528-byte boundary itself (a branch with all sixteen children as
Hash256). A single[TestCase]asserting the new path's output equals the old for a full branch would keep the scratch size honest ifBranchesCountor the inline threshold ever moves. - I could not compile or run the test suite in this environment —
dotnet buildrequires approval here, so the review is static. Nothing in the change looked like it would fail to compile (InlineArraysize is a constant expression,Span<byte> children = scratch;is the writable-variable conversion, andSystem.Runtime.CompilerServices/...Intrinsics.X86are both already imported).
· branchperf/zkevm-one-pass-branch
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| private byte _element0; | ||
| } | ||
|
|
||
| public static CappedArray<byte> RlpEncodeBranch(TrieNode item, ITrieNodeResolver tree, ref TreePath path, ICappedArrayPool? pool, bool canBeParallel) |
There was a problem hiding this comment.
Medium (performance): RlpEncodeBranch is the only encoder in this class without [SkipLocalsInit] (EncodeExtension, EncodeLeaf and HashPreparedBranchPairs all have it, and there is no module-level SkipLocalsInit in Nethermind.Trie). Unsafe.SkipInit(out scratch) only satisfies definite assignment — it does not clear the .locals init flag, and because the scratch is address-exposed (a Span is taken over it) the JIT zeroes all 528 bytes in the prologue of every branch encode. That is a memset of the same size as the copy the change was introduced to save.
It is safe to skip here: every byte in [0, childrenLength) is written before it is copied out (each write advances position by exactly what it wrote), so no uninitialized stack bytes can reach the RLP.
This is also a plausible explanation for the ~4.5 ns Encode_Extension regression reported in the PR body — prologue zeroing is unconditional, so if the JIT inlines the encoders into the RlpEncode dispatcher the extension path pays for the branch path's frame. Worth re-running the benchmark with this applied before reaching for [MethodImpl(NoInlining)].
| public static CappedArray<byte> RlpEncodeBranch(TrieNode item, ITrieNodeResolver tree, ref TreePath path, ICappedArrayPool? pool, bool canBeParallel) | |
| [SkipLocalsInit] | |
| public static CappedArray<byte> RlpEncodeBranch(TrieNode item, ITrieNodeResolver tree, ref TreePath path, ICappedArrayPool? pool, bool canBeParallel) |
| if (Avx512F.VL.IsSupported || UseParallel(canBeParallel, item)) | ||
| { | ||
| contentLength = valueRlpLength + (UseParallel(canBeParallel, item) | ||
| ? GetChildrenRlpLengthForBranchParallel(tree, ref path, item, pool, canBeParallel) | ||
| : GetChildrenRlpLengthForBranch(tree, ref path, item, pool, canBeParallel)); |
There was a problem hiding this comment.
Low (performance): on a non-AVX-512 host Avx512F.VL.IsSupported folds to false, so the || evaluates UseParallel in the condition and then again in the ternary — the 16-iteration _nodeData scan runs twice on every parallel encode. Only the root branch takes that path, so it is small, but hoisting is free and no worse on AVX-512 (where the short circuit means the ternary is the single call site anyway).
| if (Avx512F.VL.IsSupported || UseParallel(canBeParallel, item)) | |
| { | |
| contentLength = valueRlpLength + (UseParallel(canBeParallel, item) | |
| ? GetChildrenRlpLengthForBranchParallel(tree, ref path, item, pool, canBeParallel) | |
| : GetChildrenRlpLengthForBranch(tree, ref path, item, pool, canBeParallel)); | |
| bool useParallel = UseParallel(canBeParallel, item); | |
| if (Avx512F.VL.IsSupported || useParallel) | |
| { | |
| contentLength = valueRlpLength + (useParallel | |
| ? GetChildrenRlpLengthForBranchParallel(tree, ref path, item, pool, canBeParallel) | |
| : GetChildrenRlpLengthForBranch(tree, ref path, item, pool, canBeParallel)); |
| // The children have to be measured before they can be written, because the sequence | ||
| // header carries their length and CappedArray has no offset to write it backwards into. | ||
| // Measuring means walking all sixteen children twice - resolving each one, decoding the | ||
| // old RLP to find the next - which is the whole of GetChildrenRlpLengthForBranch. Where | ||
| // there is nothing else to gain from the walk, write the children into a scratch buffer | ||
| // instead and copy them in behind the header: one bounded copy for a whole pass. On a | ||
| // host with AVX-512 the measuring pass also collects full branches for batched hashing, | ||
| // which is worth more than the walk costs, so that target keeps it. |
There was a problem hiding this comment.
Low (comment quality): this reads as the commit message rather than a code comment, and one line of it is wrong: "Measuring means walking all sixteen children twice" — measuring is one walk; the pair of walks is measure + write. The parts that earn their place are the two facts a reader cannot derive from the code (the header carries the content length and CappedArray has no offset to write it backwards into; the AVX-512 measuring walk also collects full branches for batched hashing). The rest restates GetChildrenRlpLengthForBranch. Suggest trimming to roughly:
// The sequence header carries the children's length and CappedArray has no offset to write it
// backwards into, so the length must be known first. Writing into a scratch and copying in
// behind the header avoids the measuring walk. On AVX-512 that walk also collects full branches
// for batched hashing (HashPreparedBranches), which is worth more than the walk costs.
There was a problem hiding this comment.
Trimmed in edfc43c — the inaccurate "walking all sixteen children twice" line is gone; what's left is the header/CappedArray constraint and the AVX-512 walk feeding HashPreparedBranches.
| /// <summary>Packs <c>2 * count</c> nibble bytes into <paramref name="count"/> whole bytes.</summary> | ||
| /// <param name="nibbles">The nibbles, high nibble first, each byte holding one nibble.</param> | ||
| /// <param name="bytes">Destination for the packed bytes.</param> | ||
| /// <param name="count">Number of bytes to write.</param> | ||
| /// <remarks>Split per target; see <c>Nibbles.std.cs</c> and <c>Nibbles.zkevm.cs</c>. | ||
| /// Caller guarantees <paramref name="nibbles"/> holds <c>2 * count</c> bytes and | ||
| /// <paramref name="bytes"/> has room for <paramref name="count"/>.</remarks> | ||
|
|
There was a problem hiding this comment.
Low (inherited from the stacked commit a06e7be, not this PR's top commit): this doc block is not attached to PackNibbles — there is no declaration after it, so the compiler binds it to private const int StackAllocLengthLimit. That gives a const three <param> tags (CS1572/CS1573 if XML doc generation is ever enabled on this project), and leaves <inheritdoc cref="Nibbles.PackNibbles" /> in Nibbles.std.cs / Nibbles.zkevm.cs resolving to the implementations themselves rather than to a shared contract.
SpanExtensions.SeedHashes in this same stack gets it right: it declares a public static partial void in the shared file and carries the doc there. Either use a partial-method declaration here too, or move the doc onto the two per-target implementations.
There was a problem hiding this comment.
Resolved by the rebase — #13164 has merged, so Nibbles.cs is no longer in this PR's diff.
EXPB Benchmark ComparisonRun: View workflow run superblocksScenario: Client Processing (SSE)
K6 TTFB
realblocksScenario: Client Processing (SSE)
K6 TTFB
fusakaScenario: Client Processing (SSE)
K6 TTFB
|
| // which is worth more than the walk costs, so that target keeps it. | ||
| if (Avx512F.VL.IsSupported || UseParallel(canBeParallel, item)) | ||
| { | ||
| contentLength = valueRlpLength + (UseParallel(canBeParallel, item) |
There was a problem hiding this comment.
UseParallel(canBeParallel, item) runs in the guard at line 176 and again here on the non-AVX-512 path. Could a single bool useParallel local feed both the guard and this ternary, so its child walk is not repeated?
There was a problem hiding this comment.
Good catch — hoisted into a bool useParallel local in edfc43c, so the sixteen-child scan runs once on either path.
A branch's children were walked twice: once to measure, because the sequence header carries the content length and CappedArray has no offset to write the header backwards into, and once to write. The measuring walk resolves every child and decodes the old RLP to find the next, which is the whole of GetChildrenRlpLengthForBranch. Writing the children into a bounded stack scratch first and copying them in behind the header trades that walk for one copy. Hosts with AVX-512 keep the measuring pass: it also collects full branches for batched hashing, which is worth more than the walk costs.
e9442de to
55c4d86
Compare
03540df to
4efd897
Compare
The base branch was changed.
|
@claude re-review |
|
Claude finished @LukaszRozmej's task in 2m 31s —— View job Re-review —
|
| Check | Result |
|---|---|
| 528-byte bound | Exact. GenerateKey (TrieNode.cs:693) embeds a child only when rlp.Length < 32 and the child of a branch is never isRoot, so a child item is 1 (0x80), ≤31 (embedded), or 33 (Rlp.LengthOfKeccakRlp). 16 × 33 = 528 is the max, and it agrees with FullBranchRlpLength = 532 (:32) = 528 + 1 value + 3 header. |
| Returned length == old measured length | Matches case for case in both WriteChildrenRlpBranchNonRlp and ...Rlp. The new position += runLength after the tail-run flush (:606) is what makes the total correct; it was harmlessly absent before because nothing read position there. |
| Destination sizing | resultSpan[position..] is childrenLength + 1 long against a childrenLength copy — fits. Every write in the scratch path goes through a Span, so an over-long branch throws instead of overrunning. |
| AVX-512 gate | Right. The measuring walk's only side effect beyond measuring is PrepareRlp + candidateMask + HashPreparedBranches, entirely behind Avx512F.VL.IsSupported (:351, :456). Nothing is lost on the single-pass targets. |
| Recursion / thread safety | A stack local, not [ThreadStatic], so the recursive ResolveKey → RlpEncodeBranch descent is safe. ~528 B/level, bounded at 64 levels by the hashed key — ≲35 KB worst case. |
| Rental lifetime | Improved: pool.SafeRent now happens after the children are written, so a throw mid-encode no longer strands a rental. |
Findings
| # | Severity | Where | Status |
|---|---|---|---|
| 1 | Medium | TrieNode.Decoder.cs:157 |
Open. Missing [SkipLocalsInit]. Re-confirmed: no module-level SkipLocalsInit in Nethermind.Trie, and EncodeExtension (:35), EncodeLeaf (:103) and HashPreparedBranchPairs (:239) all carry it — RlpEncodeBranch is the only encoder without. Unsafe.SkipInit(out scratch) satisfies definite assignment but does not clear .locals init, and Span<byte> children = scratch makes the local address-exposed, so the JIT zeroes 528 bytes in the prologue of every branch encode — a memset the size of the copy this change exists to save. Safe to skip: every byte in [0, childrenLength) is written before it is copied out. Still my best guess for the ~4.5 ns Encode_Extension regression; worth trying before [MethodImpl(NoInlining)]. |
| 2 | Low | TrieNode.Decoder.cs:180 |
Open. UseParallel(canBeParallel, item) evaluated twice on non-AVX-512 hosts, running its 16-iteration _nodeData scan twice per parallel encode. @AnkushinDaniil raised the same point. Hoisting to a bool local is free and no worse on AVX-512. |
| 3 | Low | TrieNode.Decoder.cs:168-175 |
Open. The block comment reads as the commit message, and "Measuring means walking all sixteen children twice" is wrong — measuring is one walk; the pair is measure + write. The two facts worth keeping are the ones a reader cannot derive from the code (the header carries the content length and CappedArray has no offset to write it backwards into; the AVX-512 walk also feeds HashPreparedBranches). |
| 4 | Low | Nibbles.cs |
Resolved — the floating XML doc is not in master; StackAllocLengthLimit (Nibbles.cs:17) is now bare. No action. |
| 5 | Low | TrieNode.Decoder.cs:158-162 |
New nit. The five hoisted declarations exist only so both branches can share names; each path assigns all of them independently and the AVX-512 path returns before the second. Declaring them inside each branch would read better and keep the frames distinct — which may matter given finding 1. |
One question before merge (not a code defect)
The EXPB comparison posted above is not obviously consistent with the +44 % microbenchmark:
realblocks: +0.48 % AVG / −0.61 % median — noise.fusaka: +3.40 % AVG, +4.13 % median, +2.85 % p90, +2.96 % p95 — positive across every percentile, above the ~1.8 % CV the workflow guidance quotes for flat runs.superblocks: +1.23 % AVG with a +28 % p99; superblocks tail is noisy, so I would not read much into it.
Which way this reads depends on one fact I can't check from here: does the amd64 benchmark box report AVX-512? If it does, the runner takes the old two-pass path and those deltas measure only the enlarged frame and the extra branch — which would make finding 1 more than a micro-optimisation. If it does not, the fusaka numbers point the opposite way from RlpTrieNodeEncodingBenchmark. Either answer is fine, but the "performance is good" label deserves the check — a re-run with [SkipLocalsInit] applied would likely settle both this and the extension regression at once.
Notes
- Behaviour change on a malformed node (informational): a branch whose stored RLP holds an over-long child item was previously measured then re-encoded; it now throws
ArgumentOutOfRangeExceptionout ofSpan.Slicerather than aTrieExceptionlike the rest of this layer. Safe direction, unreachable for a hash-consistent trie — not a merge gate. - Tests: the existing suites plus the byte-identical guest output cover the path densely. The one case not obviously pinned is the 528-byte boundary itself — a branch with all sixteen children as
Hash256. One[TestCase]asserting that encodes identically would keep the scratch size honest ifBranchesCountor the 32-byte embedding threshold ever moves.
Fix findings 1, 2, 3, 5 →
· branch perf/zkevm-one-pass-branch
RlpEncodeBranch was the only encoder in TrieNodeDecoder without [SkipLocalsInit]; Unsafe.SkipInit satisfies definite assignment but does not clear .locals init, so the JIT zeroed all 528 bytes of the address-exposed scratch on every branch encode. Every byte copied out is written first, so skipping the zeroing is safe. Hoist UseParallel so its sixteen-child scan does not run twice on a non-AVX-512 host, and pin the 528-byte bound with a branch whose every child is a hash.
|
@claude re-review |
|
Claude finished @LukaszRozmej's task in 4m 55s —— View job Re-review —
|
| # | Severity | Status |
|---|---|---|
| 1 | Medium | Fixed — [SkipLocalsInit] on RlpEncodeBranch (:157). Re-verified safe: WriteChildrenRlpBranchNonRlp and ...Rlp both advance position by exactly what they write, contiguously, so every byte in [0, childrenLength) is written before children[..childrenLength].CopyTo(...). Unsafe.SkipInit is still required (definite assignment) and is not redundant with the attribute. |
| 2 | Low | Fixed — bool useParallel hoisted (:174), one _nodeData scan on either path. |
| 3 | Low | Fixed — the "walking all sixteen children twice" line is gone; what's left is the header/CappedArray constraint and the AVX-512 walk feeding HashPreparedBranches. Accurate. |
| 4 | Low | Resolved by the rebase (Nibbles.cs out of scope). |
| 5 | Low | Open, now purely cosmetic — see inline. |
Re-verification against edfc43c
| Check | Result |
|---|---|
| 528-byte bound | Exact. GenerateKey (TrieNode.cs:693) embeds only when rlp.Length < 32 && !isRoot, and both writers call path.AppendMut(i) before ResolveKey, so a child is never isRoot. Child items are 1 / ≤31 / 33 bytes → 16 × 33 = 528, matching FullBranchRlpLength = 532 (:32). |
| Returned length | Matches the old measuring walk case for case; position += runLength after the tail-run flush (:605) is what makes it correct. |
| Destination sizing | resultSpan[position..] is childrenLength + 1 against a childrenLength copy. Every scratch write goes through a Span, so an over-long branch throws rather than overrunning. |
| AVX-512 / parallel gate | Right. The measuring walk's only extra side effect (PrepareRlp + candidateMask + HashPreparedBranches) is entirely behind Avx512F.VL.IsSupported (:455, :481). |
New int return plumbing |
WriteChildrenRlpBranch → ternary keeps both callees in tail position; the AVX-512 path discards the value harmlessly. |
| Recursion / thread safety | Stack local, not [ThreadStatic] — the recursive ResolveKey → RlpEncodeBranch descent is safe. ~528 B/level, ≤64 levels. |
New test — Can_encode_branch_with_every_child_a_hash
Good addition, and it compiles as written (BranchesCount is internal with InternalsVisibleTo("Nethermind.Trie.Test"); the TrieNode(NodeType, CappedArray<byte>) ctor exists at TrieNode.cs:296; EnterMultipleScope is already used six times in this file). Asserting 532 pins the widest branch, which is exactly the invariant the scratch size depends on.
One caveat worth knowing rather than fixing: RlpEncode defaults canBeParallel: false, so the test never takes the parallel path — but it does take the two-pass path on any AVX-512 host. So on such a machine the test asserts the right output without exercising the scratch at all. There's no clean way to force the other path from a test (RlpEncodeBranch is private to a nested class), so I'd leave it; just don't read a green run as proof the scratch path was covered.
The open benchmark question from last round — answered
I asked whether the amd64 EXPB box reports AVX-512. It does: the job log for run 33932029915 reports CPU: AMD EPYC 4344P 8-Core Processor (8C16T) — EPYC 4004, Zen 4, which implements AVX-512 including AVX512VL. So Avx512F.VL.IsSupported is true on that runner.
Two consequences:
- EXPB cannot validate this change. The benchmark rig takes the old two-pass path on every branch encode. The
+3.40 % / +4.13 % / +2.85 %fusaka deltas therefore measure nothing this change does — at best they are variance, at worst incidental cost on a path that was meant to be untouched. Equally, a green re-run is only a no-regression check, not evidence of the win. - The host-side win is narrower than the PR body reads.
RlpTrieNodeEncodingBenchmark's +44 % was measured on an AVX2 box; on AVX-512 hardware — which is most modern server silicon, and the benchmark rig — branch encoding is unchanged. The real target is the zkVM guest (riscv64, no AVX-512), where the −1.79 % step count stands. That is exactly what the PR set out to do, so this isn't a criticism of the design; it's just worth not over-reading the "performance is good" label as a mainnet-host win.
Medium (evidence, not a code defect): the only production-hardware measurement on record for this PR shows a consistent regression across every fusaka percentile, and it was taken on 4efd897 — before [SkipLocalsInit] landed. On an AVX-512 host the whole single-pass tail is dead code after the JIT folds IsSupported, so at Tier-1 it should cost nothing; the likeliest explanations are run variance or Tier-0 frame size during warm-up. A single EXPB re-run on edfc43c closes it either way, and given the fix specifically targets prologue zeroing it's cheap to get right before merge rather than after.
gh workflow run run-expb-reproducible-benchmarks.yml --ref perf/zkevm-one-pass-branch -f arch=amd64Findings this round
| # | Severity | Where | Issue |
|---|---|---|---|
| A | Medium | — | Stale EXPB evidence: recorded +3.4 % fusaka regression is from the pre-fix commit and unaddressed. Re-run on edfc43c. No code change implied. |
| B | Low | TrieNode.Decoder.cs:175 |
Comment justifies the AVX-512 arm of the gate but not || useParallel. Inline |
| C | Low | TrieNode.Decoder.cs:163-167 |
Five hoisted declarations shared by two independent paths; cosmetic now that [SkipLocalsInit] is applied. Inline |
| D | Low | TrieNode.Decoder.cs:491 |
// Tail call optimized. now sits inside an expression body, immediately before the ternary. The statement is still true — both callees remain in tail position — but the comment originally justified not writing a ternary, so in its new spot it reads as a leftover. Moving it into the <remarks> of the doc above, or dropping it, would read better. |
Notes
- Behaviour on a malformed node (informational, carried forward): a branch whose stored RLP holds an over-long child item now throws
ArgumentOutOfRangeExceptionout ofSpan.Slicerather than aTrieException. Safe direction, unreachable for a hash-consistent trie, not a merge gate. - I could not compile or run the suite here (
dotnet buildrequires approval), so the compile check above is by inspection of signatures andusingdirectives rather than by building.
Fix findings B, C, D →
· branch perf/zkevm-one-pass-branch
|
@claude re-review |
|
Claude finished @LukaszRozmej's task in 4m 45s —— View job Re-review —
|
| # | Status |
|---|---|
| A (Medium, stale EXPB evidence) | Still open, and now more load-bearing — see below. |
B (comment doesn't cover || useParallel) |
Fixed. The comment now names both reasons the walk can pay for itself: "spreading the children over cores, or collecting branch pairs for batched hashing." Accurate for both arms. |
| C (five hoisted locals) | Fixed by the split — each shape owns its own locals. |
D (// Tail call optimized. in an expression body) |
Resolved. WriteChildrenRlpBranch (:519) now has exactly the shape the sibling GetChildrenRlpLengthForBranch (:312) already had in master. Consistent with the file; withdrawing the nit. |
The narrowed gate — verified
HasBatchableChildPair (:196) must be an upper bound on the walk's candidateMask, or a batching pair would be silently dropped. It is:
| gate | measuring walk (:378, :474) |
|
|---|---|---|
| predicate | is TrieNode { IsBranch: true, Keccak: null } |
same, plus PrepareRlp(...).Length == FullBranchRlpLength |
_nullNode |
excluded (new object(), TrieNode.cs:34) |
excluded explicitly |
unchanged child (null in BranchData) |
excluded | read from the old RLP, never a candidate |
Strict superset, so a false verdict provably means popcount(candidateMask) < 2. And with 0 or 1 candidates the walk buys nothing: HashPreparedBranches (:254) falls through to a plain ResolvePreparedKey() on a lone candidate. Skipping it is equivalent work — ResolveKey → GenerateKey → PrepareRlp + Keccak.Compute (TrieNode.cs:683) versus PrepareRlp + ResolvePreparedKey, with the isRoot clause unreachable because a branch child always has path.AppendMut(i) applied. Same encode count, same TreeNodeHashCalculations metric.
Two-pass when the gate over-approximates is harmless: candidateMask == 0 skips HashPreparedBranches entirely.
Everything else re-verified against ccbc8a6
| Check | Result |
|---|---|
| 528-byte bound | Exact. GenerateKey embeds only when rlp.Length < 32 && !isRoot (TrieNode.cs:693), so a child item is 1 / ≤31 / 33 bytes → 16 × 33 = 528, agreeing with FullBranchRlpLength = 532. |
RentBranch geometry |
children = [position, position + childrenLength) and the value byte lands at sequenceLength - 1 == position + childrenLength — adjacent, non-overlapping, so writing 128 before the children is safe. Identical arithmetic to the old contentLength - valueRlpLength slice. |
[SkipLocalsInit] safety |
Both writers advance position by exactly what they write, contiguously from 0, so every byte in [0, childrenLength) is initialised before the CopyTo. Now correctly scoped to only the method that owns the scratch. |
| Rental lifetime | Single-pass rents after writing, so a mid-encode throw strands nothing; two-pass rents after measuring, as before. |
children escaping RentBranch |
Fine — the span is over the pooled byte[] on the heap, not the returned struct. |
| Recursion / thread safety | Stack local, not [ThreadStatic]; the recursive ResolveKey → RlpEncodeBranch descent is safe. Now ~528 B only on levels that take the single pass. |
useParallel evaluation |
Once, on both paths. |
The test
Can_encode_branch_with_every_child_a_hash is better than I gave it credit for last round. Its children are NodeType.Unknown with a non-null Keccak, so HasBatchableChildPair returns false and it takes the single-pass path on every host, AVX-512 included — the caveat I raised previously is gone. Asserting 532 pins the exact invariant BranchScratch's size rests on.
Findings this round
| # | Severity | Where | Issue |
|---|---|---|---|
| A | Medium | — | Stale benchmark evidence, now materially so. This commit moves most branch encodes on an AVX-512 host onto the single-pass path — the bottom layer of every dirty subtree. The benchmark rig is one (AMD EPYC 4344P, Zen 4, AVX512VL), so its behaviour changes here for the first time in this PR, and the only run on record is from 4efd897 and showed +3.40 % / +4.13 % / +2.85 % on fusaka AVG/median/p90. A fresh EXPB run on ccbc8a6 is the one thing that would either confirm the host-side win or catch a real regression. No code change implied. |
| B | Low | TrieNode.Decoder.cs:202 |
HasBatchableChildPair reads through the INodeData interface indexer — sixteen dispatches added to every branch encode on AVX-512. Hoisting Unsafe.As<BranchData> once, as the two ...Rlp walks already do, makes the new gate nearly free. Inline |
| C | Low | TrieNode.Decoder.cs:226 |
The split's stated goal — scratch out of the frame of the path that never uses it — is only a guarantee if RlpEncodeBranchSinglePass isn't inlined back into the dispatcher. HashPreparedBranchPairs (:267) already pairs [SkipLocalsInit] with [MethodImpl(NoInlining)] for the same reason. Inline |
Notes
- Malformed-node behaviour (informational, carried forward but now higher-stakes): a branch whose stored RLP holds an over-long child item throws
ArgumentOutOfRangeExceptionout ofSpan.Slicerather than aTrieException. Previously that only affected non-AVX-512 hosts and the guest; after this commit it is reachable on mainnet servers. Still the safe direction — the buffer cannot be overrun — and still unreachable for a hash-consistent trie, sincePrepareRlponly re-encodes nodes that are dirty or locally built. Not a merge gate. - The PR body should be updated for the gate change: it currently says "Hosts with AVX-512 keep the two-pass form", which
ccbc8a6makes true only for branches with two or more dirty branch children. - I could not compile or run the suite here (
dotnet buildrequires approval), so the compile check is by inspection:[InlineArray(BranchesCount * Rlp.LengthOfKeccakRlp)]is a constant expression,Span<byte> scratchSpan = scratch;is the writable-variable conversion,TrieNode.BranchesCountis reachable from the test project viaInternalsVisibleTo, and a comment between=>and the expression body is legal.
Fix findings B, C →
· branch perf/zkevm-one-pass-branch
The AVX-512 target kept the two-pass form for every branch, but the second walk only pays for itself when HashPreparedBranches can pair child hashes, and only a dirty branch child is ever a candidate - a lone one is hashed on its own. Gate on there being two of them, so a branch without a pair - the whole bottom layer of any dirty subtree - takes the single pass on an AVX-512 host too. The gate is an upper bound on what the walk would collect (a candidate whose RLP is not a full branch drops out during the walk itself), so no batching opportunity is lost, and both paths encode identically either way. Guest steps for block 25532382 are unchanged at 418,197,507 vs 418,197,455 (+52): Avx512F.VL.IsSupported folds to false on riscv64, so the scan is eliminated there.
HasBatchableChildPair runs on every branch encode on an AVX-512 host, so its sixteen reads should not each go through the INodeData indexer. Hoist the cast once, as the two Rlp walks already do.
ccbc8a6 to
b8262e7
Compare
|
@claude re-review |
|
Claude finished @LukaszRozmej's task in 4m 56s —— View job Re-review —
|
| # | Status |
|---|---|
1 (Medium, [SkipLocalsInit]) |
Fixed in edfc43c, and still correct after the split revert — the attribute is on RlpEncodeBranch (:157), the only method that owns the scratch. |
2 (Low, double UseParallel) |
Fixed — one call at :174, feeding both the gate and the ternary. |
| 3 (Low, comment restated the commit message) | Fixed — :169-173 now carries only the header/CappedArray constraint and the two reasons the walk earns its keep. Accurate for both arms of the gate, which closes finding B as well. |
4 (Low, Nibbles.cs doc) |
Resolved by the rebase. |
| A (Medium, stale EXPB evidence) | Closed. The PR body now records two post-gate EXPB runs (realblocks −0.05 %/+2.12 % AVG, fusaka −1.10 %/−0.59 %, superblocks +0.20 %/−0.03 %) and states plainly that the rig reports AVX-512 so pre-gate runs never took the new path. The +3.4 % fusaka reading I flagged came from the bot comment on 4efd897, which predates the gate and is superseded. Two runs disagreeing by 2.2 % on near-identical code is the right thing to record as the rig's resolution floor. |
B (Low, comment didn't cover || useParallel) |
Fixed. |
C (Low, NoInlining on the split method) |
Moot — 78e7038 removes the separate method, so there is nothing left to keep un-inlined. Withdrawn. |
D (Low, // Tail call optimized.) |
Resolved — WriteChildrenRlpBranch (:511) now has exactly the shape GetChildrenRlpLengthForBranch (:305) already had in master. Consistent with the file. |
What I verified this round
The b8262e7 hoist is safe, and adds no new dereference risk. BranchData is sealed and the only INodeData a NodeType.Branch ever carries — CreateNodeData (TrieNode.cs:330) and ResolveNode (:651) are the only producers, and the clone ctor (:271) copies whatever the source had. More to the point, both writers already dereference item._nodeData unconditionally on every branch encode (TrieNode.Decoder.cs:523, :569), so HasBatchableChildPair cannot be reached with a _nodeData the walk would have tolerated. BranchData's indexer is ref _branches[index] over an [InlineArray(16)] (NodeData.cs:42-51), and i < BranchesCount == 16, so the index is in range. Same Debug.Assert + Unsafe.As shape as GetChildrenRlpLengthForBranchRlp (:450) and WriteChildrenRlpBranchRlp (:568).
The narrowed gate is still a strict upper bound on the walk's candidate set. Both measuring walks apply childNode is { IsBranch: true, Keccak: null } and rlp.Length == FullBranchRlpLength (:371-376, :476-481); the gate applies only the first. null (unchanged child) and _nullNode both fall out of the is TrieNode test on either side. So false provably means fewer than two candidates, and with 0 or 1 the walk buys nothing — HashPreparedBranches (:250) falls through to a plain ResolvePreparedKey on a lone candidate, which is the same PrepareRlp + Keccak.Compute work ResolveKey → GenerateKey (TrieNode.cs:686-700) does on the single pass, with the same TreeNodeHashCalculations increment and the same isRoot outcome (a branch child always has path.AppendMut(i) applied, so isRoot is false on both sides). No pair can be dropped.
The gate reorder is free where it matters. useParallel || short-circuits the scan away on the parallel path, and Avx512F.VL.IsSupported folds to a constant, so HasBatchableChildPair is dead code on every non-AVX-512 target including the riscv64 guest.
Everything from the earlier rounds still holds against b8262e7: the 528-byte bound is exact (GenerateKey embeds only below 32 bytes, so a child item is 1 / ≤31 / 33 → 16 × 33 = 528, matching FullBranchRlpLength = 532); the single-pass destination resultSpan[position..] is childrenLength + 1 against a childrenLength copy; position += runLength after the tail-run flush (:626) is what makes the returned total correct; and [SkipLocalsInit] is safe because both writers advance position contiguously from 0 by exactly what they write, so every byte in [0, childrenLength) is initialised before the CopyTo.
Robustness note in the change's favour: giving the writer the full 528-byte scratch and deriving the length from what it wrote makes a measure/write disagreement structurally impossible on the single-pass path, where previously an exactly-sized destination was the only thing catching it.
The existing AVX-512 test still passes under the narrowed gate. Resolves_full_branch_children_to_their_individual_hashes (TrieNodeTests.cs:328-334) asserts hash equality, not which path ran, and every case keeps ≥2 gate candidates anyway — even 0x0001, because the nonCandidate branch child at index 15 is also { IsBranch: true, Keccak: null }. So all five cases still take the two-pass walk and the assertions are path-independent regardless.
The new test takes the path it is meant to. Can_encode_branch_with_every_child_a_hash's children are NodeType.Unknown with non-null Keccak, so HasBatchableChildPair returns false and it exercises the scratch on every host, AVX-512 included. 532 pins the widest branch — the exact invariant BranchScratch's size rests on.
Findings this round
| # | Severity | Where | Issue |
|---|---|---|---|
| — | Low | TrieNode.Decoder.cs:163-167 |
The five hoisted locals are back with the split revert. Cosmetic only now that [SkipLocalsInit] is applied, and 78e7038 removed the split deliberately, so I'm noting it rather than re-raising it — your call. |
| — | Low | TrieNode.Decoder.cs:510 |
WriteChildrenRlpBranch's doc is a bare <returns>, so the two <inheritdoc cref="..." /> writers inherit no <summary>. Fine as-is; a one-line summary on the dispatcher would give all three one. |
Nothing here is worth another round.
Notes
- The 528-byte bound depends on the 32-byte embedding threshold in
GenerateKey, and no test pins that half of it — the new test's children are all hashes, so raising the threshold above 33 would overflow the scratch at runtime rather than fail a test. TheBranchScratchremark (:146-150) documents the dependency, which is the right place for it; I mention it only so the coupling is on the record. - Malformed-node behaviour (informational, carried forward): a branch whose stored RLP holds an over-long child item throws
ArgumentOutOfRangeExceptionout ofSpan.Slicerather than aTrieException. Safe direction, buffer cannot be overrun, unreachable for a hash-consistent trie sincePrepareRlponly re-encodes dirty or locally built nodes. Not a merge gate. - I could not compile or run the suite here (
dotnet buildrequires approval), so the compile check is by inspection:Debug,Unsafe,InlineArray,SkipLocalsInitandAvx512Fare all already imported in this file, andTrieNode.BranchesCountisinternal(TrieNode.cs:27) withInternalsVisibleTo("Nethermind.Trie.Test")(TrieNode.cs:21).
· branchperf/zkevm-one-pass-branch
Its doc was a bare <returns>, so the two <inheritdoc /> implementations inherited no summary either.
4 participants
Changes
Encoding a branch node walked its sixteen children twice. The second walk writes them; the first
exists only to measure, because an RLP sequence header carries the content length and
CappedArrayhasno offset that would let the header be written backwards into the buffer afterwards. Measuring is not
cheap:
GetChildrenRlpLengthForBranchresolves every child and decodes the old RLP to find where thenext one starts — in the zkVM guest that pass is 2.53 % of all steps, against 2.73 % for the pass
that does the work.
Where nothing else depends on the measuring walk, the children are now written into a bounded stack
scratch first, and copied in behind the header once their length is known: one copy of at most 528
bytes in place of an entire pass. The scratch is an
[InlineArray]struct local rather than astackalloc, for the reasonKeccakHash's state buffer documents — localloc would pin the method atTier0-FullOpts and add stack-probe overhead per call — and everything is written through a
Span, so abranch that somehow exceeded the buffer throws rather than running past it.
The measuring walk is kept where it earns its cost. The parallel path keeps it. On a host with AVX-512
it is kept for a branch with at least two dirty branch children, because those are the only children
HashPreparedBranchescan pair up for batched hashing — a branch without such a pair gains nothingfrom a second walk, so the whole bottom layer of any dirty subtree takes the single pass on AVX-512
too. That gate is an upper bound on what the walk would collect (a candidate whose RLP is not a full
branch drops out during the walk itself), so no batching opportunity is lost either way.
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Can_encode_branch_with_every_child_a_hashpins the 528-byte scratch bound against the widest branchthere is, and round-trips it back through the decoder. Beyond that the existing suites cover this
encoder densely —
Nethermind.Trie.Test537/0,Nethermind.State.Test1283/0,Nethermind.Blockchain.Test1772/0 — andnethermind-tests-checked.ymlruns all three in theno-intrinsicsvariant, so the single-pass path is exercised on every PR whatever the runner's CPUhappens to be. The guest output for mainnet block 25532382 is byte-identical, which exercises the new
path on every branch commit in a real block.
Caveat: the AVX-512 arm of the gate cannot be exercised locally — this box is AVX2 — so it rests on
review plus whichever CI runners happen to have AVX-512. Both arms encode identically, so a wrong gate
would cost throughput, not correctness.
Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
ziskemusteps for mainnet block 25532382,bflat-riscv64+zisk:1.2.0-alpha,-Ot:3bb4807754The trie is 21.6 % of guest steps and node re-encoding is 6.45 % of it, so this takes roughly a quarter
of the encoder. The guest has no AVX-512, so it takes the single pass unconditionally.
The batching gate was measured separately against the tip of this branch (same emulator,
-Ot, outputbyte-identical): 418,197,507 steps against 418,197,455, i.e. +52.
Avx512F.VL.IsSupportedfolds tofalse on riscv64, so the candidate scan is eliminated in the guest and the gate is a host-only change.
For the record, tidying
RlpEncodeBranch's duplicated tail was tried and dropped: splitting it intoRlpEncodeBranchTwoPass/RlpEncodeBranchSinglePasscosts +0.73 % guest steps (421,244,816), ashared rent-header helper the same, and
AggressiveInliningon all of them does not recover it(421,327,494) - ILC already inlines them. Folding the two tails into one branch inside the single
method costs the same +0.73 %. The duplication stays because every alternative measured worse.
Host side
RlpTrieNodeEncodingBenchmark, BenchmarkDotNet ShortRun, quiet machine, run base → branch → base sodrift is visible (this box is AVX2, so it takes the single-pass path):
Encode_BranchEncode_ExtensionEncode_LeafBranch encoding is about 44 % faster, bracketed by both baseline runs.
On the extension reading above: re-measuring it five arms interleaved (base -> branch -> base ->
branch -> base) shows the box drifting monotonically,
Encode_Leafincluded - untouched by any commithere and still spreading 33 % (29.20 -> 33.17 -> 30.18 -> 23.57 -> 22.17 ns). Normalised against that
control there is no extension signal in either direction, so the ~4.5 ns is drift, not the scratch.