-
Notifications
You must be signed in to change notification settings - Fork 690
Pool collators in LS #1582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pool collators in LS #1582
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR optimizes the language server's completion functionality by pooling collators to avoid the performance overhead of repeatedly creating new collator instances. Profiling showed that collate.New
was consuming 15% of execution time in fourslash tests.
- Implements a per-language collator pool using
sync.Pool
andcollections.SyncMap
- Replaces direct
collate.New()
calls with pooled collator retrieval and return - Adds helper functions
getCollator
andputCollator
to manage the pool lifecycle
Comments suppressed due to low confidence (1)
} | ||
|
||
func putCollator(tag language.Tag, collator *collate.Collator) { | ||
pool, _ := collatorCache.Load(tag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The putCollator function assumes the pool exists in the cache, but there's no guarantee. If the pool was somehow evicted or never created, this will panic. Add a nil check or use LoadOrStore pattern similar to getCollator.
pool, _ := collatorCache.Load(tag) | |
pool, _ := collatorCache.LoadOrStore(tag, &sync.Pool{ | |
New: func() any { | |
return collate.New(tag) | |
}, | |
}) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested code is wrong because it'll just waste loads and loads of allocs. If I wanted to solve this, I'd just return a done
func from get
.
func getCollator(tag language.Tag) *collate.Collator { | ||
pool, ok := collatorCache.Load(tag) | ||
if !ok { | ||
pool, _ = collatorCache.LoadOrStore(tag, &sync.Pool{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a LoadOrStore
instead of just a Store
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise two callers would update totally different maps
Collators are not safe for concurrent use, so we create them fresh. But, pprof shows that
collate.New
is 15% of the time spent in the fourslash tests.Stick them in a
sync.Pool
per language.