lru: accounting per-key overhead - #23552
Conversation
8c8b970 to
1aa2db4
Compare
The budget derived its entry-count ceiling from the payload estimate alone, but freelru allocates nextPowerOfTwo(capacity*5/4) elements plus a bucket index per slot -- 232 B for the domain caches against 88 charged. A 1GB storage budget bought ~12.2M slots and ~2.9GB of RAM. Charge the table too, and clamp the ceiling to the largest capacity the table already covers, so a budget no longer pays for elements it cannot use.
1aa2db4 to
142e798
Compare
# Conflicts: # execution/cache/generic_cache.go # execution/cache/generic_cache_concurrency_test.go
domiwei
left a comment
There was a problem hiding this comment.
Reviewed the accounting change — the direction is right, and I verified the table in the description: all three before/after numbers recompute exactly from the new formula. The new test really pins the bug (pre-fix it reserves 262144×88 ≈ 23 MB against ~61 MB actually allocated, so it fails red). Envelope symmetry also checks out: birth / fundGrow / refundGrow / Clear / Close / Purge all go through the single perSlot field, so the reservation always equals curCap×perSlot.
Three things worth addressing (details in the inline comments):
- The envelope still under-reserves the slot array in reachable default configs —
capFitsTableis a near-identity in practice, and freelru's per-shardceilsplit can double every shard table (storage default at 256 shards: real table 928 MB vs 742 MB charged). - The new
maxCacheBytesclamp silently caps user-configured budgets (STATE_CACHE_*,StateCacheBudget) above 1 GiB — an existing 4 GB override now behaves exactly like 1 GB, with no log. - The code-size layer's entry ceiling silently drops from 1,000,000 to 216,216, because its budget is synthesized as
entries*codeSizeEntryBytesexpecting the old divide.
Smaller nits:
freelruSlotBytes' 112 is a hardcoded mirror of freelru's element size — exact forentry[[]byte]andcodeEntry, conservative forcodeSizeEntry(96 B). Anunsafe.Sizeofstatic assert would keep a future larger value type from under-charging silently.- "Stacked on #23546" in the description is stale — that PR was closed unmerged, and this diff is clean against main.
capFitsTable was an identity function: NextPowerOfTwo(c+c/4)/5*4 >= c for every c, so only integer-truncation crumbs were trimmed and nothing pinned the table ratio. freelru is then asked for NextPowerOfTwo(perShard*5/4) elements per shard, and the per-shard split is where the whole-cache computation broke: the real ratio landed anywhere in [5/4, 5/2) while the charge was a flat 2x. Storage at its 1 GiB default came out 195MB short at 256 shards and 292MB over at 128 or 512 -- the error swung with GOMAXPROCS, in both directions. fitTableSlots now rounds a shard's capacity down to 4/5 of a power of two, the only capacity freelru does not round up, and the slot count and shard count are derived together so the charge matches what is allocated. growLRU gets its own computation: freelru.NewSharded rounds the whole capacity rather than each shard. The 1 GiB byte clamp is gone -- it silently pinned STATE_CACHE_STORAGE=4GB to the same slot count as 1GB while capacityB kept the unclamped value that PrintStats and ModeNoOp report against. The old 1<<24 slot ceiling is back in its place. The size layer is built from its entry count again rather than a synthesized byte budget, which had dropped its ceiling from 1,000,000 to 216,216. The envelope test now runs the production maxCap across 128/256/512 shards and compares only the overhead share of the reserve: the payload estimate pays for values the table does not hold, so including it let an undercharged table pass.
1 << 24 does not read as a slot count.
|
@AskAlexSharov It says "Stacked on #23546", but 23546 is closed? |
stale description |
|
The two smaller nits are closed; head is
|
There was a problem hiding this comment.
Pull request overview
Updates LRU memory accounting to include freelru metadata overhead.
Changes:
- Adds per-slot overhead calculations and capacity fitting.
- Preserves entry-count semantics for the code-size cache.
- Adds allocation-envelope coverage testing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
execution/cache/grow_lru.go |
Adds overhead-aware grow-LRU sizing. |
execution/cache/generic_cache.go |
Accounts for freelru slot metadata. |
execution/cache/generic_cache_concurrency_test.go |
Tests envelope allocation coverage. |
execution/cache/code_cache.go |
Uses an entry-count constructor for code sizes. |
Suppressed comments (2)
execution/cache/grow_lru.go:80
- The byte-derived slot count is narrowed to
uint32before it is capped. Once the quotient exceedsmath.MaxUint32, it wraps and may create a much smaller cache rather than selectingmaxCacheSlots. Apply the cap while the value is stilluint64.
perSlot := int64(avgBytes) + freelruSlotBytes
maxCap := max(fitTableSlots(min(uint32(uint64(maxBytes)/uint64(perSlot)), maxCacheSlots)), 1)
return newGrowLRUWith(maxCap, perSlot, onEvict)
execution/cache/generic_cache.go:94
- The quotient is converted to
uint32before applyingmaxCacheSlots. For a valid large byte budget where the quotient exceedsmath.MaxUint32, this wraps and can produce a tiny cache instead of saturating at the configured ceiling. Clamp inuint64first, then convert.
perSlot := uint64(payloadBytes) + freelruSlotBytes
approx := min(uint32(uint64(capacityBytes)/perSlot), maxCacheSlots)
shards = initialShardCount(approx, shardCeil())
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
yperbasis
left a comment
There was a problem hiding this comment.
Requesting changes for two accounting regressions. Intermediate grow generations still under-reserve the freelru table, so the shared memory envelope can be exceeded. Also, the byte-usage metric no longer shows when the cache reaches its new overhead-aware entry ceiling. I reproduced the first issue on this head; the existing execution/cache tests pass because the new envelope test measures only the fitted final generation.
…d ceiling freelru rounds capacity+25% up to a power of two and sizes both arrays at the result, so a power-of-two generation gets a 2x table while the envelope charged the ceiling's 5/4 — 153MB reserved against 243MB allocated at 1M slots. Charge each step from tableSlots of the two capacities, and report payload usage against the payload the ceiling buys rather than the whole budget.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
execution/cache/generic_cache.go:251
avgBytesstill includes the 24-byte per-entry overhead (putStripedand the account/storage averages all add it), whilefreelruElemBytesnow charges for the complete freelru element, including the inlineentry[T]. Treating the unchanged estimate as external payload therefore double-counts that overhead, reducing the account/storage ceilings and consuming the shared envelope early. Split the estimates into externally allocated key/value bytes versus table-resident bytes, or adjust the callers before using them aspayloadBytes.
payloadBytes: int64(avgBytes),
…m for, and clamp the slot quotient in uint64
lru accounting logic has bug: didn't account overhead metada per-key. but in some cases our keys/vals are small and it's noticable
At 256 shards. The ceiling now moves with the shard count, because each shard's
table is fitted to the 5/4 boundary rather than charged a flat per-slot constant —
that flat charge was wrong by up to ±292 MB on the storage cache purely as a
function of GOMAXPROCS.
Grow steps are charged from the real table on both sides of the step, not from the
fitted ceiling: at a power-of-two generation freelru allocates 2x the capacity, so
an intermediate generation costs about double what it used to be charged. That is
what it was really allocating, and it means a cache can now be refused a step it
used to get.