-
Notifications
You must be signed in to change notification settings - Fork 659
Port TS PR 61505 - Cache mapper instantiations #1358
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
base: main
Are you sure you want to change the base?
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 ports TypeScript’s PR 61505 to introduce caching for type mapper instantiations, aiming to reduce repeated instantiation overhead and improve performance.
- Added a stack of active mappers with per-mapper caches backed by
sync.Pool
. - Updated
instantiateTypeWithAlias
to check and store in those caches. - Cleared mapper caches after each inference in
getInferredType
.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
internal/checker/inference.go | Inserted clearActiveMapperCaches() after each inference loop |
internal/checker/checker.go | Added activeMappers , activeTypeMappersCaches , cache logic in instantiateTypeWithAlias , and push/pop/clear helper methods |
Comments suppressed due to low confidence (2)
internal/checker/inference.go:1332
- [nitpick]
clearActiveMapperCaches()
is invoked inside the loop for each inference, leading to repeated map clears. Consider moving this call outside the loop or only once per context to avoid redundant operations.
c.clearActiveMapperCaches()
activeMappers []*TypeMapper | ||
activeTypeMappersCaches []map[string]*Type |
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 other "global" caches don't use two separate lists in the Go port, instead scoping them together, but I'm keeping them separate since I'm trying to reuse the maps.
|
||
func (c *Checker) clearActiveMapperCaches() { | ||
for _, cache := range c.activeTypeMappersCaches { | ||
clear(cache) |
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 does the old code not just set them all to undefined
instead of clearing all of them and keeping them in place?
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.
Since that would mean call stacks which have pushed onto them will then have an undefined map. Well, they wouldn't, but they'd have a reference to a cache that would never be used again.
internal/checker/checker.go
Outdated
if cap(c.activeTypeMappersCaches) > lastIndex { | ||
c.activeTypeMappersCaches = c.activeTypeMappersCaches[:lastIndex+1] | ||
if c.activeTypeMappersCaches[lastIndex] == nil { | ||
c.activeTypeMappersCaches[lastIndex] = make(map[string]*Type, 1) |
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.
Trying to reuse an empty map is already subtle - I see the comment in popActiveMapper
, but leave a comment here as well since this code is also involved.
This is an early port of microsoft/TypeScript#61505, because I like to go fast
Calculating the key increases allocs by a bit (really wish we had the yet-to-exist Go custom map implementation), but for https://github.com/AnswerOverflow/AnswerOverflow/tree/1ab687793a598789a0c86b4ca5f17ee6f3a256aa/apps/dashboard:
TypeScript 5.8 vs 5.9:
tsgo main vs this PR;
(Instantiations look higher than 5.8 or 5.9 because tsgo's metrics sum up instantiations between all concurrent checkers)