Add a jump-table for visitEachChild #50266
Merged
Conversation
This file contains 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
typescript-bot
added
Author: Team
For Uncommitted Bug
rbuckton
force-pushed
the
visitEachChildTable
branch
from
419d989
to
56c8d67
Compare
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 56c8d67. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:
CompilerComparison Report - main..50266
System
Hosts
Scenarios
TSServerComparison Report - main..50266
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rbuckton
force-pushed
the
visitEachChildTable
branch
from
56c8d67
to
b43e228
Compare
rbuckton
force-pushed
the
visitEachChildTable
branch
from
b43e228
to
83b60d7
Compare
jakebailey
reviewed
Aug 11, 2022
| @@ -885,6 +885,144 @@ namespace ts { | |||
| /* @internal */ jsDocCache?: readonly JSDocTag[]; // Cache for getJSDocTags | |||
| } | |||
|
|
|||
| /* @internal */ | |||
| export type HasChildren = | |||
There was a problem hiding this comment.
I assume that if we need to update this, we'll know because some other code won't be able to compile?
There was a problem hiding this comment.
To add a method to visitEachChild (and potentially forEachChild), we'd have to add the node to this union.
jakebailey
reviewed
Aug 11, 2022
| // Names | ||
|
|
||
| case SyntaxKind.Identifier: | ||
| Debug.type<Identifier>(node); |
There was a problem hiding this comment.
This is maybe a silly question, but are we sure the perf boost here isn't only because the new code never calls Debug.type?
There was a problem hiding this comment.
Debug.type() had almost no impact on perf when I added it. It's a noop, so inlining pretty much results in elimination. It definitely wouldn't account for a 3% difference in emit time.
amcasey
reviewed
Aug 11, 2022
| @@ -885,6 +885,144 @@ namespace ts { | |||
| /* @internal */ jsDocCache?: readonly JSDocTag[]; // Cache for getJSDocTags | |||
| } | |||
|
|
|||
| /* @internal */ | |||
| export type HasChildren = | |||
There was a problem hiding this comment.
What happens if someone adds a new syntax kind and forgets to update this? Seems like their new node might not emit its children?
There was a problem hiding this comment.
AFAIK that's what would have happened in the old code too, because it'd hit the default case to just do nothing.
There was a problem hiding this comment.
If both this and #50225 use HasChildren, then you wouldn't be able to define a forEachChild branch or visitEachChild branch without adding an entry, and if you add an entry to HasChildren you would have compile time errors if you forget to add branches to forEachChild and visitEachChild.
rbuckton
force-pushed
the
visitEachChildTable
branch
from
83b60d7
to
35b5e31
Compare
|
One more perf test to see if it's worth dropping the early check for tokens in |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 35b5e31. You can monitor the build here. Update: The results are in! |
| @@ -378,929 +378,799 @@ namespace ts { | |||
| export function visitEachChild<T extends Node>(node: T | undefined, visitor: Visitor, context: TransformationContext, nodesVisitor?: typeof visitNodes, tokenVisitor?: Visitor): T | undefined; | |||
| /* @internal */ | |||
| export function visitEachChild<T extends Node>(node: T | undefined, visitor: Visitor, context: TransformationContext, nodesVisitor?: NodesVisitor, tokenVisitor?: Visitor, nodeVisitor?: NodeVisitor): T | undefined; // eslint-disable-line @typescript-eslint/unified-signatures | |||
| export function visitEachChild(node: Node | undefined, visitor: Visitor, context: TransformationContext, nodesVisitor = visitNodes, tokenVisitor?: Visitor, nodeVisitor: NodeVisitor = visitNode): Node | undefined { | |||
| export function visitEachChild<T extends Node>(node: T | undefined, visitor: Visitor, context: TransformationContext, nodesVisitor = visitNodes, tokenVisitor?: Visitor, nodeVisitor: NodeVisitor = visitNode): T | undefined { | |||
There was a problem hiding this comment.
Isn't the input possibly unrelated to the output? @jakebailey noticed something like this while converting to SFT.
There was a problem hiding this comment.
It totally is. In this case, the implementation fails under SFT because it doesn't overlap with its overloads, except that we don't properly check that without SFT which is why this weird narrower implementation works.
Changing the signature to match the overload is totally fine; no worse than what we have now and users won't see the difference. Actual SFT support and a corrected signature will have to come later when we're ready for a break.
There was a problem hiding this comment.
I'm asserting visitEachChildTable[node.kind] as VisitEachChildFunction<T> | undefined. It's difficult to prove this in the type system, but VisitEachChildTable[T["kind"]] should be VisitEachChildFunction<T> | undefined. This isn't perfect, but its better than any.
There was a problem hiding this comment.
We also don't have a great way of expressing the existing overloads in the type system. visitEachChild<BinaryExpressionButWithExtraTypeInfo>(...) would say it produces a BinaryExpressionButWithExtraTypeInfo, but would actually produce a BinaryExpressionButWithExtraTypeInfo | BinaryExpression depending on whether we updated the node. Unless we introduce a big ADT union of all possible Node subtypes, I don't think this can be improved.
There was a problem hiding this comment.
Hm, unless I'm mistaken, the transform API is used heavily to completely and entirely replace nodes when visiting with... whatever, even nodes of differing types. So, I would expect that a fixed signature would instead have an overload with a more specific return first, but also a more generic "it's just a node" when the result can't be inferred.
But, that's offtopic.
There was a problem hiding this comment.
visitEachChild always returns the same type of node passed as the first argument. You can only update child nodes using the supplied visitor.
There was a problem hiding this comment.
Ah, oops, I think Daniel and I are thinking of visitNode and visitNodes, which do have a bogus signature.
There was a problem hiding this comment.
It is and it isn't. visitNode is really supposed to be used with a test parameter, such that you validate the result is a T when visiting a class of nodes such as Statement, Expression, etc. The test is optional, but really shouldn't be. Unfortunately, making test a required argument would probably be a breaking change, and we'd have to expose some of the higher-level node tests (i.e., isStatement, isExpression, isClassElement, etc.) in the public API that are currently internal.
|
@rbuckton Here they are:
CompilerComparison Report - main..50266
System
Hosts
Scenarios
TSServerComparison Report - main..50266
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rbuckton
force-pushed
the
visitEachChildTable
branch
from
35b5e31
to
dadca7f
Compare
DanielRosenwasser
approved these changes
Aug 17, 2022
weswigham
approved these changes
Aug 17, 2022
jakebailey
approved these changes
Aug 17, 2022
|
Have we decided on whether we prefer the Array or Object Literal variant for jump tables? |
|
Does the same type safety work for the array? My guess was not (especially if we are using push to keep the array compact) |
No, it doesn't. To do the same thing with an array would require using an array literal and packing it with leading |
|
For me, if the perf is the same, I'd prefer the safer one. |
|
Yeah, ultimately I didn't see enough of a win doing one over the other. |
This was referenced Dec 9, 2022
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.
Similar to #50225, this creates a table to handle each potential syntax kind in
visitEachChild.