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

Move complex parameter lists of async function into downlevel generator body #56296

Merged
merged 5 commits into from Nov 3, 2023

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Nov 2, 2023

This modifies our down-level emit for async functions when they contain non-trivial parameter lists that may potentially throw when bound. Prior to this change, the following would throw immediately when it should instead produce a rejected promise:

// @target: es2015
// @lib: esnext
async function f(x, y = doesNotExist) {}
f();

The reason for this is that we eagerly evaluate the parameter list in the down-level emit:

function f(x, y = doesNotExist) {
  return __awaiter(this, void 0, void 0, function*() {});
}

To address this, this PR repurposes the second parameter to __awaiter, which was previously only used to bind a lexically-captured arguments, to instead be used to forward arguments for the function to the down-level generator body, producing the following emit instead:

function f(x_1) {
  return __awaiter(this, arguments, void 0, function*(x, y = doesNotExist) {});
}

When the transformed function is not an arrow function, we merely forward the function's arguments and emit placeholders for each fixed parameter to preserve the function's length, as can be seen with x_1 above.

Arrow functions, however, do not have their own arguments. In that case, we instead capture all fixed arguments, followed by a rest parameter that holds any additional arguments.

Thus,

const f = async (x, y = doesNotExist) => {};

now transforms to

const f = (x_1, ...args_1) => __awaiter(this, [x_1, ...args_1], void 0, function*(x, y = doesNotExist) {});

This change means that we can no longer rely on the previous mechanism for lexical arguments capturing (i.e., just passing it along as the second parameter to __awaiter), and instead must manually capture a lexical arguments binding.

Thus,

function f() {
  return async (x, y = doesNotExist) => arguments.length;
}

now transforms to

function f() {
  return (x_1, ...args_1) => {
    var arguments_1 = arguments;
    return __awaiter(this, [x_1, ...args_1], void 0, function* (x, y = doesNotExist) {
      return arguments_1.length;
    });
  };
}

Aside from the change to lexical arguments capturing, a function with a trivial parameter list does not undergo this transformation.

Thus,

async function f(x, y) {}

still transforms to

function f(x, y) { return __awaiter(this, void 0, void 0, function*() {}); }

in an effort to limit the complexity of the down-level emit when it is unwarranted.

It should also be noted that this PR applies a similar change to async generators as well.

Fixes #40410

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Nov 2, 2023
@jakebailey
Copy link
Member

Just to understand, is this backwards compatible for tslib helpers? Given they aren't changing, I think yes?

@rbuckton
Copy link
Member Author

rbuckton commented Nov 3, 2023

Just to understand, is this backwards compatible for tslib helpers? Given they aren't changing, I think yes?

That's correct. No change needs to be made to the helpers, so this is backwards compatible with existing tslib versions.

@rbuckton
Copy link
Member Author

rbuckton commented Nov 3, 2023

@typescript-bot perf test

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 3, 2023

Heya @rbuckton, I've started to run the regular perf test suite on this PR at b783f61. You can monitor the build here.

Update: The results are in!

@rbuckton
Copy link
Member Author

rbuckton commented Nov 3, 2023

@typescript-bot test this
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 3, 2023

Heya @rbuckton, I've started to run the diff-based user code test suite on this PR at b783f61. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@rbuckton Here are the results of running the user test suite comparing main and refs/pull/56296/merge:

There were infrastructure failures potentially unrelated to your change:

  • 3 instances of "Package install failed"

Otherwise...

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Compiler

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v18.15.0, x64)
Memory used 295,092k (± 0.01%) 295,122k (± 0.02%) ~ 295,072k 295,199k p=0.297 n=6
Parse Time 2.62s (± 0.45%) 2.62s (± 0.39%) ~ 2.60s 2.63s p=0.801 n=6
Bind Time 0.84s (± 1.17%) 0.83s (± 0.62%) ~ 0.83s 0.84s p=0.417 n=6
Check Time 8.05s (± 0.23%) 8.05s (± 0.19%) ~ 8.03s 8.06s p=0.928 n=6
Emit Time 7.08s (± 0.21%) 7.09s (± 0.40%) ~ 7.06s 7.14s p=0.514 n=6
Total Time 18.59s (± 0.14%) 18.59s (± 0.12%) ~ 18.57s 18.63s p=0.627 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 191,102k (± 0.56%) 190,710k (± 0.02%) ~ 190,678k 190,761k p=0.128 n=6
Parse Time 1.35s (± 1.05%) 1.36s (± 0.60%) ~ 1.35s 1.37s p=0.402 n=6
Bind Time 0.73s (± 0.00%) 0.73s (± 0.00%) ~ 0.73s 0.73s p=1.000 n=6
Check Time 9.13s (± 0.38%) 9.15s (± 0.35%) ~ 9.11s 9.20s p=0.170 n=6
Emit Time 2.63s (± 0.52%) 2.63s (± 0.52%) ~ 2.61s 2.64s p=0.933 n=6
Total Time 13.84s (± 0.35%) 13.87s (± 0.28%) ~ 13.83s 13.92s p=0.145 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,358k (± 0.01%) 347,374k (± 0.01%) ~ 347,353k 347,406k p=0.336 n=6
Parse Time 2.46s (± 0.67%) 2.46s (± 0.21%) ~ 2.45s 2.46s p=0.797 n=6
Bind Time 0.94s (± 0.55%) 0.94s (± 0.00%) ~ 0.94s 0.94s p=0.174 n=6
Check Time 6.94s (± 0.53%) 6.94s (± 0.26%) ~ 6.92s 6.97s p=0.807 n=6
Emit Time 4.05s (± 0.25%) 4.06s (± 0.34%) ~ 4.04s 4.07s p=0.934 n=6
Total Time 14.39s (± 0.29%) 14.39s (± 0.20%) ~ 14.35s 14.43s p=0.568 n=6
TFS - node (v18.15.0, x64)
Memory used 302,599k (± 0.01%) 302,606k (± 0.01%) ~ 302,562k 302,653k p=0.936 n=6
Parse Time 1.99s (± 1.36%) 1.99s (± 1.21%) ~ 1.95s 2.02s p=1.000 n=6
Bind Time 1.00s (± 0.54%) 1.01s (± 0.81%) ~ 1.00s 1.02s p=0.859 n=6
Check Time 6.25s (± 0.34%) 6.26s (± 0.47%) ~ 6.22s 6.30s p=0.808 n=6
Emit Time 3.57s (± 0.31%) 3.58s (± 0.25%) ~ 3.57s 3.59s p=0.149 n=6
Total Time 12.82s (± 0.21%) 12.84s (± 0.33%) ~ 12.79s 12.89s p=0.375 n=6
material-ui - node (v18.15.0, x64)
Memory used 470,531k (± 0.01%) 470,521k (± 0.00%) ~ 470,488k 470,548k p=0.936 n=6
Parse Time 2.58s (± 0.80%) 2.57s (± 0.53%) ~ 2.55s 2.59s p=0.869 n=6
Bind Time 0.99s (± 0.76%) 0.99s (± 0.76%) ~ 0.98s 1.00s p=0.487 n=6
Check Time 16.63s (± 0.33%) 16.62s (± 0.24%) ~ 16.57s 16.67s p=0.936 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.20s (± 0.34%) 20.19s (± 0.20%) ~ 20.13s 20.23s p=0.872 n=6
xstate - node (v18.15.0, x64)
Memory used 512,630k (± 0.01%) 512,684k (± 0.00%) +54k (+ 0.01%) 512,668k 512,703k p=0.016 n=6
Parse Time 3.26s (± 0.36%) 3.27s (± 0.16%) +0.01s (+ 0.36%) 3.27s 3.28s p=0.039 n=6
Bind Time 1.55s (± 0.00%) 1.54s (± 0.54%) ~ 1.54s 1.56s p=0.121 n=6
Check Time 2.85s (± 1.07%) 2.85s (± 0.82%) ~ 2.81s 2.88s p=0.628 n=6
Emit Time 0.08s (± 0.00%) 0.08s (± 0.00%) ~ 0.08s 0.08s p=1.000 n=6
Total Time 7.74s (± 0.41%) 7.74s (± 0.23%) ~ 7.72s 7.77s p=0.870 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

tsserver

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-UnionsTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,371ms (± 0.61%) 2,390ms (± 1.11%) ~ 2,349ms 2,431ms p=0.173 n=6
Req 2 - geterr 5,413ms (± 1.33%) 5,347ms (± 1.10%) ~ 5,294ms 5,460ms p=0.199 n=6
Req 3 - references 326ms (± 0.36%) 328ms (± 1.28%) ~ 323ms 334ms p=0.357 n=6
Req 4 - navto 276ms (± 1.27%) 279ms (± 1.03%) ~ 273ms 281ms p=0.244 n=6
Req 5 - completionInfo count 1,356 (± 0.00%) 1,356 (± 0.00%) ~ 1,356 1,356 p=1.000 n=6
Req 5 - completionInfo 85ms (± 8.46%) 80ms (± 7.30%) ~ 74ms 90ms p=0.203 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,478ms (± 0.96%) 2,489ms (± 0.85%) ~ 2,455ms 2,517ms p=0.748 n=6
Req 2 - geterr 4,112ms (± 1.99%) 4,141ms (± 1.83%) ~ 4,034ms 4,199ms p=0.378 n=6
Req 3 - references 338ms (± 1.51%) 336ms (± 1.57%) ~ 332ms 344ms p=0.459 n=6
Req 4 - navto 282ms (± 0.27%) 282ms (± 0.43%) ~ 280ms 283ms p=0.867 n=6
Req 5 - completionInfo count 1,518 (± 0.00%) 1,518 (± 0.00%) ~ 1,518 1,518 p=1.000 n=6
Req 5 - completionInfo 83ms (± 7.69%) 81ms (± 7.31%) ~ 77ms 89ms p=0.798 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,587ms (± 0.64%) 2,599ms (± 0.48%) ~ 2,584ms 2,620ms p=0.296 n=6
Req 2 - geterr 1,699ms (± 2.59%) 1,691ms (± 1.51%) ~ 1,654ms 1,716ms p=1.000 n=6
Req 3 - references 122ms (± 6.96%) 113ms (± 9.74%) ~ 105ms 128ms p=0.514 n=6
Req 4 - navto 367ms (± 1.88%) 365ms (± 0.22%) ~ 364ms 366ms p=0.282 n=6
Req 5 - completionInfo count 2,073 (± 0.00%) 2,073 (± 0.00%) ~ 2,073 2,073 p=1.000 n=6
Req 5 - completionInfo 307ms (± 1.29%) 305ms (± 1.26%) ~ 299ms 308ms p=0.261 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • CompilerTSServer - node (v18.15.0, x64)
  • Compiler-UnionsTSServer - node (v18.15.0, x64)
  • xstateTSServer - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Startup

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
tsc-startup - node (v18.15.0, x64)
Execution time 152.44ms (± 0.21%) 152.52ms (± 0.18%) +0.08ms (+ 0.05%) 151.48ms 155.52ms p=0.001 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 227.56ms (± 0.17%) 227.40ms (± 0.16%) -0.16ms (- 0.07%) 226.07ms 232.17ms p=0.000 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 228.92ms (± 0.18%) 229.11ms (± 0.19%) +0.19ms (+ 0.08%) 227.16ms 236.33ms p=0.000 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 228.62ms (± 0.15%) 228.86ms (± 0.18%) +0.24ms (+ 0.10%) 227.23ms 234.76ms p=0.000 n=600
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • tsc-startup - node (v18.15.0, x64)
  • tsserver-startup - node (v18.15.0, x64)
  • tsserverlibrary-startup - node (v18.15.0, x64)
  • typescript-startup - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Behavior change when lowering async function which throws during argument evaluation
3 participants