Devirtualize ForEachChild and IterChildren#4395
Open
jakebailey wants to merge 2 commits into
Open
Conversation
(*Node).ForEachChild previously dispatched through the nodeData interface (`n.data.ForEachChild(v)`). An interface call is opaque to escape analysis, which must then assume the visitor `v` escapes; this forced every caller's visitor closure -- and any locals it captured -- onto the heap. Generate a (*Node).ForEachChild that instead switches on n.Kind and calls the concrete node type's method directly (n.data.(*T).ForEachChild(v)). The statically-resolved call lets escape analysis prove the visitor does not escape, keeping caller closures on the stack, and the integer switch compiles to a jump table that is cheaper than the interface call it replaces. A recursive walk of checker.ts (the language-server traversal pattern, passing a fresh closure per node) drops from ~596k allocations to 0 and runs ~4.2x faster. Sites that already cached their visitor (e.g. the binder's bindFunc) are unaffected; this makes that optimization automatic everywhere.
Node.IterChildren was doubly affected by escape analysis being unable to see through dynamic calls. (*Node).IterChildren() dispatched through the nodeData interface (`n.data.IterChildren()`), returning a closure, and each iteration allocated two closures: the bound method value `forEachChildIter` plus the `invert(yield)` wrapper used to reconcile TS-visitor early-return semantics (`true` stops) with Go iterator semantics (`false` stops). Implement IterChildren directly on *Node, using the now-devirtualized ForEachChild and folding the inversion into the yield. Because ForEachChild no longer leaks its visitor, the iterator and its inner closure stay on the stack. Remove the now-unused nodeData.IterChildren interface method, NodeDefault.IterChildren, forEachChildIter, and invert. A recursive walk of checker.ts via IterChildren drops from ~1.19M allocations to 0 and runs ~8x faster.
Member
Author
|
@typescript-bot perf test this |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR devirtualizes AST child traversal by moving (*Node).ForEachChild from an interface dispatch (node.data.ForEachChild) to a generated switch n.Kind dispatcher, and by implementing IterChildren directly on Node. The goal is to make the concrete ForEachChild call visible to Go escape analysis so visitor/yield closures are less likely to escape to the heap (and may inline more often).
Changes:
- Implement
(*Node).IterChildren()directly inast.go, removingIterChildrenfromnodeData/NodeDefault. - Add a generated
(*Node).ForEachChild(v Visitor) boolkind-switch dispatcher inast_generated.go. - Update
_scripts/generate-go-ast.tsto generate the newForEachChilddispatch block.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/ast/ast.go | Implements IterChildren directly on Node and removes IterChildren from the nodeData interface/default implementation. |
| internal/ast/ast_generated.go | Adds the generated (*Node).ForEachChild kind-switch dispatcher that calls concrete node ForEachChild methods. |
| _scripts/generate-go-ast.ts | Generates the new ForEachChild dispatcher based on schema nodes that have children (or are hand-written like SourceFile). |
Files not reviewed (1)
- internal/ast/ast_generated.go: Generated file
|
@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: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Now that we have code gen, generating a function to do the mapping instead of relying on interface calls is now possible. Doing this lets escape analysis see the actual functions being called, which then allows for the functions to not escape and potentially even inline.