-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Destructure scanner methods in parser #53144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Destructure scanner methods in parser #53144
Conversation
This might be faster!
@typescript-bot perf test this |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 2f30aee. You can monitor the build here. Update: The results are in! |
Local run on node 19 with If the perf machine agrees, this is a good change even if it's only a small bump. Edit: who knows, the performance bump might happen just because there's less source text. |
The failing test failed to download the action at all. I don't think it's relevant in any case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some subset of these we could inline instead of doing all of them, sorta like we did factories?
getTokenText, | ||
getTokenValue, | ||
hasExtendedUnicodeEscape, | ||
hasExtendedUnicodeEscape: scanner_hasExtendedUnicodeEscape, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it bad that I wish these all were scanner_
prefixed? Maybe it's not such a big deal to remember where these come from, I suppose.
@DanielRosenwasser Here they are:
CompilerComparison Report - main..53144
System
Hosts
Scenarios
TSServerComparison Report - main..53144
System
Hosts
Scenarios
StartupComparison Report - main..53144
System
Hosts
Scenarios
Developer Information: |
Eek, it's worse? |
|
This might be faster!
Addresses #52959