execution/commitment: make the storage-branch cache route allocation-free - #23189
execution/commitment: make the storage-branch cache route allocation-free#23189awskii wants to merge 7 commits into
Conversation
…free storageRoute ran on every storage-branch Get/Put/Invalidate/PinEntry once any contract is pinned, and rebuilt the 32-byte account hash the slow way — a CompactToHex expansion plus a second 32-byte alloc — even though ContractHashFromPrefix already computes exactly that value with no allocations, proven equivalent by TestContractHashFromPrefix_MatchesReference. Only the shallow fixed-array tier needs the expanded storage nibbles at all; the deep overflow tier is keyed by the original prefix. Also collapses Invalidate's deep-tier Get+Delete into one LoadAndDelete, which hashes the key once and closes the TOCTOU window between the two. storageRoute takes caller-owned scratch for the nibble slice: the function is far past the inlining budget, so returning a slice into a local array heap-promotes it on every call and undoes the win. The now-dead acct return is dropped. BenchmarkPreloadDrain, Apple M5 Max (18 cores), -benchtime=100x -count=10: d67 sec/op 1.558m -> 1.406m -9.72% B/op 4.954Mi -> 4.488Mi -9.42% d68 sec/op 33.83m -> 29.87m -11.69% B/op 108.3Mi -> 100.8Mi -6.90% allocs/op -16.37% (d67) / -15.82% (d68) All p=0.000, n=10. execution/commitment/... and db/state/... green, plus -race on TestBranchCache*.
There was a problem hiding this comment.
Pull request overview
This PR optimizes the execution/commitment BranchCache pinned-storage routing hot path to avoid per-lookup allocations by reusing the existing zero-allocation account-hash extractor and decoding only the shallow storage-nibble prefix needed for the fixed-array tier. It also tightens deep-tier invalidation to a single atomic delete operation and adds regression tests to lock in correctness and allocation behavior.
Changes:
- Update
storageRouteto useContractHashFromPrefixand a newstorageNibbleshelper with caller-provided scratch space to keep lookups allocation-free. - Replace deep-tier
Get+DeleteduringInvalidatewithLoadAndDeleteto avoid double hashing and remove the TOCTOU window. - Add tests to validate
storageNibblescorrectness against thenibbles.CompactToHexreference and to assert zero allocations forstorageRoute.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| execution/commitment/branch_cache.go | Makes storage-branch routing allocation-free and updates deep-tier invalidation to use LoadAndDelete. |
| execution/commitment/branch_cache_test.go | Adds correctness + zero-allocation coverage for the new nibble decoding and routing logic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Code review of the current head. Findings, most important first. 1. The hash decode moved in front of the
|
| ns/op | |
|---|---|
main |
5.75 – 9.09 |
| PR | 14.33 – 20.56 |
Even-parity is flat (5.3 → 5.9). Roughly half of storage-branch prefixes are odd-flag, and db/state/execctx/domain_shared.go:1209 hits this on every commitment branch read.
Fix, verified back to 5.2–6.2 ns/op with TestBranchCache* / TestStorageNibbles* still green: keep the len(prefix) < 33 guard first, and move ContractHashFromPrefix after the c.pinned.Load() == nil check.
2. Check-then-act on the create path can drop a whole storage trunk — branch_cache.go:461
p.Get(packed) followed by pinnedForWrite().Set(packed, st) is not serialized by the put stripe: putStripe keys on prefix[len-1] ^ prefix[0], so two storage slots of the same contract land on different stripes. Both goroutines see no pinned trunk, both newStorageTrunk, both Set(packed, ...) — the loser's trunk is orphaned, its entry unreachable, and pinnedEntries was already incremented for it.
Reproduced with a probe (two goroutines PinEntry-ing prefixes sharing 64 account nibbles, differing in byte 33): 63/200 iterations lost a pin on the PR head, 55/200 on base. So it is pre-existing, but it is in the function this PR rewrote, and it is the same TOCTOU class the PR closes two lines away in Invalidate.
Latent today because AdaptivePinController.mu happens to serialize the only two callers (preload.go:113, preload_parallel.go:186) — but the type doc claims concurrency safety and PinEntry takes a stripe lock precisely to be callable concurrently. maphash.Map.LoadOrStore already exists here; accounting then keys off !loaded.
3. store() still has the non-atomic pair that Invalidate just dropped — branch_cache.go:662
Invalidate now uses LoadAndDelete, and it runs lock-free from Get's stale path. store() still does deep.Get + deep.Set (and slot.Load() != nil + slot.Store).
Interleaving: reader A finds a stale-epoch entry → Invalidate(prefix) with no stripe held → LoadAndDelete removes it and does pinnedEntries.Add(-1). Writer B, holding only its own stripe, is inside store(): it already saw deep.Get(prefix) → exists, so it does deep.Set(prefix, entry), re-inserting without Add(1).
The counter then permanently under-reports and can go negative after enough interleavings. PinnedCount() feeds mxPinnedEntries (trunk_pin_metrics.go:51) and the preload/adaptive-pin budget logs (preload.go:184, preload_parallel.go:346, adaptive_pin.go:254). The mirror image on the fixed-slot branch (slot.Load() != nil then slot.Store) refills a slot Invalidate just decremented, so a later Invalidate double-decrements.
4. storageNibbles is computed on the miss path — branch_cache.go:446
Once any contract is pinned, c.pinned stays non-nil until Clear, so the !create early-out stops firing. Every subsequent storage-branch Get for a non-pinned contract — the overwhelming majority, since pins are capped by MaxPromotedContracts — decodes up to 4 nibbles and builds a slice, returns ok=false, and the caller discards stor.
stor is only ever consumed by st.slot(stor, ...) inside if ... ok { blocks. Moving n := storageNibbles(prefix, nibBuf) into the if st, found := p.Get(packed); found branch and the create branch takes it off the miss path.
5. stor = prefix placeholder rests on an unstated invariant, and the deep tier has no coverage — branch_cache.go:448
The comment says slot() only inspects len(path) past 4 — true only because n > 4 implies len(prefix) >= 35. Any future change to trunk.slot's switch (a case 5: tier), to maxDepth, or to the prefix encoding turns a routing decision into an index into d0..d4 using raw compact bytes as nibbles. That is a wrong-branch service — a wrong trie root, not a crash.
Coverage today: TestBranchCache_StorageTrunkPin only covers even-flag depth 0; the new TestBranchCache_StorageRoute_ZeroAlloc pins depth 0 and 3 but never Gets them back; nothing reaches the stor = prefix branch at all. I confirmed with a throwaway probe that PinEntry → Get → Invalidate round-trips correctly for depths 0..8 in both parities — that probe is what the suite is missing.
A cleaner shape: return (nib [4]byte, n int) and let each caller form its own path slice, which deletes both the out-param and the placeholder.
6. storageNibbles has no length precondition — branch_cache.go:508
It reads prefix[0] and computes n = 2*len(prefix)-2-64 with no guard. storageNibbles([]byte{0x00}, &buf) returns n = -64, and the caller's nibBuf[:n] panics with slice bounds out of range.
The docstring documents the terminator-flag precondition but not the length one, and the guard now lives in a different function (ContractHashFromPrefix) than it used to — the inline len(prefix) < 33 check was deleted. Either restate the precondition or add if len(prefix) < 33 { return 0 }.
7. The two parity branches of storageNibbles collapse into one loop — branch_cache.go:517
Two 10-line loops that differ only by byte offset and inverted nibble parity, so a future fix has to be applied twice and the 32+(i+1)/2 vs 33+i/2 derivations each re-verified. Equivalent single-loop form, verified byte-identical against the current code over 200k random prefixes (len 33-52, both parities):
off := 2
if prefix[0]&0x10 != 0 {
off = 1
}
n = 2*len(prefix) - 64 - off
if n > 4 {
return n
}
for i := range n {
j := 64 + i + off
if b := prefix[j/2]; j&1 == 0 {
nib[i] = b >> 4
} else {
nib[i] = b & 0x0f
}
}It also derives n from the same off, removing the separate if odd { n++ }.
8. TestBranchCache_StorageRoute_ZeroAlloc never calls c.Close() — branch_cache_test.go:493
NewBranchCache does adaptiveTrunkDepth(activeBranchCaches.Add(1)). A leaked count is never repaid, so past trunkInstanceDepthThreshold = 10 live caches every new cache gets trunkDepthShallow = 2 instead of 4. That pushes later tests toward shallow trunks, where depth-3/4 branches route to deep/the LRU tail instead of the fixed arrays — silently changing which tier assertions like require.Equal(t, uint64(1), c.trunkHits.Load()) actually exercise.
Every newer test in the file (_ClearRacingPut_EpochAlias, _ClearFencesStartedPut, _ClearFencesStartedPinEntry, _StateKeyNeverCached, _ShardedTailUnwindAcrossShards, _ConcurrentTailGrow) does defer c.Close(); this one is the odd one out.
9. The new test never reads back what it pinned — branch_cache_test.go:505
c.PinEntry(odd, []byte("odd"), 0, 100) is followed only by c.storageRoute(prefix, false, &nibBuf) with all three results discarded, and then an allocation-count assertion.
A decode producing the right nibble values but the wrong n (an off-by-one on parity, say) would route a pin into d2 and a read into d3 — a permanent pinned miss, with TestStorageNibbles_MatchesReference still green if the reference agreed. Adding got, _, ok := c.Get(prefix); require.True(t, ok); require.Equal(t, want, string(got)) for both prefixes, plus one depth > 4 prefix (currently zero coverage), closes it.
10. c.pinned.Load() runs twice per call — branch_cache.go:442
Once for the fast-path check, once for if p := c.pinned.Load(). Two atomic loads of the same pointer on the per-branch-read hot path, and the second can observe a different value than the first if Clear stores nil between them — so the two reads are not even guaranteed consistent. Fixing item 1 rewrites this block anyway: hoist it to p := c.pinned.Load(); if !create && p == nil { return nil, nil, false } and reuse p.
11. Nit: the new doc comments are over the repo comment budget — branch_cache.go:430
.claude/rules/comments.md: "Keep it concise and why-focused... Two clear sentences beat both a cryptic one-liner and a six-line essay." Root CLAUDE.md: "Most comments fit in a sentence or two."
The five-line nibBuf paragraph ("storageRoute is too large to inline, so a value it returned a slice into would otherwise force a heap allocation on every call; letting the caller's own stack array back it instead keeps the common lookup allocation-free") is the PR description inlined into the source. One sentence carries the same warning: "caller-owned scratch: storageRoute cannot inline, so a returned slice into a local would heap-allocate." Same for the eight-line storageNibbles docstring, where the "n > 4 routes to the deep overflow map, which is keyed by prefix directly and never inspects these nibbles" clause restates what the code and the call site already show.
… two pin races storageRoute decoded the account hash, and then the storage nibbles, before the pinned-map check that discards both. With nothing pinned a storage-branch read paid the whole decode to learn there were no pins; once anything was pinned it paid it again for every non-pinned contract. Length guard and pinned check now come first, and pinned is loaded once instead of twice with no guarantee the two reads agreed. Measured on a 34-byte odd-flag prefix with nothing pinned, 2e6 iters x5: 9.812 -> 1.498 ns/op odd, 2.228 -> 1.344 ns/op even. The create path did Get-then-Set on the pinned map. Two storage slots of one contract can take different put stripes, so both racers could miss, both build a trunk, and the loser's trunk and its already-counted entry were dropped: 45-56 lost pins per 200 attempts, never zero. LoadOrStore hands both the same trunk, and 200 attempts now lose none. store() re-populated a pinned slot behind Invalidate's back — it checked the slot non-nil, then stored, while Invalidate cleared it lock-free and decremented. The entry came back without a matching increment, so PinnedCount drifted and reached -2 under -race. The slot now takes a CAS loop and the deep tier a new maphash.ReplaceIfPresent, which never inserts, so neither can resurrect what Invalidate removed. slot() takes the nibble count, which drops the stor = prefix placeholder and the unstated invariant that n > 4 implied len(prefix) >= 35. storageNibbles gets the length guard it assumed and collapses to one loop over a single parity offset. The new round-trip test covers storage depths 0-8 in both parities, including the deep tier the suite never reached.
TestRecordPreload_RecordsElapsedAndBytes differences a float64 counter, which lands an ulp under the bound however much real time elapsed. It reports 0.04999999999999999 against a 0.05 lower bound. Whether it fires depends on the magnitude the shared accumulator already carries, so it turns on test order.
|
All eleven addressed in 43bf315, plus one unrelated fix in 15a71dc. Measurements below are from this machine, both trees benchmarked back to back. 1, 10 — decode ahead of the early-out, and
Ranges do not overlap in either parity. One correction to your note: even-parity is not flat. 4 — nibble decode on the miss path. Same fix covers it: 2 — check-then-act on create. Confirmed and reproduced: two prefixes sharing a contract hash, differing in the trailing byte so they take different put stripes, two goroutines pinning concurrently, then both read back. Baseline lost a pin in 45, 56 and 51 of 200 attempts and never scored zero across seven runs. 3 — Worth recording: 5, 7 — the placeholder and the two parity loops. Took the shape you suggested. 6 — no length precondition. Guard added. 8, 9 — the new test. 11 — comment budget. Cut. The Unrelated, found while running the suite: CI never ran on the previous head, so this push is also the first full run for this PR. |
storageRouteruns on every storage-branchGet/Put/Invalidate/PinEntryonce any contract is pinned, and rebuilt the 32-byte account hash the slow way — aCompactToHexexpansion plus a second 32-byte alloc — even thoughContractHashFromPrefixalready computes exactly that value with no allocations, proven equivalent byTestContractHashFromPrefix_MatchesReference.Changes
storageRouteusesContractHashFromPrefix. Only the shallow fixed-array tier needs the expanded storage nibbles; the deep overflow tier is keyed by the original prefix, so the expansion is skipped there entirely.Invalidate's deep-tierGet+Deletecollapses into oneLoadAndDelete, which hashes the key once and closes the TOCTOU window between the two.storageRoutetakes caller-owned scratch for the nibble slice. The function is far past the inlining budget, so returning a slice into a local array heap-promotes it on every call and undoes the win. The now-deadacctreturn is dropped.Worth a close look in review: past depth 4 the returned
storis the prefix itself as a length-only placeholder, valid becausetrunk.slotswitches onlen(path)and only indexes content at len ≤ 4. NewstorageNibblesdecodes the shallow case directly from the compact bytes; it is pinned against thenibbles.CompactToHexreference byTestStorageNibbles_MatchesReference.BenchmarkPreloadDrain, Apple M5 Max (18 cores),-benchtime=100x -count=10:All p=0.000, n=10.
execution/commitment/...anddb/state/...green, plus-raceonTestBranchCache*.