execution/cache: fence GenericCache generation swaps and make size accounting exact - #22466
Merged
Conversation
…counting exact Split from #22159 (StateCache review findings #22120). - maybeGrow copies and swaps the generation with every put stripe held, so a striped put can no longer land in a retired generation (older value resurfacing as live) and a conditional put can no longer fill a mid-resize gap with a stale value. Grow detection moves inside the stripe, the grow itself outside it; the triggering insert (and racers until the swap) evict at the pre-grow cap — a bounded transient. - Clear performs its counter reset, coherence re-init and generation swap under the same fence. - Delete and the lazy stale-drop run under the key's put stripe, and currentSize is subtracted solely via the OnEvict callback: freelru picks eviction victims per shard (hash bits 16+), which the stripes (bits 0-7) don't cover, so any subtraction computed outside the callback double-counts against a racing capacity eviction. - One Debug line per grow (caps, copied count, alloc/fenced split) so the once-per-lifetime writer stall is self-explaining in logs. - growLRU documents its intentionally unfenced swap contract: safe only for content-addressed layers; counters approximate across grows.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens execution/cache against concurrent writers during GenericCache generation swaps (jump-grow and Clear) and makes currentSize accounting exact under concurrency by routing all size subtraction through OnEvict while serializing intentional removals under per-key put stripes.
Changes:
- Fence
GenericCachejump-grow andCleargeneration swaps by taking all put stripes during the copy/swap window. - Make size accounting exact by striping
Delete/stale-drop, removing “delta” subtraction, and relying onOnEvictas the sole size subtractor. - Add targeted concurrency tests covering grow fencing,
PutIfAbsentsemantics during grow, and size-drift regressions (delete/stale-drop/clear/capacity-eviction).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
execution/cache/grow_lru.go |
Documents that unfenced generation swaps are only safe for content-addressed (immutable-per-key) layers. |
execution/cache/generic_cache.go |
Implements fenced generation swaps, striped removals, and OnEvict-only size subtraction; adds grow observability. |
execution/cache/generic_cache_concurrency_test.go |
Adds concurrency tests for grow correctness and eviction/size-drift scenarios. |
execution/cache/code_cache_concurrency_test.go |
Adjusts addr-binding concurrency test to avoid per-iteration deletes. |
execution/cache/cache_test.go |
Adds domain-cache concurrency tests for delete/stale-drop/clear size drift and adjusts existing if-absent atomicity test. |
Comments suppressed due to low confidence (1)
execution/cache/generic_cache.go:305
- GetWithTxNum may return a miss even if a concurrent Put revives the key between the initial stale read and dropStale's stripe-held re-check. This defeats the purpose of the re-check (keeping revived entries) and also inflates misses/staleEvicted for a key that is now live. Re-read after dropStale and serve the entry if it is no longer stale.
if c.coh.IsStale(e.txNum, e.epoch) {
c.dropStale(h, key)
c.staleEvicted.Add(1)
c.misses.Add(1)
var zero T
return zero, 0, false
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced Jul 14, 2026
putLocked read coh.Epoch() before acquiring the key's stripe, so a put losing the stripe race to Clear (whose coh.Init resets the epoch counter) stamped a pre-Clear epoch onto an entry landing in the post-Clear generation. Once a later unwind re-reached that epoch value, the entry aliased the live epoch and survived IsStale despite its txNum being at or above the floor - serving dead-fork state. Move the epoch sample inside the stripe, next to the generation load the fence already synchronizes. The new test parks Clear on the key's stripe pre-Init and the put behind it (starvation-mode FIFO hands the stripe to Clear first); it failed deterministically before the fix.
…ert window The prober only burst conditional puts after observing the generation swap, but every design publishes the new generation after the copy completes, so the hot key was always present and the test could not fail - with or without the fence. Replace the body: a writer hammers puts of brand-new keys while an insert at a lowered curCap triggers the grow, then every key that straddled the swap is probed with a stale PutIfAbsent. Unfenced, a new-key put landing in the retiring generation after the Keys() snapshot is lost on the swap and the conditional put installs the stale value as live - the test now fails in the first rounds when the fence is removed (20/20 runs). The candidate presence asserts also give a stronger migration-completeness check than the old single hot key.
removeLocked compensated the OnEvict increment by decrementing the evictions counter; a PrintStatsAndReset Swap(0) landing between the two zeroed the counter first, so the decrement underflowed it to ~1.8e19 for the next interval. The remove-then-add update path fires this on every warm update, so the window is open a measurable fraction of wall time on a busy node. Count intentional removals in their own counter instead - both counters are increment-only between resets, so no interleaving with a reset can underflow either - and net them at print time. Removals are snapshotted before evictions: OnEvict bumps evictions before the removal is counted, so this order keeps every captured removal paired with a captured eviction; the clamp absorbs a removal deferred to the next interval. Reported semantics are unchanged: evictions still means capacity evictions only. The new test plays the stats reset against a Delete hammer; it underflowed within milliseconds before the fix.
The -Locked suffix conventionally means the caller holds the lock, which is removeLocked's contract; putStriped acquires the stripe itself.
…movals dropStale's stripe-held re-check keeps an entry a racing put revived, so the counter tracks detections; the old comment implied actual drops.
yperbasis
marked this pull request as draft
July 14, 2026 14:41
OnEvict fires for intentional Removes too, so any scheme that routes them through the evictions counter - decrement compensation or netting against a removals counter at print time - races a concurrent stats reset: the swap straddles the paired updates and reports phantom evictions (or underflows). Count evictions where they happen instead, from freelru.Add's evicted return, and leave OnEvict with size accounting only. This deletes the removals counter and the print-time netting, and surfaces copy evictions in the jump-grow log line.
Left to pick its own geometry per generation, freelru chooses more, smaller shards as capacity rises (it derives the count from GOMAXPROCS and table size), so a new shard receiving more entries than its capacity evicts during the migration copy - keys clustered on the shard-selection bits silently vanish across a grow, and a follow-up conditional put can install a stale value in the hole. Generations now get an explicit shard count: a lineage starts at ~64 entries per shard and the count doubles across grows only while per-shard capacity does not shrink, bounded by freelru's own GOMAXPROCS-derived ceiling. Power-of-two counts make the selection bits nest, so each new shard receives a subset of exactly one old shard and the copy can never overfill one. Steady-state shard counts at full size match what freelru would choose; the jump-grow log line now also reports the shard count.
The update and collision paths removed the old entry before adding the new one and only then adjusted the byte counter, so a concurrent ModeNoOp admission on another stripe could observe the transient dip and admit a key past a full budget - breaking "drop new keys when full". Reserve the new size before the removal instead: the counter transiently over-states usage, which at worst drops a new key, and the settled sums are unchanged.
…he-fences # Conflicts: # execution/cache/cache_test.go # execution/cache/code_cache_concurrency_test.go
Main now enforces modernize's rangeint; the tests added on this branch predate that.
GetWithTxNum judged a captured entry against live coherence, so a Clear completing between the generation load and the staleness check re-inited coherence (fresh epoch, lifted floor) and revalidated an unwind-invalidated entry from the retiring generation - serving dead-fork state. The fix needs both orderings: the reader snapshots coherence before loading the generation, and Clear re-inits coherence only after its swap. Either alone leaves a window - a snapshot taken in Clear's (init, swap) gap still pairs post-init coherence with the retiring generation; the new test caught exactly that on the first fix attempt. An old-generation entry is now always judged by a pre-init snapshot that still carries the unwind, and a live entry judged by a pre-Clear snapshot degrades to a safe miss via dropStale's re-check. The test parks a reader on the fence reaching the key's stripe so its Get lands next to the re-init; it served the dead value within tens of rounds before the fix.
makeAddr stores only byte(round), so the test cycled through 256 addresses and every round past the first cycle found a live leftover - PutIfAbsent deferred to it and the absent-key insert race went unexercised. Encode the full round, as the CodeCache twin does.
yperbasis
marked this pull request as ready for review
July 14, 2026 20:16
Comment on lines
422
to
428
| if c.mode == ModeNoOp { | ||
| // Refuse once full by either bound — freelru would otherwise evict at the | ||
| // entry-count cap, which ModeNoOp ("drop new keys when full") must not do. | ||
| if c.currentSize.Load()+int64(newSize) > int64(c.capacityB) || lru.Len() >= int(c.maxCap) { | ||
| c.dropped.Add(1) | ||
| return | ||
| return false | ||
| } |
This was referenced Jul 16, 2026
mh0lt
approved these changes
Jul 16, 2026
mh0lt
left a comment
mh0lt
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the fences and accounting in full context (fetched the branch, reverted each fix in a scratch worktree to confirm the regression tests fail pre-fix, ran -race). LGTM — approving.
Verified correct:
- Jump-grow fence: uniform lock order (
resizeMu→ all stripes), no self-deadlock (putreleases its stripe beforemaybeGrow), and the capturedoldstays live until the swap so no put lands in a retiring generation unmigrated. Shard-count doubling keeps per-shard capacity from shrinking, so the migration copy can't overfill/evict. - Clear fence + reader ordering: safe under Go seq-cst atomics — a reader that observes post-
Initcoherence also observes the post-swap empty generation (→ miss); it can't pair a post-Clear generation with pre-Clear coherence to serve dead-fork state. - Epoch-under-stripe, exact size accounting (subtract solely via
OnEvict, reserve-before-remove, cap-evictions fromAdd's return), and the growLRU unfenced-swap contract (content-addressed ⇒ lost write = benign miss). - Closes the mid-resize
PutIfAbsentstale-install hole that #22467's if-absent read-fill relies on.
Merge order: this is the prerequisite for #22467 (the if-absent read-fill is only race-safe once these grow/Clear swaps are fenced). #21414 (bg-commit) is being held until this merges. Expected order: #22466 → #22467 → re-merge #21414.
Two non-blocking test-quality notes (fixes are correct; these just don't red→green the invariant):
TestGenericCache_ModeNoOpAdmissionAtomicWithUpdateupdatesawith an identical-sized value, so the byte-counter dip it targets is 0 on both branches — it never exercises the transient. Updating with a different-sized value would actually guard reserve-before-remove.TestGenericCache_ClearRacingPut_EpochAliaspasses onmain(the pre-fix eagercoh.Init()+ unlocked epoch sample can't be forced into the targeted ordering), so it guards against regression but doesn't demonstrate the fix.
JackCC703
pushed a commit
to JackCC703/erigon
that referenced
this pull request
Jul 20, 2026
…fTwo (erigontech#22476) The tree carried three identical `NextPowerOfTwo` implementations: `cl/merkle_tree/utils.go`, `polygon/bor/merkle.go` (byte-for-byte copies of the Stanford bithack), and a private `nextPow2` in `execution/cache/generic_cache.go` (added by erigontech#22466 — neither existing copy is importable from execution code without inverting the layering). All three consolidate into `common/math`, the shared leaf package every call site can import. Behavior-preserving, including the edges: `0 → 1`, exact powers unchanged, and inputs above `1<<63` wrap to 0 (as the bithack did). --------- Co-authored-by: Alex Sharov <AskAlexSharov@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split from #22159 (the #22120 StateCache review findings): the
GenericCacheconcurrency and accounting fixes, self-contained toexecution/cache.What changed
Jump-grow fence.
maybeGrownow copies and swaps the generation with every put stripe held (allocation stays outside the fence). Previously a striped put could land in the retiring generation — not a benign miss: the copy may already have migrated the key's older value, which then resurfaced as live; and aPutIfAbsentarriving in the mid-resize gap could install a stale snapshot value as live, defeating the if-absent semantics the read-fill paths rely on. Grow detection moves inside the stripe and the grow itself outside it (calling it stripe-held would self-deadlock against take-them-all); the triggering insert and racers until the swap evict at the pre-grow cap — a transient bounded by the grow window, noted in source. Generations carry an explicit freelru shard count that doubles across grows only while per-shard capacity does not shrink, so the migration copy can never overfill a shard and evict — left to freelru, a grown generation could pick more, smaller shards and silently drop clustered entries.Clear fence.
Clearis the second wholesale generation replacement; it now runs its counter reset, coherence re-init and swap under the same fence, so a racing put can neither land in the retired generation nor add its size after the counter was zeroed. Readers stay lock-free and are ordered instead:GetWithTxNumsnapshots coherence before loading the generation andClearre-inits coherence only after its swap, so an entry captured from the retiring generation is always judged by pre-init coherence that still carries the unwind — judged against the live state, the re-init (fresh epoch, lifted floor) revalidated dead-fork entries for in-flight readers. A live entry judged by a pre-Clear snapshot degrades to a safe miss via the stale-drop's re-check.Epoch stamp under the stripe.
putsamplescoh.Epoch()with the key's stripe held, next to the generation load the fence synchronizes. Read before the stripe, the stamp racedClear's coherence re-init: a put that lost the stripe toClearlanded a pre-Clear epoch on an entry in the post-Clear generation, and once a later unwind re-reached that epoch value the entry aliased the live epoch and served dead-fork state despite a txNum at or above the floor.Exact size accounting.
Deleteand the lazy stale-drop run under the key's put stripe, andcurrentSizeis subtracted solely via theOnEvictcallback (update/collision paths do remove-then-add instead of delta arithmetic). freelru picks eviction victims per shard — hash bits 16 and up, which the put stripes (bits 0–7) don't cover — so any subtraction computed outside the callback races a cross-stripe capacity eviction and double-subtracts. Capacity evictions are counted fromfreelru.Add's evicted return at the call sites — OnEvict also fires for intentional Removes, and routing those through the metric races a concurrent stats reset — which also stops stale drops counting in bothstaleEvictedandevictions. The byte counter is reserved before a remove-then-add, so a ModeNoOp admission never observes a transient dip and over-admits past the budget.Observability. One Debug line per grow with the caps, shard count, copied and copy-evicted counts, and the alloc/fenced duration split. Measured on the production accounts geometry (1 GB / 96 B avg), the final 4.19M-entry step is ~470 ms total of which ~150 ms is the writer-visible fenced copy — once per process lifetime.
growLRU contract. The CodeCache layers'
growLRUkeeps its unfenced swap deliberately; its doc comment now states why that is safe only for content-addressed layers (lost write = benign miss, raced removal = resurrect-once dropped on the next stale read, counters approximate) and points mutable-per-key values at the fencedGenericCache.Testing
TDD: each behavioral fix has coverage that failed on the pre-fix code.
TestGenericCache_PutNotLostAcrossGrowandTestGenericCache_PutIfAbsentDefersAcrossGrow(fail in the first rounds when the fence is removed)TestGenericCache_GrowMigrationLossless(clustered keys deterministically evicted by a resharding migration pre-fix)TestGenericCache_ClearRacingPut_EpochAlias(deterministic stripe-parking choreography; fails every run pre-fix)TestGenericCache_ClearRacingGet_DeadEntryStaysDead(reader gated on the fence reaching its stripe; served dead-fork state within tens of rounds pre-fix)TestDomainCache_ClearAtomicWithPut_NoSizeDriftTestDomainCache_DeleteAtomicWithPut_NoSizeDriftandTestDomainCache_StaleDropAtomicWithPut_NoSizeDriftTestGenericCache_CapacityEvictionAtomicWithPut_NoSizeDrift(cap-1 cache forces same-shard cross-stripe eviction; reproduces the drift in ~0.2 s pre-fix)TestGenericCache_StatsResetAtomicWithDelete_NoPhantomEvictions(intentional removals leak into the metric within milliseconds pre-fix)TestGenericCache_ModeNoOpAdmissionAtomicWithUpdate(over-admission past a full budget pre-fix)Test caches now close on cleanup, returning their envelope reservations — the process-global
cachebudget.Globalotherwise accumulates leaked reservations across the package run, which would starve the grow tests'Reservecalls.Verification:
go test ./execution/cache/...,-raceon the whole concurrency family, repeated cleanmake lint.Note: #22159's warmup-lifecycle split also appends tests to
cache_test.go; whichever lands second rebases trivially.