perf(loadbalance): avoid redundant Store in consistBalancer.GetPicker#1948
Open
Wizard-Guido wants to merge 1 commit intocloudwego:mainfrom
Open
perf(loadbalance): avoid redundant Store in consistBalancer.GetPicker#1948Wizard-Guido wants to merge 1 commit intocloudwego:mainfrom
Wizard-Guido wants to merge 1 commit intocloudwego:mainfrom
Conversation
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.
What type of PR is this?
Check the PR title.
(Optional) Translate the PR title into Chinese.
(Optional) More detailed description for this PR(en: English/zh: Chinese).
en:
When multiple goroutines concurrently miss the cache for the same
CacheKey,singleflightcorrectly ensuresnewConsistInforuns only once — but every waiting goroutine then independently callscachedConsistInfo.Store(key, cii)with the same value. For N waiters this produces N-1 redundantsync.Mapwrites.Moving the
Storeinside thesingleflight.Docallback guarantees it runs exactly once, regardless of how many goroutines were waiting. This also alignsconsistBalancer.GetPickerwith the pre-existing pattern inweightedBalancer.GetPicker(same package), where the cache write already lives insidesfg.Do.As a side effect, the race window where a stale singleflight result can overwrite a fresher entry written by a concurrent
Rebalanceis narrowed (the store now completes before any waiter observes the result, instead of N-1 stores racing afterward). The broaderDelete/Rebalancerace noted in the existingFIXMEon line 369 is not addressed by this PR.This fix aligns with the existing pattern in the same package:
weightedBalancer.GetPickerin this same package already places the cache write inside thesingleflight.Docallback — this is the established best practice withinpkg/loadbalance:Also fixes a typo in a test function name:
TestConsistPicker_Reblance→TestConsistPicker_Rebalance.zh(optional):
consistBalancer.GetPicker在缓存未命中时使用singleflight构建一致性哈希环。当多个 goroutine 并发请求同一个CacheKey时,singleflight正确保证了newConsistInfo只执行一次 —— 但所有等待的 goroutine 拿到结果后各自独立调用了一次cachedConsistInfo.Store(key, cii)。N 个等待者就会产生 N-1 次完全冗余的sync.Map写入,每次写入内部都会走锁竞争、原子操作,甚至可能触发 dirty map 晋升。此外还存在一个小的竞态窗口:因为
Store发生在sfg.Do返回之后,如果这期间有并发的Rebalance写入了更新的数据,这些迟到的Store会把新数据覆盖回旧的。修复:对齐本包内已有的最佳实践
同包中的
weightedBalancer.GetPicker早已把缓存写入放在singleflight.Do的回调内部,这是pkg/loadbalance里已经建立的惯例:本 PR 让 consistBalancer.GetPicker 遵循同样的模式:
顺便修复了一个测试函数名拼写错误:
TestConsistPicker_Reblance→TestConsistPicker_Rebalance。不在本 PR 范围内:consist.go:369 处 FIXME 标注的 Delete / Rebalance 竞态问题本 PR 不处理。
(Optional) Which issue(s) this PR fixes:
(optional) The PR that updates user documentation: