perf(xds): replace once-cache's single mutex with sync.Map - #18234
Open
sujanchalla0510 wants to merge 1 commit into
Open
sujanchalla0510 wants to merge 1 commit into
sujanchalla0510 wants to merge 1 commit into
Conversation
omap (backing Cache.GetOrRetrieve's miss path in pkg/xds/cache/once) took one sync.Mutex around map lookup, *once allocation, and insertion. That mutex is shared across every key a Cache instance manages - every mesh for the mesh-context cache, every CLA cache entry, etc - so it serializes concurrent misses for entirely unrelated keys, not just repeated misses of the same key. Under load (hundreds of concurrent DataplaneWatchdog goroutines, one per mesh, all hitting this path together) that turned map access into a significant bottleneck: goroutine profiles showed hundreds parked on the lock, with DataplaneWatchdog.syncDataplane's cumulative time dominated by serialization rather than actual work. sync.Map is built for exactly this shape - a small, mostly-stable set of keys, read far more often than written - and lets lookups for different keys proceed independently instead of contending on one lock. LoadOrStore replaces the check-then-insert critical section with a single atomic operation, preserving the "insert-if-absent, otherwise return the existing entry" semantics the callers rely on. Added BenchmarkOmapGetConcurrentKeys, simulating many goroutines hitting a small set of distinct keys concurrently. Measured on 8 CPUs: old (single mutex): 94.48 ns/op new (sync.Map): 22.78 ns/op (~4.1x) Existing once package tests (including the 100-goroutine "should cache concurrent Get() requests" case, which exercises the exactly-once retrieval guarantee this change must preserve) pass unmodified under -race. Fixes kumahq#16188 Signed-off-by: sujan reddy <sujanchalla0510@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.
Motivation
Fixes #16188.
omap(backingCache.GetOrRetrieve's miss path inpkg/xds/cache/once) took onesync.Mutexaround map lookup,*onceallocation, and insertion. That mutex is shared across every key a
Cacheinstance manages — every mesh for the mesh-context cache, everyCLA cache entry, etc — so it serializes concurrent misses for entirely
unrelated keys, not just repeated misses of the same key.
Under load (hundreds of concurrent
DataplaneWatchdoggoroutines, oneper mesh, all hitting this path together) that turned map access into a
significant bottleneck, per the issue's profiling evidence: goroutine
profiles showed hundreds parked on the lock, with
DataplaneWatchdog.syncDataplane's cumulative time dominated byserialization rather than actual work.
Implementation information
sync.Mapis built for exactly this shape — a small, mostly-stable setof keys, read far more often than written — and lets lookups for
different keys proceed independently instead of contending on one lock.
LoadOrStorereplaces the check-then-insert critical section with asingle atomic operation, preserving the "insert-if-absent, otherwise
return the existing entry" semantics
Cache.GetOrRetrieverelies on. Nocaller-visible API change —
omap.Get/Deletekeep the samesignatures.
Added
BenchmarkOmapGetConcurrentKeys, simulating many goroutineshitting a small set of distinct keys concurrently. Measured on 8 CPUs:
Existing
oncepackage tests — including the 100-goroutine "shouldcache concurrent
Get()requests" case, which exercises theexactly-once retrieval guarantee this change must preserve — pass
unmodified under
-race.Out of scope: the issue also flags a structurally similar single-mutex
pattern in
cachedManager.mapMutex(
pkg/core/resources/manager/cache.go:143-152). That's a separate datastructure in a different subsystem; leaving it for a follow-up to keep
this PR focused on one change.
Supporting documentation
Fix #16188