Ditch nodeData interface in favor of generated accessors - #64063
Ditch nodeData interface in favor of generated accessors#64063Jake Bailey (jakebailey) wants to merge 15 commits into
Conversation
Replace nine nodeData interface methods with generated Kind-switch dispatchers to reduce interface method promotion and itab size.
|
TypeScript Bot (@typescript-bot) perf test this |
There was a problem hiding this comment.
Pull request overview
Replaces nodeData interface dispatch with generated kind-based AST accessors to reduce binary size and metadata overhead.
Changes:
- Generates AST visitor, clone, subtree-fact, and accessor dispatch.
- Updates compiler and language-service consumers for the new representation.
- Adds canonical token-payload validation and adjusts direct node construction.
Reviewed changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tools/scripts/tsc/generate-go-ast.ts |
Generates kind-based AST dispatch. |
tools/scripts/tsc/generate-encoder.ts |
Updates generated private-field access. |
tsc/internal/ast/ast.go |
Reworks node storage and common fields. |
tsc/internal/ast/ast_generated.go |
Contains generated factories and dispatch. |
tsc/internal/ast/flow.go |
Adapts synthetic flow-node construction. |
tsc/internal/ast/token_test.go |
Tests canonical token payloads. |
tsc/internal/ast/utilities.go |
Updates type-only checks. |
tsc/internal/api/encoder/encoder.go |
Updates source-file position access. |
tsc/internal/api/encoder/encoder_generated.go |
Updates generated text access. |
tsc/internal/checker/grammarchecks.go |
Uses common node accessors. |
tsc/internal/checker/nodebuilder_hover.go |
Updates optional-token access. |
tsc/internal/ls/autoimport/fix.go |
Updates type-only import access. |
tsc/internal/ls/completions.go |
Updates source-file access. |
tsc/internal/ls/lsutil/organizeimports.go |
Updates import classification. |
tsc/internal/ls/organizeimports.go |
Updates type-only grouping. |
tsc/internal/ls/signaturehelp.go |
Updates type-argument access. |
tsc/internal/parser/reparser.go |
Updates mutable typed-node casts. |
tsc/internal/printer/printer_test.go |
Adapts transformed source-file casts. |
tsc/internal/scanner/scanner_test.go |
Adapts node initialization. |
tsc/internal/transformers/declarations/transform.go |
Updates syntax-list traversal. |
tsc/internal/transformers/estransforms/optionalchain.go |
Updates element-access casting. |
tsc/internal/transformers/moduletransforms/commonjsmodule.go |
Updates common expression/name access. |
tsc/internal/transformers/tstransforms/legacydecorators.go |
Updates decorator access. |
tsc/internal/transformers/tstransforms/runtimesyntax.go |
Updates parameter-list access. |
tsc/internal/transformers/tstransforms/typeeraser.go |
Updates type-only import access. |
Files not reviewed (1)
- tsc/internal/api/encoder/encoder_generated.go: Generated file
Suppressed comments (1)
tsc/internal/ast/flow.go:74
- This node likewise stores
*FlowReduceLabelDataunderKindUnknown, while the generatedCloneand subtree-facts switches assert that everyKindUnknownpayload is*Token. Those methods now panic for reduce-label nodes; previously dynamic dispatch reachedNodeDefault. Use a distinct kind or explicitly handle this synthetic payload in dispatch, with a regression test.
return newNode(KindUnknown, node.AsNode(), node, NodeFactoryHooks{})
|
Jake Bailey (@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: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 26 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- tsc/internal/api/encoder/encoder_generated.go: Generated file
- tsc/internal/ast/kind_stringer_generated.go: Generated file
|
TypeScript Bot (@typescript-bot) perf test this faster |
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Gabriela Araujo Britto (gabritto)
left a comment
There was a problem hiding this comment.
This looks good to me, and like I said, better for the upcoming extra type checking, but not sure other people have opinions.
|
This should be perf neutral but trying one more time TypeScript Bot (@typescript-bot) perf test this faster |
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hm, there's something to this, I think, I need to investigate. |
Replace large sparse Kind switches with byte-indexed dispatch tables and dense switches, while leaving small switches unchanged. Keep the concrete calls visible to escape analysis without restoring promoted interface methods. Add accessor correctness tests and fixture-based benchmarks covering both mixed nodes and nodes with populated accessor results.
|
Sadly, I had to reintroduce indirection to recover the lost perf (which was in fact a real result). I'm sure this won't make the type checking happier, but it does seem to work and makes the binary smaller yet. |
Index generated accessor tables directly with the node kind. Invalid kinds now hit the normal Go bounds check rather than returning nil. Update accessor tests to use valid constructed nodes.
There was a problem hiding this comment.
🔵 Needs a closer look
It broadly changes core AST dispatch and node construction across the compiler, warranting final human validation.
Review details
Files not reviewed (1)
- tsc/internal/ast/kind_stringer_generated.go: Generated file
- Files reviewed: 5/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This ditches the dynamic dispatch with
nodeDatain place of generated kind switches, like I did withForEachChild..text.gopclntab.go.typetsc/internal/astIt also makes compiling the ast package 5% faster.