Conversation
| if kind == stKindMembers { | ||
| return nil | ||
| } | ||
| // Globals and exports tables are large and revisited often; use cache. | ||
| if kind == stKindGlobals || kind == stKindExports { |
There was a problem hiding this comment.
The initial fix copilot made wanted to cache everything; I modified it to only cache for globals/exports, which I think are much more bounded. That didn't negatively affect perf that much, so I think it's fine. But maybe there are not that many things we could cache such that all could be cached?
There was a problem hiding this comment.
Yeah, I had a change for strada from years ago that added a ton of caching to symbol chain lookups, and in strada all it did was drop a bomb on memory usage and didn't really help perf (anything we gained was eaten by extra GC pressure). Different runtime, different automatic optimizations - I'm unsurprised some manual caching helps here.
There was a problem hiding this comment.
Yeah, it goes faster if you really do cache everything. May just cut this down to globals.
There was a problem hiding this comment.
Pull request overview
This PR targets a major performance hotspot in the TypeScript-Go checker’s symbol qualification logic by reducing repeated full-table scans during symbol chain lookup (used by lookupSymbolChain via accessibility checks).
Changes:
- Add an alias-only iteration path (with caching) for large symbol tables to avoid scanning thousands of non-alias entries.
- Replace an inner-loop name equality check with a direct map lookup for the symbol’s name.
- Add a fast-path for the
globalThisfallback to avoid an expensive recursive search when the symbol is directly available in globals underignoreQualification=true.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/checker/symbolaccessibility.go | Introduces alias extraction/caching and refactors trySymbolTable to reduce work per scope/symbol-table scan, plus a globals globalThis fast-path. |
| internal/checker/checker.go | Adds symbolTableAliasCache to Checker to support the new caching behavior. |
| if kind == stKindMembers { | ||
| return nil | ||
| } | ||
| // Globals and exports tables are large and revisited often; use cache. | ||
| if kind == stKindGlobals || kind == stKindExports { |
There was a problem hiding this comment.
Yeah, I had a change for strada from years ago that added a ton of caching to symbol chain lookups, and in strada all it did was drop a bomb on memory usage and didn't really help perf (anything we gained was eaten by extra GC pressure). Different runtime, different automatic optimizations - I'm unsurprised some manual caching helps here.
| // Check for ExportSymbol via direct O(1) lookup instead of inside the alias loop. | ||
| // In the original loop, this checks `symbolFromSymbolTable.Name == ctx.symbol.Name`, | ||
| // but there's at most one such symbol in the table (keyed by name). | ||
| if ok && res != nil && res.ExportSymbol != nil { |
There was a problem hiding this comment.
This is kinda misleadingly wrong since we iterate all the possible chains for result stability nowadays anyway, isn't it? It's not like we skip the full iteration when we find the fast-path candidate. All this does is move those nil checks and a name comparison out of the loop...
There was a problem hiding this comment.
Yeah, I wasn't happy with this, given we do already loop.
There was a problem hiding this comment.
Ah, no, I misread this diff when reading it back. The thing is that the new loop doesn't go over every symbol in the table anymore, just the ones that are aliases. So it can't go back into the loop.
So we'd have to then go back to looping over everything again, or do something more direct. Which, I think this is doing?
|
Unfortunately it seems like if I remove these fast paths (which presumably are iffy), and then restrict to just globals, it goes back to having the same perf... |
|
Drafting this for now, I was overexcited 😄 It's a shame that we have no tests that can prove that this is wrong, though |
After #3213 I was curious if
lookupSymbolChaincould be made faster.Do
go test -cpuprofile=cpu.out -memprofile=mem.out ./internal/testrunnerand look:A big chunk from this. This isn't new, the same thing happens before #3213.
Threw copilot and pprof at it, and it came up with this change:
trySymbolTablenow iterates a cached list of only alias-flagged symbols instead of the full symbol table. The globals table has ~5000 entries but only ~100 aliases, eliminating ~98% of per-symbol flag checks.getExportsOfSymbol/getExportsOfModule) use a distinctstKindResolvedExportsID to avoid cache collisions with rawsym.Exportstables passed bysomeSymbolTableInScope.someSymbolTableInScopealready filters them toSymbolFlagsType & ^SymbolFlagsAssignment, which never includes aliases.symbolFromSymbolTable.ExportSymbolcheck from the original full-table loop is preserved via a direct name lookup, since it's not gated by the Alias flag and would be missed by alias-only iteration.mainlookupSymbolChainis much reduced.