FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Switch let/const to var in the scanner & parser for top-levelish variables. by DanielRosenwasser · Pull Request #52832 · microsoft/TypeScript · GitHub

/ TypeScript Public

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

Merged
Daniel Rosenwasser (DanielRosenwasser) merged 2 commits into
mainfrom
letToVarInParser
Feb 18, 2023
ConversationCommits (2)ChecksFiles changed
Merged

Switch let/const to var in the scanner & parser for top-levelish variables.#52832
Daniel Rosenwasser (DanielRosenwasser) merged 2 commits into
mainfrom
letToVarInParser

Conversation

Daniel Rosenwasser (DanielRosenwasser) commented Feb 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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.

Daniel Rosenwasser (DanielRosenwasser) commented Feb 17, 2023

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) perf test this faster

TypeScript Bot (typescript-bot) commented Feb 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Daniel Rosenwasser (DanielRosenwasser) commented Feb 17, 2023

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) perf test this faster

TypeScript Bot (typescript-bot) commented Feb 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Daniel Rosenwasser (@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!

Daniel Rosenwasser (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
Daniel Rosenwasser (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 (typescript-bot) commented Feb 17, 2023

Copy link
Copy Markdown
Contributor

Daniel Rosenwasser (@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

Jake Bailey (jakebailey) commented Feb 17, 2023

Copy link
Copy Markdown
Member

Yay!!!

Daniel Rosenwasser (DanielRosenwasser) marked this pull request as ready for review February 17, 2023 23:06

Jake Bailey (jakebailey) commented Feb 17, 2023

Copy link
Copy Markdown
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.

This was referenced Feb 17, 2023
Closed
Closed
Jake Bailey (jakebailey) approved these changes Feb 18, 2023
View reviewed changes

Jake Bailey (jakebailey) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

TypeScript Bot (typescript-bot) added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Feb 18, 2023
Merged
Kevin Gibbons (bakkot) reviewed Feb 18, 2023
View reviewed changes
Comment thread src/compiler/scanner.ts
scanRange,
};

/* eslint-disable no-var */

Kevin Gibbons (bakkot) Feb 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Jake Bailey (jakebailey) Feb 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Uh oh, sure was!

Jake Bailey (jakebailey) mentioned this pull request Feb 21, 2023
Merged
Closed
Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
This file contains hidden or 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
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Jake Bailey (jakebailey) jakebailey approved these changes
+1 more reviewer
Kevin Gibbons (bakkot) bakkot left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

Daniel Rosenwasser (DanielRosenwasser)

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL