GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Switch let/const to var in the scanner & parser for top-levelish variables. by DanielRosenwasser · Pull Request #52832 · microsoft/TypeScript

Conversation

@DanielRosenwasser

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

Uh oh!

There was an error while loading. Please reload this page.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
robpalme, unional, paulirish, SlurpTheo, and alexandercerutti reacted with eyes emoji
All reactions
  • 👀 5 reactions
@DanielRosenwasser
Switch let to var in the parser for top-levelish variables.
450cfe1
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Feb 17, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

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

Uh oh!

There was an error while loading. Please reload this page.

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser
Switch more in parser to var, switch scanner shared state to var too.
cd8b3a6
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Feb 17, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

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

Uh oh!

There was an error while loading. Please reload this page.

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser 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
@DanielRosenwasser 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 (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
strager, zhangenming, zyhou, k-egor-smirnov, BasixKOR, and jakeisnt reacted with rocket emoji strager and yhdgms1 reacted with eyes emoji
All reactions
  • 🚀 6 reactions
  • 👀 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jakebailey

Jake Bailey (jakebailey) commented Feb 17, 2023

Copy link
Copy Markdown
Member

Yay!!!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) marked this pull request as ready for review February 17, 2023 23:06
@jakebailey

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Feb 17, 2023
[experiment] Run babel over output for let/const to var #52656
Closed
Switch let/const to var in the scanner for top-levelish variables. #52833
Closed
jakebailey
Jake Bailey (jakebailey) approved these changes Feb 18, 2023
View reviewed changes

@jakebailey Jake Bailey (jakebailey) left a comment

Copy link
Copy Markdown
Member

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.

:shipit:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@typescript-bot TypeScript Bot (typescript-bot) assigned Daniel Rosenwasser (DanielRosenwasser) Feb 18, 2023
@typescript-bot TypeScript Bot (typescript-bot) added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Feb 18, 2023
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) merged commit 0463766 into main Feb 18, 2023
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) deleted the letToVarInParser branch February 18, 2023 01:07
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Feb 18, 2023
Swap closure state in the type-checker to var to avoid TDZ checks. #52835
Merged
bakkot
Kevin Gibbons (bakkot) reviewed Feb 18, 2023
View reviewed changes
Comment thread src/compiler/scanner.ts
scanRange,
};

/* eslint-disable no-var */

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
DanielRosenwasser, SlurpTheo, and ExE-Boss reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

@jakebailey Jake Bailey (jakebailey) Feb 18, 2023

Copy link
Copy Markdown
Member

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.

Uh oh, sure was!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jakebailey Jake Bailey (jakebailey) mentioned this pull request Feb 21, 2023
Switch to var in binder for top level variables #52903
Merged
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Feb 22, 2023
Using var in limited contexts to avoid runtime TDZ checks #52924
Closed
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@DanielRosenwasser 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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@DanielRosenwasser @typescript-bot @jakebailey @bakkot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.