Swap forEachChild to use a table of functions instead of a switch statement.
#50225
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 perf test this faster |
|
Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at f671ea7. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser Here they are:Comparison Report - main..50225
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this faster |
|
Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at e42cfaf. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser Here they are:Comparison Report - main..50225
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot test this |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at e42cfaf. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at e42cfaf. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at e42cfaf. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at e42cfaf. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser |
|
Heya @DanielRosenwasser, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@DanielRosenwasser Here they are:
CompilerComparison Report - main..50225
System
Hosts
Scenarios
TSServerComparison Report - main..50225
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typescript-bot
added
Author: Team
For Uncommitted Bug
DanielRosenwasser
requested review from
ahejlsberg,
jakebailey,
rbuckton and
sheetalkamat
sheetalkamat
approved these changes
Aug 9, 2022
|
I wonder if the same benefit could be found in const forEachChildTable: Partial<Record<SyntaxKind, ForEachChildFunction>> = {
[SyntaxKind.QualifiedName]: forEachQualifiedName,
[SyntaxKind.TypeParameter]: forEachTypeParameter,
// etc.
}; |
If an object has identical perf, then I'd assume it's an indication that the array being constructed is sparse - but we need the look-up to be fast. If there's an easy way to create a definitely-packed array, then I'll switch to it here. All the tests do admittedly indicate higher memory usage - but more than I would've expected.
It's definitely worth playing with, especially since |
amcasey
approved these changes
Aug 9, 2022
src/compiler/parser.ts
Outdated
| @@ -95,6 +95,979 @@ namespace ts { | |||
| return isMetaProperty(node) && node.keywordToken === SyntaxKind.ImportKeyword && node.name.escapedText === "meta"; | |||
| } | |||
|
|
|||
| type ForEachChildFunction = <T>(node: any, cbNode: (node: Node) => T | undefined, cbNodes?: (nodes: NodeArray<Node>) => T | undefined) => T | undefined; | |||
|
|
|||
| const forEachChildTable = new Array<ForEachChildFunction>(SyntaxKind.Count); | |||
There was a problem hiding this comment.
Some thoughts on this initialization mechanism:
- AFAIK, the ctor arg does nothing (@jakebailey?)
- The array will always be sparse (among other things, we're always skipping the first 160 elements)
- We don't have any protection against specifying two different functions for the same kind.
There was a problem hiding this comment.
Yes, AFAIK the size argument doesn't actually make this a compact array or anything, hence them trying to add a Array.withCapacity instead or something.
|
@DanielRosenwasser piles of people have signed off on this PR; should we merge it for 4.9? |
|
It sounds like Ron and Jake are both on board with introducing an internal So I think we should get one more sign-off on Ron's change first at #50266. Once that's in, I'll update this PR to leverage the same |
DanielRosenwasser
changed the title
forEachChild to use an array of functions instead of a switch statement.forEachChild to use a table of functions instead of a switch statement.
|
@typescript-bot perf test this faster |
|
Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at cc9ebf0. You can monitor the build here. Update: The results are in! |
|
Because integration with the I want to be able to untangle the perf wins here with whatever new stuff that might introduce. |
Comment on lines
+758
to
+759
| // TODO: should we separate these branches out? | ||
| visitNode(cbNode, (node as CallExpression).questionDotToken) || |
There was a problem hiding this comment.
I'll do this for a follow-up change.
|
@DanielRosenwasser Here they are:Comparison Report - main..50225
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Those perf results make me happy every time I see them. |
rbuckton
approved these changes
Aug 19, 2022
Comment on lines
+697
to
+703
| node.typeExpression.kind === SyntaxKind.JSDocTypeExpression | ||
| ? visitNode(cbNode, node.typeExpression) || | ||
| visitNode(cbNode, node.fullName) || | ||
| (typeof node.comment === "string" ? undefined : visitNodes(cbNode, cbNodes, node.comment)) | ||
| : visitNode(cbNode, node.fullName) || | ||
| visitNode(cbNode, node.typeExpression) || | ||
| (typeof node.comment === "string" ? undefined : visitNodes(cbNode, cbNodes, node.comment))); |
There was a problem hiding this comment.
The indentation here makes it a bit difficult to read.
There was a problem hiding this comment.
I agree, it's awful - but I believe it's from the original code. I'll fix it up in the HasChildren PR.
There was a problem hiding this comment.
I guess I have some lints I have to fix so... I guess I'll fix now.
This was referenced Dec 9, 2022
|
This was a welcome addition. Nice work @DanielRosenwasser I'm just going to leave this here as something I've noticed. About enums, switches and performance in V8. V8 doesn't like switches with enums, it does however deal with const enums very differently. I wonder what would happen if the SyntaxKind enum was made into a const enum (and you kept the switch). Changing the SyntaxKind enum into a cost enum creates other problems but I think the performance "problem" with the switch statement has more to do so that it isn't being generated into a jump table in machine code when you don't use literal integer numbers (SMIs) as case labels. I don't remember the exact number but I had a switch with 250+ cases in it and when I changed from enum to const enum it made a huge difference. |
|
Oh really, interesting! Thanks for letting me know. |
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.
forEachChildis one of the core pieces of machinery we have for traversing nodes in our trees. It's coded using a fairly largeswitchstatement; however, while reading up on an emulator built in JavaScript (https://artemis.sh/2022/08/07/emulating-calculators-fast-in-js.html), the author called out that most engines do not seem to do the jump-table optimization that some compilers use forswitchstatements.So this change introduces a function table for all of
forEachChild.In some other experimentation (namely #50245) it seems like the failure to optimize might have more to do with the function size than the
switch/caseitself. Regardless, this change seems to cut off around 50ms - 150ms from each perf test's compile time, and speeds up several operations in the language service.