Destructure common factory methods in the parser. #52920
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
|
@typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 41257fe. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser Here they are:
CompilerComparison Report - main..52920
System
Hosts
Scenarios
TSServerComparison Report - main..52920
System
Hosts
Scenarios
StartupComparison Report - main..52920
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Seems to be a percent or so? Still, pretty cool. |
|
My guess is that an engine would need to still check each time in case these are possibly |
|
All of the I'm a little concerned we'd be introducing complexity for little actual benefit, as reaching for new We also might want to limit the local references to factory functions to only those involved in hot paths, i.e., |
|
I'm definitely open to it. Plus, I think that long-term, we should actually run a different experiment - use the functions directly, and just write a lint rule to say "nobody apart from the parser and the factory should import the |
The
They might show a difference given they do open projects like the compiler or xstate, but because the benchmarks capture the entire "open" call or the entire "geterr" call, every stage shows up together, so it doesn't move the needle very much. |
You can't just "use the functions directly" with the current design, as they depend on closed-over parameters to control behavior like automatic parenthesis wrapping. Also, I'm working on a completely different refactor related to dropping |
|
I'm mostly "pro" this change, but, if we want to get this into 5.0, should we limit its scope down to the common ones? Or should we just skip this for 5.0? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
DanielRosenwasser
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@typescript-bot perf test faster |
|
Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at 1015e3a. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser Here they are:Comparison Report - main..52920
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rbuckton
approved these changes
Feb 24, 2023
|
But the results for half of these make no sense. |
|
@typescript-bot perf test faster |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 1015e3a. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here they are:Comparison Report - main..52920
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
That one seems like the benchmark that I can actually pretend to trust! Do we want to remove the "chain" functions? I feel like those are rarer. |
|
I'm fine either way. Without doing all of them the, difference feels really small already, but this can't hurt. |
|
On a serious note, we've got two runs that show slightly faster parsing results on a few projects - so that does feel like an improvement. The biggest suspicion to me is the xstate benchmark, which is dominated by parse time at the moment, but whose parse time doesn't get faster at all outside of Node 14. It feels like there are some savings to be had there |
This pull request was closed.
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.
In #35282, we switched over to using factory functions within the parser - this ensured that nodes of the same type were more uniformly initialized, but with a severe slowdown in the parser. This presumably came from several new indirections like object member access and method invocation costs.
This PR gets some performance savings back from directly destructuring every factory method from the parser's factory. It ends up being a relatively small amount saved, but I figured we might want to discuss it anyway.