Skip to content

lru: grow 1 shard at a time - #23553

Merged
awskii merged 10 commits into
mainfrom
alex/lru_grow_one_shard_37
Aug 26, 2026
Merged

lru: grow 1 shard at a time#23553
awskii merged 10 commits into
mainfrom
alex/lru_grow_one_shard_37

Conversation

@AskAlexSharov

@AskAlexSharov AskAlexSharov commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

lru-grow worst case on main: copied=1048576 fenced=194.8ms. And a grow takes every put stripe — stop-the-world for concurrent RPC, parallel-exec and parallel-commitment alike.

Solution: grow one shard at a time. A writer waits on that shard's mutex only; the other 255/256 of traffic is untouched. The generation swap, the all-stripe fence and curCap go away with it.

Numbers vs main (n0, AMD EPYC 4344P, idle, benchstat)

Steady-state throughput — 1024 goroutines, hot/cold key mix, read-through fill, ~79% hit rate:

                             │   main    │            this PR             │
GenericCacheParallelMixed-16   37.18n ±2%   37.66n ±6%   +1.28% (p=0.041)
                        hit%    78.95 ±1%    78.80 ±1%        ~ (p=0.677)

Worst single put while filling a cache from cold to 1M entries:

                        │   main    │             this PR              │
worst-put               │ 92.247k µs │  1.191k µs   -98.71% (p=0.002)  │
cold fill               │  1216.5ms  │   328.3ms    -73.01% (p=0.002)  │
B/op                    │  322.9Mi   │   440.9Mi    +36.56% (p=0.002)  │

So the 92 ms all-writers stall becomes a 1.2 ms one-shard stall, cold fill is 3.7x cheaper, and steady-state throughput is unchanged. The extra bytes are the per-shard tables: shard count follows the ceiling, so more shards are allocated up front.

Falls out of it

  • Shard count is fixed for the life of the cache, so a grow can no longer re-shard. The hazard TestGenericCache_GrowMigrationLossless was written to catch is structurally impossible; that test is retargeted at one shard's migration.
  • Entry count is an atomic on the shard array rather than freelru's all-shard Len() walk.
  • Shards select on bits 16+ of the key, matching freelru's own choice — the low bits index buckets inside a shard.
  • Each shard's table is sized nextPowerOfTwo(capacity*5/4), as freelru's sharded constructor does. Without it the bucket index falls back to fastModulo, which keys off the high hash bits — the same bits that pick the shard, so they are constant inside one, and a shard collapses onto a few chains.
  • Growth is funded from the shared envelope before it happens (Reserve, refusable), and a step that loses the race hands its reservation back.

The jump-grow copied the whole cache into a new generation behind every put
stripe, so one grow blocked every writer for as long as the copy took -- 195ms
at 1M entries, and the ladder ends with a 4.19M-entry step. Own the shards
instead of handing them to freelru.ShardedLRU: a full shard is rebuilt one
step larger under its own lock, so a grow blocks one shard's writers for
capacity/shards entries. Shard count is fixed for the life of the cache, which
also removes the re-shard hazard the migration had to defend against.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Reworks GenericCache growth to resize one LRU shard at a time, reducing global write stalls.

Changes:

  • Adds a shard-owned, independently growing LRU.
  • Replaces whole-cache generation growth with per-shard growth.
  • Retargets concurrency tests and adds a mixed-workload benchmark.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
execution/cache/sharded_lru.go Implements independently growing shards.
execution/cache/generic_cache.go Integrates shard growth and budget accounting.
execution/cache/generic_cache_concurrency_test.go Updates growth tests and adds a benchmark.
Suppressed comments (3)

execution/cache/sharded_lru.go:135

  • A capacity eviction invokes the callback and decrements n, but the shard length is unchanged, so delta is zero and the newly inserted entry is never counted. After reaching its ceiling, every eviction makes Len() drift downward (eventually negative). Compensate for the callback when Add reports an eviction.
	before := s.shards[i].Len()
	evicted = s.shards[i].Add(h, v)
	// freelru replaces a present key in place, returning false and firing no
	// OnEvict, so the live count follows the shard's own length.
	delta := s.shards[i].Len() - before
	s.mus[i].Unlock()
	if delta != 0 {
		s.n.Add(int64(delta))

execution/cache/generic_cache.go:427

  • resizeMu is held before the following loop takes every put stripe, but a growing putStriped call already holds one of those stripes and takes resizeMu in fundGrow. If Clear wins resizeMu, the two goroutines wait on each other indefinitely. Fence the put stripes first, then take resizeMu only for the budget update.
	next := c.newShards(c.startCap, c.maxCap, c.shardCount) // allocate before excluding writers
	for i := range c.putStripes {
		c.putStripes[i].Lock()

execution/cache/sharded_lru.go:153

  • This doc comment names growLocked, but the function is migrateLocked.
// growLocked rebuilds shard i one step larger. Only that shard's readers and

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread execution/cache/sharded_lru.go Outdated
Comment thread execution/cache/generic_cache.go Outdated
@AskAlexSharov
AskAlexSharov requested a balanced review from Copilot August 25, 2026 13:57
@AskAlexSharov
AskAlexSharov marked this pull request as ready for review August 25, 2026 13:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

Suppressed comments (1)

execution/cache/generic_cache_concurrency_test.go:499

  • The new implementation does not allocate slabs; the longest put performs a one-shard migration. Describing it as one slab allocation misstates what this benchmark measures.
// The longest a single put is held up while filling a cache from cold: on the
// jump-grow lineage that is a migration copy, with slab-allocated elements it
// is one slab allocation.

Comment thread execution/cache/sharded_lru.go
Comment thread execution/cache/generic_cache.go Outdated
Comment thread execution/cache/generic_cache_concurrency_test.go Outdated
Comment thread execution/cache/sharded_lru.go Outdated
Comment thread execution/cache/generic_cache_concurrency_test.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread execution/cache/generic_cache.go Outdated
Comment thread execution/cache/sharded_lru.go Outdated
… shards

Close settled reservedBytes under resizeMu, which no grow holds -- a grow runs
under a put stripe. A grow racing Close either released its step a second time,
under-counting the shared budget for the process lifetime, or funded one nothing
gave back. Settle behind the same stripe fence Clear uses, and refuse to fund a
step once closed.

The entry count was incremented after the shard unlock but decremented inside it
from the evict callback, so Len() could go negative: the ModeNoOp guard admits
past the cap and any make() sized from it panics. Derive the count from the
shard's own length under the lock and drop the callback's decrement.

Also: overwrite through a shard-locked Replace instead of remove-then-add, which
could span a rebuild; delete the unused Keys; stop allocating a shard array the
constructor discards; hoist the duplicated GOMAXPROCS shard ceiling; assert the
grow copy cannot evict; and give the grow tests a fill sized from the real shard
geometry plus an assertion that a shard grew.
Comment thread execution/cache/sharded_lru.go Outdated
@AskAlexSharov
AskAlexSharov requested a review from awskii August 26, 2026 07:59
@awskii
awskii added this pull request to the merge queue Aug 26, 2026
Merged via the queue into main with commit 0e5d9bf Aug 26, 2026
156 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants