Store value symbol links inline on checker-created symbols - #4703
Open
mds-ant wants to merge 4 commits into
Open
Store value symbol links inline on checker-created symbols#4703mds-ant wants to merge 4 commits into
mds-ant wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Moves value-symbol links inline for checker-created symbols, reducing map traffic while retaining per-checker storage for binder symbols.
Changes:
- Adds inline checker data to transient symbols.
- Introduces unified value-link accessors.
- Migrates checker call sites to the new storage abstraction.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/ast/symbol.go |
Adds checker-owned side data. |
internal/checker/symbollinks.go |
Implements inline link storage and accessors. |
internal/checker/checker.go |
Allocates transient symbols and migrates link access. |
internal/checker/services.go |
Uses the new accessors. |
internal/checker/nodebuilderimpl.go |
Migrates node-builder link access. |
internal/checker/jsx.go |
Migrates JSX symbol links. |
internal/checker/inference.go |
Migrates inference symbol links. |
internal/checker/exports.go |
Updates exported name-type lookup. |
Member
|
@typescript-bot perf test this |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
lspComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jakebailey
reviewed
Jul 22, 2026
| ExportSymbol *Symbol | ||
| // CheckerData is side data attached by the one Checker that created this symbol; nil on | ||
| // binder-created symbols, which are shared between checkers. Owned by the checker package. | ||
| CheckerData any |
Member
There was a problem hiding this comment.
Right now Symbol is 96 bytes with no slack, so this makes every symbol larger. Maybe that's not a major problem?
Contributor
Author
There was a problem hiding this comment.
On the VS Code corpus, this change is still net-positive on memory overall (see the "Memory" section). Looking at the "Memory Used" table rows in the perf results, the general increase seems to be negligibly small (mostly 0.0x%).
jakebailey
reviewed
Jul 22, 2026
| // transientSymbol co-allocates a checker-created symbol with its value symbol links, reached via | ||
| // symbol.CheckerData. Such symbols are owned by the creating Checker alone and must never be | ||
| // handed to another checker; binder symbols use c.binderValueSymbolLinks (only via the accessors). | ||
| type transientSymbol struct { |
Member
There was a problem hiding this comment.
This is a nit, but we probably don't need a whole new file just for these 40 lines.
Contributor
Author
There was a problem hiding this comment.
Your call! Happy to inline.
Member
There was a problem hiding this comment.
That being said #4329 is probably going to provide a good place to put this, so probably not worth touching right now until that's in and you have to rebase
Member
There was a problem hiding this comment.
I'm also curious whether the performance gains from this PR persist after #4329.
Contributor
Author
There was a problem hiding this comment.
mds-ant
force-pushed
the
claude/tsgo-checker-perf-2e234b
branch
from
July 23, 2026 09:57
edd2d57 to
102a349
Compare
Member
|
@typescript-bot perf test this |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
lspComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
|
Seems like it's in the noise? |
A few call sites asked `valueSymbolLinks.Has` and then immediately fetched the same entry with `Get` or `TryGet`, probing the map two or three times for one answer. The `Get` behind a positive `Has` can never create anything, so a single `TryGet` plus a nil check does the same job with one probe.
The checker keeps per-symbol side data in pointer-keyed `LinkStore` maps, and `valueSymbolLinks` is by far the hottest of them. Instantiation is the worst case: every freshly instantiated symbol does a guaranteed-miss map probe, an arena allocation, and a map insert, just to get an empty links struct back. Symbols the checker makes for itself are only ever seen by that checker, so their links can sit right next to the symbol: the arena now hands out `transientSymbol`, an `ast.Symbol` plus an inline `checkerSymbolData` block that the new `ast.Symbol.CheckerData` field points at. Binder symbols are shared across checkers and stay on the map; their `CheckerData` is nil. `getValueSymbolLinks` and `tryGetValueSymbolLinks` behave exactly like the map's `Get` and `TryGet`, and call sites move over in the next commit. TypeScript does the same: `TransientSymbol` carries its `SymbolLinks` inline.
Purely mechanical: every `c.valueSymbolLinks.Get` and `.TryGet` becomes `getValueSymbolLinks`/`tryGetValueSymbolLinks`, so symbols created by the checker hit their inline links instead of the map. Checker-created symbols no longer pay the probe on each access or the insert on first touch.
Only binder-created symbols live in this map now, and nothing outside the accessors should touch it. The old name made the muscle-memory spelling `c.valueSymbolLinks.Get(symbol)` compile fine while silently skipping a transient symbol's inline links; the new name makes that mistake impossible to type by habit.
mds-ant
force-pushed
the
claude/tsgo-checker-perf-2e234b
branch
from
July 24, 2026 16:15
102a349 to
f122ecb
Compare
Contributor
Author
|
@jakebailey I just rebased and reran the benchmark. For single-threaded checking, we're no longer seeing a performance improvement now that #4329 has been merged, but we are still seeing wins for multi-threaded checking. |
Member
|
Hmm, if so I do still expect to see a result: @typescript-bot perf test this |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
lspComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
|
It does seem 3% faster on vscode, at least. |
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.
Context
Since #4329, value symbol links live in a paged store indexed by symbol ID, which already removed the hash map probes they used to cost. What remains is the machinery around each access: loading or assigning the symbol's ID, walking the page table, and, on first touch, allocating the links struct in a separate arena. On a full
--noEmitcheck of VS Code'ssrcproject, the paged store handles 47.5M accesses and 7.1M links allocations, and 11.4M of those accesses plus 5.5M of those allocations belong to symbols the checker created itself.Those symbols are only ever seen by the checker that created them, so this PR stores their links directly on the symbol: the checker's arena now hands out a
transientSymbol, anast.Symbolplus an inlinecheckerSymbolDatablock reached through the newast.Symbol.CheckerDatafield. An access becomes a single pointer chase with no ID, no page walk, and no separate allocation. Binder symbols are shared across checkers and stay on the per-checker paged store withCheckerDatanil. TypeScript does the same thing:TransientSymbolcarries itsSymbolLinksinline.The work is split into four commits:
Has+Get/TryGetlookups into a singleTryGetat the few sites that used them.getValueSymbolLinks/tryGetValueSymbolLinksaccessors.binderValueSymbolLinks.Performance
Measured on VS Code's
srcproject with--noEmitagainst main including #4329:Memory
ast.Symbolgrows by 16 bytes for the newCheckerDatafield, including binder symbols that never use it.This PR was assisted by Claude Code.