Pre-allocate parsedPatterns and cache some calculations
#371
Conversation
internal/compiler/module/cache.go
Outdated
| @@ -18,6 +19,7 @@ type caches struct { | |||
| typeReferenceDirectiveCache *resolutionCache[*ResolvedTypeReferenceDirective] | |||
| packageJsonInfoCache *packagejson.InfoCache | |||
| resolvedTypeReferenceDirectiveLookupLocations map[*ResolvedTypeReferenceDirective]*LookupLocations | |||
| parsedPatternsCache map[*collections.OrderedMap[string, []string]]parsedPatterns | |||
There was a problem hiding this comment.
I can't say I super like holding onto these maps like this, but I guess we'll probably throw these objects away periodically.
There was a problem hiding this comment.
Wait, we don't even need this to be a map; isn't tryParsePatternsCached only ever called on r.compilerOptions.Paths, so we could just sync.Once it.
There was a problem hiding this comment.
I haven’t exactly worked out how I want to do this, but eventually there will be a module resolution cache that persists across program updates and/or is shared between different programs.
There was a problem hiding this comment.
PR Overview
This PR improves module resolution performance by caching the parsed patterns used for path matching. Key changes include:
- Replacing the direct call to tryParsePatterns with a cached version (tryParsePatternsCached) in the Resolver.
- Updating the parsedPatterns structure and its initialization to use core.Set instead of collections.OrderedSet.
- Adding a new cache field for parsed patterns in the Resolver's caches and minor documentation updates in the core set implementation.
Reviewed Changes
| File | Description |
|---|---|
| internal/compiler/module/resolver.go | Introduces a caching mechanism for parsedPatterns and refactors the pattern parsing logic. |
| internal/core/set.go | Adds a comment to NewSetWithSizeHint for clarity. |
| internal/compiler/module/cache.go | Adds the parsedPatternsCache field to the caches struct. |
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more
internal/compiler/module/resolver.go
Outdated
| if r.parsedPatternsCache == nil { | ||
| r.parsedPatternsCache = make(map[*collections.OrderedMap[string, []string]]parsedPatterns) | ||
| } | ||
| r.caches.parsedPatternsCache[paths] = pathPatterns |
There was a problem hiding this comment.
There is an inconsistency in caching: the code first checks r.parsedPatternsCache but then assigns the new value to r.caches.parsedPatternsCache. Consider using a single cache field consistently to avoid unexpected behavior.
| r.caches.parsedPatternsCache[paths] = pathPatterns | |
| r.parsedPatternsCache[paths] = pathPatterns |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
DanielRosenwasser
force-pushed
the
parsedPatternsOptimization
branch
from
6b825a0 to
1bf7ca3
Compare
| @@ -4,6 +4,13 @@ type Set[T comparable] struct { | |||
| M map[T]struct{} | |||
| } | |||
|
|
|||
| // NewSetWithSizeHint creates a new Set with a hint for the number of elements it will contain. | |||
| func NewSetWithSizeHint[T comparable](hint int) *Set[T] { | |||
There was a problem hiding this comment.
Probably just do Set[T] here, since it's always dereferenced at its use.
There was a problem hiding this comment.
But everyone else is doing *core.Set (everyone else is one function)
There was a problem hiding this comment.
With the one change, seems okay, but will defer to @andrewbranch.
For
pathsofCompilerOptions, we currently split the entries into a set of strings to do a quick lookup, and a (shorter) list of patterns to walk on. Later on, exact matches or any match with the longest prefix (breaking any ties by ordering inpaths, see #369 and #370 for fixes around that behavior).A lot of this work was done in microsoft/TypeScript#59048, but that PR also cached the creation of these
parsedPatterns. tsgo currently does not cache these at all, and on a fairly large internal codebase, this results in a significant amount of user time (13.7 seconds below) spent intryParsePatterns:This PR reintroduces the cache for
parsedPatternsforpathsinCompilerOptions, and places it on theResolver's caches.This PR also does an extra walk of paths to provide a size hint. When uncached, this actually added time at the cost of more allocation. It may be worth experimenting with removing the extra walk.