Skip to content
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

Switch let/const to var in the scanner & parser for top-levelish variables. #52832

Merged
merged 2 commits into from
Feb 18, 2023

Conversation

DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Feb 17, 2023

This pull request regains some of the performance we lost in #51387 when we switched our emit from ES5 to ES2018 - a significant part due to TDZ checks that engines need to perform. These TDZ checks can often be eliminated, but engines probably have a hard time knowing whether a function can eliminate these checks.

By switching many of the top-level shared closure variables in the parser and scanner from let/const to var, we're able to eliminate those checks. Doing so should be fairly safe, since our functions follow the same initialization mechanism of declaring variables, and then calling the "workhorse" function afterwards.

@DanielRosenwasser
Copy link
Member Author

@typescript-bot perf test this faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 17, 2023

Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at 450cfe1. You can monitor the build here.

@DanielRosenwasser
Copy link
Member Author

@typescript-bot perf test this faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 17, 2023

Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at cd8b3a6. You can monitor the build here.

Update: The results are in!

@DanielRosenwasser DanielRosenwasser changed the title Switch let to var in the parser for top-levelish variables. Switch let to var in the scanner & parser for top-levelish variables. Feb 17, 2023
@DanielRosenwasser DanielRosenwasser changed the title Switch let to var in the scanner & parser for top-levelish variables. Switch let/const to var in the scanner & parser for top-levelish variables. Feb 17, 2023
@typescript-bot
Copy link
Collaborator

@DanielRosenwasser
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..52832

Metric main 52832 Delta Best Worst p-value
Angular - node (v16.17.1, x64)
Memory used 359,093k (± 0.01%) 358,732k (± 0.00%) -361k (- 0.10%) 358,719k 358,755k p=0.005 n=6
Parse Time 4.16s (± 0.42%) 3.72s (± 0.55%) 🟩-0.44s (-10.61%) 3.69s 3.75s p=0.005 n=6
Bind Time 1.24s (± 0.33%) 1.22s (± 0.68%) -0.02s (- 1.34%) 1.22s 1.24s p=0.009 n=6
Check Time 9.52s (± 0.44%) 9.50s (± 0.25%) ~ 9.47s 9.53s p=0.332 n=6
Emit Time 8.10s (± 0.68%) 8.05s (± 0.51%) ~ 7.98s 8.08s p=0.142 n=6
Total Time 23.02s (± 0.40%) 22.50s (± 0.27%) -0.53s (- 2.28%) 22.40s 22.57s p=0.005 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 191,731k (± 0.02%) 191,531k (± 0.05%) -200k (- 0.10%) 191,454k 191,717k p=0.013 n=6
Parse Time 1.81s (± 0.97%) 1.57s (± 0.35%) 🟩-0.24s (-13.38%) 1.56s 1.57s p=0.004 n=6
Bind Time 0.84s (± 0.48%) 0.84s (± 0.48%) ~ 0.84s 0.85s p=1.000 n=6
Check Time 10.13s (± 0.70%) 10.16s (± 0.69%) ~ 10.07s 10.26s p=0.419 n=6
Emit Time 3.04s (± 0.70%) 3.07s (± 1.41%) ~ 3.01s 3.12s p=0.374 n=6
Total Time 15.82s (± 0.62%) 15.63s (± 0.64%) -0.18s (- 1.16%) 15.51s 15.80s p=0.030 n=6
Monaco - node (v16.17.1, x64)
Memory used 343,537k (± 0.01%) 343,235k (± 0.01%) -302k (- 0.09%) 343,196k 343,251k p=0.005 n=6
Parse Time 3.10s (± 0.53%) 2.78s (± 0.42%) 🟩-0.31s (-10.17%) 2.77s 2.80s p=0.005 n=6
Bind Time 1.11s (± 0.73%) 1.11s (± 0.00%) ~ 1.11s 1.11s p=0.290 n=6
Check Time 7.79s (± 0.70%) 7.80s (± 0.38%) ~ 7.76s 7.83s p=0.746 n=6
Emit Time 4.51s (± 0.33%) 4.50s (± 0.50%) ~ 4.48s 4.54s p=0.796 n=6
Total Time 16.51s (± 0.37%) 16.20s (± 0.24%) -0.32s (- 1.92%) 16.13s 16.24s p=0.005 n=6
TFS - node (v16.17.1, x64)
Memory used 299,646k (± 0.00%) 299,372k (± 0.00%) -274k (- 0.09%) 299,356k 299,381k p=0.005 n=6
Parse Time 2.45s (± 1.08%) 2.18s (± 0.73%) 🟩-0.27s (-10.89%) 2.15s 2.19s p=0.004 n=6
Bind Time 1.26s (± 0.65%) 1.26s (± 0.82%) ~ 1.24s 1.27s p=0.932 n=6
Check Time 7.23s (± 0.43%) 7.22s (± 0.45%) ~ 7.17s 7.25s p=0.686 n=6
Emit Time 4.21s (± 0.47%) 4.22s (± 1.08%) ~ 4.16s 4.29s p=0.570 n=6
Total Time 15.14s (± 0.29%) 14.88s (± 0.36%) -0.27s (- 1.76%) 14.82s 14.98s p=0.005 n=6
material-ui - node (v16.17.1, x64)
Memory used 475,972k (± 0.00%) 475,695k (± 0.01%) -278k (- 0.06%) 475,656k 475,720k p=0.005 n=6
Parse Time 3.68s (± 0.24%) 3.33s (± 0.52%) 🟩-0.35s (- 9.56%) 3.31s 3.36s p=0.005 n=6
Bind Time 1.02s (± 0.62%) 1.02s (± 0.00%) ~ 1.02s 1.02s p=1.000 n=6
Check Time 18.31s (± 1.24%) 18.21s (± 0.54%) ~ 18.11s 18.34s p=0.572 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 23.01s (± 0.97%) 22.56s (± 0.49%) -0.45s (- 1.96%) 22.46s 22.69s p=0.005 n=6
xstate - node (v16.17.1, x64)
Memory used 546,950k (± 0.02%) 546,050k (± 0.01%) -901k (- 0.16%) 545,981k 546,178k p=0.005 n=6
Parse Time 4.77s (± 0.54%) 4.24s (± 0.23%) 🟩-0.53s (-11.11%) 4.23s 4.26s p=0.004 n=6
Bind Time 1.85s (± 0.22%) 1.80s (± 0.30%) -0.05s (- 2.52%) 1.80s 1.81s p=0.003 n=6
Check Time 3.07s (± 0.78%) 3.08s (± 0.57%) ~ 3.05s 3.10s p=0.870 n=6
Emit Time 0.09s (± 4.45%) 0.09s (± 0.00%) ~ 0.09s 0.09s p=0.405 n=6
Total Time 9.78s (± 0.45%) 9.21s (± 0.26%) 🟩-0.57s (- 5.84%) 9.18s 9.25s p=0.005 n=6
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-135-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
Benchmark Name Iterations
Current 52832 6
Baseline main 6

Developer Information:

Download Benchmark

@jakebailey
Copy link
Member

Yay!!!

@DanielRosenwasser DanielRosenwasser marked this pull request as ready for review February 17, 2023 23:06
@jakebailey
Copy link
Member

Aside: is it just the scanner? Can we revert parser and test only the scanner? Is it a specific variable? That would be very interesting to know.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@@ -1031,6 +1032,8 @@ export function createScanner(languageVersion: ScriptTarget,
scanRange,
};

/* eslint-disable no-var */
Copy link
Contributor

@bakkot bakkot Feb 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by - was this maybe supposed to be eslint-enable? It's already disabled above, and below there's no new vars.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh oh, sure was!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants