execution/cache: track GenericCache entry count in an atomic, not freelru's all-shard Len - #23522
execution/cache: track GenericCache entry count in an atomic, not freelru's all-shard Len#23522AskAlexSharov wants to merge 8 commits into
Conversation
…elru's all-shard Len Every insert into a cache below its growth ceiling ran the grow check through freelru's ShardedLRU.Len, which RLocks all shards in turn (up to GOMAXPROCS*16). That serialised writers on shards they never touch, and ShardedLRU.Get takes the shard's write lock, so readers convoyed behind it too. Pair each generation of the LRU with an atomic entry count maintained by the insert path and the OnEvict callback, and read that instead. BenchmarkGenericCacheParallelPutGrow, M4 Max, -cpu 10: 466.3n -> 163.7n (-64.9%, p=0.002 n=6). The gap widens with core count.
Stacked on #23522 — review that first; this PR's own diff is the second commit. `growLRU.Add` ran its grow check through `freelru.ShardedLRU.Len`, which RLocks every shard in turn, on **every add** while the LRU was below its ceiling; `maybeGrow` scanned again inside `resizeMu`. `CodeCache` drives three of these layers (`hashToCode`, `codeHashToCode`, `codeSizeByCodeHash`) straight off the EVM code path. Same fix as #23522: reuse `lruGen`, generalised over the value type, so the count comes from an atomic maintained by `add` and the `OnEvict` callback. `Len()` becomes O(1) too. `putContentLocked` already removes an existing entry before re-adding, so every `Add` reaching freelru is for an absent key — the invariant the counter needs. `BenchmarkGrowLRUParallelAddGrow` (added), M4 Max, `-cpu 10`: ``` │ growLRU_Len │ growLRU_counter │ │ sec/op │ sec/op vs base │ GrowLRUParallelAddGrow 756.90n ± 8% 96.58n ± 13% -87.24% (p=0.002 n=6) ``` `CodeCache.Put` end-to-end does **not** move (500.2n → 497.1n, p=0.394): `addrToHash` / `addrToCodeHash` are `hashicorp/golang-lru` caches behind a single global mutex, and that dominates the put path. This PR removes real CPU time from the content layers; unlocking it end-to-end needs that addr LRU sharded, which is separate work. Two tests pin counter == the LRU's real length: `growLRU` directly across add / remove / eviction / grow / `Purge`, and all three `CodeCache` layers under concurrent puts. Both verified to fail against a mutated counter. No TDD cycle: performance refactor with no intended behaviour change. https://claude.ai/code/session_01WFkAYPPhqPe1NXg41Nph78
Stacked on #23522 — review that first; this PR's own diff is the second commit. `growLRU.Add` ran its grow check through `freelru.ShardedLRU.Len`, which RLocks every shard in turn, on **every add** while the LRU was below its ceiling; `maybeGrow` scanned again inside `resizeMu`. `CodeCache` drives three of these layers (`hashToCode`, `codeHashToCode`, `codeSizeByCodeHash`) straight off the EVM code path. Same fix as #23522: reuse `lruGen`, generalised over the value type, so the count comes from an atomic maintained by `add` and the `OnEvict` callback. `Len()` becomes O(1) too. `putContentLocked` already removes an existing entry before re-adding, so every `Add` reaching freelru is for an absent key — the invariant the counter needs. `BenchmarkGrowLRUParallelAddGrow` (added), M4 Max, `-cpu 10`: ``` │ growLRU_Len │ growLRU_counter │ │ sec/op │ sec/op vs base │ GrowLRUParallelAddGrow 756.90n ± 8% 96.58n ± 13% -87.24% (p=0.002 n=6) ``` `CodeCache.Put` end-to-end does **not** move (500.2n → 497.1n, p=0.394): `addrToHash` / `addrToCodeHash` are `hashicorp/golang-lru` caches behind a single global mutex, and that dominates the put path. This PR removes real CPU time from the content layers; unlocking it end-to-end needs that addr LRU sharded, which is separate work. Two tests pin counter == the LRU's real length: `growLRU` directly across add / remove / eviction / grow / `Purge`, and all three `CodeCache` layers under concurrent puts. Both verified to fail against a mutated counter. No TDD cycle: performance refactor with no intended behaviour change. https://claude.ai/code/session_01WFkAYPPhqPe1NXg41Nph78
d6690ac to
76c7384
Compare
|
duplicate of #23248 ? |
There was a problem hiding this comment.
Pull request overview
Replaces expensive all-shard length checks with atomic entry counters to reduce cache growth contention.
Changes:
- Adds atomic per-generation entry counting.
- Updates growth checks and cache statistics.
- Adds concurrency tests and benchmarks.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
execution/cache/grow_lru.go |
Uses counted LRU generations. |
execution/cache/generic_cache.go |
Implements atomic entry tracking. |
execution/cache/generic_cache_concurrency_test.go |
Tests concurrent counting and benchmarks inserts. |
execution/cache/code_cache_concurrency_test.go |
Tests and benchmarks growLRU. |
execution/cache/cache_test.go |
Tests mutation-path counter accuracy. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
growLRU's grow is not fenced against writers, so a same-key Remove then Add (putContentLocked's stale-entry refresh) can straddle a generation swap: the Remove lands on the retiring generation while the Add lands on the new one, which a concurrent grow already copied the key into. freelru.Add replaces an existing key in place without firing OnEvict, so the unconditional increment in growLRU's add path double-counted it. Add addIfAbsent, which checks membership before counting, and use it on growLRU's hot path.
|
Different approach from #23248, not a duplicate — this PR (and stacked #23523) uses plain per-generation atomic counters, no |
ok. feel free to go ahead with your PR, im looking into other stuff atm and might not have time to come back to that PR for another week |
…he membership probe
…aces Add assumed its caller had proven the key absent. It had not: an unfenced grow copies a key into the next generation, a read-path Remove drops it from the still-current old one, and the publish can land between the caller's miss and Add's own load -- so freelru replaced the copy in place, firing no OnEvict, while the count still rose. The generation then carried an entry that was not there, and the grow gate tripped early. Add and Replace only ever differed in the grow check, so they are one Put that removes before it stores. The removal costs a lookup and buys an exact count and a byte counter that no longer keeps a displaced copy.
| // a stale entry before re-adding it; concurrent puts of distinct code must still | ||
| // leave each layer's counter equal to its LRU's real length. | ||
| func TestCodeCache_GrowLRULenCounterUnderConcurrency(t *testing.T) { | ||
| cc := closeOnCleanup(t, NewCodeCache(8*datasize.MB, 8*datasize.MB)) |
There was a problem hiding this comment.
The doc above claims this "drives three growLRU layers through putContentLocked, which removes a stale entry before re-adding it". None of the three claims hold.
Two layers are never written. The workers call cc.Put(addr, code, uint64(i)) at line 294. CodeCache.Put → putCode (code_cache.go:332) → putCodeLocked (code_cache.go:344) writes exactly addrToHash and putContentLocked(c.hashToCode, ...) (code_cache.go:359). codeHashToCode is written solely by putWithCodeHash (code_cache.go:490/515) and codeSizeByCodeHash solely by PutCodeSizeByCodeHash / putCodeSizeByCodeHashLocked (code_cache.go:549/565), neither of which this test calls. The assertions at lines 301 and 302 both evaluate 0 == 0, and would still pass with either layer's n counter deleted outright, or with putCodeSizeByCodeHashLocked's keyCost=1/zeroCost accounting inverted.
The one layer that is written can never grow. NewCodeCache(8*datasize.MB, ...) builds hashToCode as newGrowLRU[codeEntry](8MB, avgCodeEntryBytes=12288, ...), so maxCap = 8388608/12288 = 682 and startCap = min(1024, 682) = 682. growLRU.Put's gate at grow_lru.go:103 requires curCap < g.maxCap — 682 < 682 is false from construction — so maybeGrow never runs and no generation swap ever happens. The unfenced grow window this change's whole per-generation-exactness argument is about is never entered.
No stale refresh occurs. Every worker writes a distinct w*perWorker+i into both addr and code, so every codeID = maphash(code) is unique across the run and putContentLocked's lru.Get(h) always misses; the test never calls Unwind, so no entry is ever stale.
Failure case: revert growLRU.Put to a bare gen.add(key, value) — the over-count 489fb3163a exists to fix — and this test still passes. What it actually verifies is counter-vs-length agreement on one layer under concurrent fresh inserts with capacity eviction at 682 slots, which the counter already handled before the change. A counter regression in codeSizeByCodeHash (maxCap 1,000,000 against startCap 1024, so an under-report there wedges growth permanently) ships green.
There was a problem hiding this comment.
All three confirmed, and the test is rewritten in 9424fba.
- Two layers never written. It now calls
PutWithCodeHash, which writeshashToCodeviaputCodeLocked,codeSizeByCodeHashviaputCodeSizeByCodeHashLocked, andcodeHashToCodedirectly. All three assertions are live, and each layer also assertscurCap > startCap, so a layer that stops being written fails on the growth check rather than comparing 0 to 0. - Never grows. Budget raised from 8MB to 64MB, which puts the code layers' ceiling well above
genericCacheStartCapacityinstead of at 682 == startCap. Growth is now asserted, not assumed. - No stale refresh. The key space is bounded at 4096 so puts land on resident keys, and an unwinder goroutine calls
Unwind(0)throughout. A leadingUnwind(0)sets the floor to 0, so each later epoch bump turns the whole resident set stale andputContentLockedtakes the displacing path instead of returning early.
Your failure case now fails: reverting growLRU.Put to a bare gen.add(key, value) gives hashToCode counter drifted.
| h := key(uint64(w)*20000 + i) | ||
| stripe := &stripes[uint8(h)] | ||
| stripe.Lock() | ||
| if _, ok := g.Get(h); ok { |
There was a problem hiding this comment.
Both arms are byte-identical and the Get result is discarded, so the whole conditional is equivalent to a bare g.Put(h, i). Residue of 2df32ebf5a, where the hit arm called Replace and the miss arm called Add; 489fb3163a folded those into one Put and the branch became meaningless without being removed.
The cost is to the reader, not the runtime — the final gen.lru.Len() == gen.len() assertion still holds. But the doc comment at lines 320-324 sells this as interleaving "a striped writer's stale-entry refresh" against unstriped removers, and this if/else is the only thing in the body that looks like it distinguishes refresh-on-hit from cold insert. It does not. Someone auditing whether the refresh path is covered — in the one test standing in for an under-count race with no deterministic reproducer — reads a discrimination that isn't there, and the next person to add a Put variant will edit one arm believing the other still covers the old path.
There was a problem hiding this comment.
Fixed in 9424fba -- the branch is gone, the body is a bare g.Put(h, i) under the stripe. You are right that it was 2df32ebf5a residue and that it read as a discrimination the code no longer makes. The refresh path is still covered: the key space is 4096 against 8x20000 puts, so most puts land on a resident key, which is exactly the displacing path.
|
|
||
| // growLRU's generation swap is not fenced against writers (see its doc | ||
| // comment): a grow can copy a key into the new generation while a same-key | ||
| // refresh (putContentLocked's stale-entry path) is in flight. Replace resolves |
There was a problem hiding this comment.
This names Replace as a method performing an action — "Replace resolves g.cur once so both halves land on the same generation". 489fb3163a folded Add and Replace into a single Put (grow_lru.go:101), and the body below calls g.Put(h, 1) / g.Put(h, 2) at lines 247 and 262. rg Replace execution/cache/ finds no such method anywhere in the package — this comment is the only mention outside an unrelated test name in code_cache_codehash_test.go.
A reader auditing the count-exactness argument follows the comment to a symbol that is not there, and cannot tell whether it describes a removed design or the current one. git log -S places the sentence in 2df32ebf5a, invalidated by the very next commit in the same PR.
Secondary naming drift, not itself wrong: BenchmarkGrowLRUParallelAddGrow (line 307) is named after Add while its body calls g.Put.
There was a problem hiding this comment.
Fixed in 9424fba. The comment now names Put and describes what it does ("resolves g.cur once so its removal and its store land on the same generation"). Renamed the test to TestGrowLRU_GrowRacePutDoesNotDoubleCount and the benchmark to BenchmarkGrowLRUParallelPutGrow, so no Replace naming survives.
It wrote only hashToCode, at a budget where the ceiling equals the start capacity so no grow ever ran, with distinct keys so no stale refresh ran. The other two layers' assertions compared 0 to 0. Now all three layers are written through PutWithCodeHash over a repeating key space, an unwinder makes the repeats stale, and the code budget leaves room to grow. Also drop a dead if/else whose arms were identical, and retire the Replace naming folded away in the previous commit.
…tion_37 # Conflicts: # execution/cache/generic_cache.go # execution/cache/generic_cache_concurrency_test.go
|
Merged What is left is the The title says Lint is clean. The one |
Resolve
GenericCachegrow mutex contention caused by.Len()callBenchmarkGenericCacheParallelPutGrow(added), M4 Max,-cpu 10: