Reduce unnecessary Awaits for nullish values in blocks containing `await using` #219

rbuckton · 2024-03-22T17:25:02Z

This is an alternative to #216 and is based on another fix in #218.

Per #196 (comment), this reduces the number of Awaits in blocks containing await using and in AsyncDisposableStack by collapsing the Awaits introduced for null or undefined resources to a single Await.

As currently specified, the following code results in three Awaits when only one is necessary:

{
  await using x = null, y = undefined, z = { [Symbol.dispose]() {} };
}

This was also the case in an AsyncDisposableStack containing synchronous disposables:

const stack = new AsyncDisposableStack();
stack.use(null);
stack.use(undefined);
stack.use({ [Symbol.dispose]() { } });
await stack.disposeAsync(); // Awaits four times, including the explicit await on this line.

This PR changes this behavior such that we only need an implicit Await in the following cases:

Disposal of an async resource¹ or a non-nullish async-from-sync resource², so long as it does not throw synchronously.
When transitioning from a null or undefined async-from-sync resource to an async resource, non-nullish async-from-sync resource, or sync resource³.
When transitioning from a null or undeifned async-from-sync resource is the last disposal performed for a scope.

Example 1

try {
  using A = syncRes;
  await using B = null, C = undefined;
  HAPPENS_BEFORE
} catch { }
HAPPENS_AFTER

C is undefined so no disposal happens, but we track that an Await must occur. [+0 turns]
B is null so no disposal happens, but we track that an Await must occur. [+0 turns]
A is neither null nor undefined and we have tracked that an Await must occur, so we Await(undefined). [+1 turn]
A[@@dispose]() is invoked in the following turn. [+0 turns]
HAPPENS_AFTER happens in the same turn as A[@@dispose](), but a turn after HAPPENS_BEFORE.

Example 2

try {
  using A = syncRes;
  await using B = null, C = asyncRes;
  HAPPENS_BEFORE
} catch { }
HAPPENS_AFTER

C is neither null nor undefined so we invoke its @@asyncDispose method. If it completes normally we Await the result [+1 turns], but if it throws we continue on the same turn [+0 turns].
B is null so no disposal happens, but we track that an Await must occur. [+0 turns]
A is neither null nor undefined and we have tracked that an Await must occur, so we Await(undefined). [+1 turns]
A[@@dispose]() is invoked in the following turn. [+0 turns]
HAPPENS_AFTER happens in the same turn as A[@@dispose](), but between 1-2 turns after HAPPENS_BEFORE.

Example 3

try {
  using A = syncRes;
  await using B = asyncRes, C = null;
  HAPPENS_BEFORE
} catch { }
HAPPENS_AFTER

C is null so no disposal happens, but we track that an Await must occur. [+0 turns]
B is neither null nor undefined and we have tracked that an Await must occur, so we Await(undefined). [+1 turns]
Next, we invoke B's @@asyncDispose method. If it completes normally we Await the result [+1 turns], but if it throws we continue on the same turn [+0 turns].
A is neither null nor undefined so we invoke A[@@dispose]() either on the turn after B's disposal (if it was normal), or on the same turn (if it threw). [+0 turns]
HAPPENS_AFTER happens in the same turn as A[@@dispose](), but between 1-2 turns after HAPPENS_BEFORE.

It is important to note that a synchronous throw completion from a call to @@asyncDispose will result in the Await being passed over, but this is consistent with for await and AsyncIterator.next() behavior. Following #218, this is unlikely to occur for a @@dispose method as it will be automatically wrapped in a new PromiseCapability, though it is still possible to throw synchronously if Promise.prototype.then is patched to throw an exception synchronously.

This PR also removes unnecessary Awaits for null/undefined resources in AsyncDisposableStack.prototype.disposeAsync().

The PR against ecma262 is being tracked in rbuckton/ecma262#10

Fixes #196
Fixes #208

An async resource is an object with an @@asyncDispose method initialized in an await using declaration. ↩
An sync-from-sync resource is either null, undefined, or an object with a @@dispose method (but not an @@asyncDispose method) initialized in an await using declaration. ↩
A sync resource is either null, undefined, or an object with a @@dispose method initialized in a using declaration. ↩

…sing`

…ait using`

github-actions · 2024-03-22T17:25:28Z

A preview of this PR can be found at https://tc39.es/proposal-explicit-resource-management/pr/219.

bakkot · 2024-03-22T18:47:39Z

In example 2, if C[@@asyncDispose]() completes normally and so is await'd, I wouldn't expect another await to be necessary for B. We've already done an await, and we haven't done anything after that, so waiting another tick doesn't really do anything: that is, having that additional await undefined doesn't take any code that was previously going to run within a single turn and split it across two turns.

Did you mean to say that in step 3 there is only an await if C[@@asyncDispose]() throws?

rbuckton · 2024-04-01T16:28:14Z

In example 2, if C[@@asyncDispose]() completes normally and so is await'd, I wouldn't expect another await to be necessary for B. We've already done an await, and we haven't done anything after that, so waiting another tick doesn't really do anything: that is, having that additional await undefined doesn't take any code that was previously going to run within a single turn and split it across two turns.

I agree, in principle. My concern is this: should C[@@asyncDispose]() throw synchronously and we do not introduce a synthetic Await for B, then there would be no Await at all between HAPPENS_BEFORE and HAPPENS_AFTER, despite B being "disposed" after C. We could perhaps elide the Await if we are certain that C[@@asyncDispose]() did not throw synchronously.

Did you mean to say that in step 3 there is only an await if C[@@asyncDispose]() throws?

Example 3, steps 2 and 3:

B is null so no disposal happens, but we track that an Await must occur. [+0 turns]

A is neither null nor undefined and we have tracked that an Await must occur, so we Await(undefined). [+1 turns]

Step 3 performs an Await if one or more null/undefined values were encountered immediately before since we are switching from an async resource declared in an await using back to a sync resource declared in a using. At the moment, this occurs regardless as to whether C[@@asyncDispose]() throws in Step 1.

Essentially, this ensures that the following code will Await at least once, but up to twice depending on whether asyncNonNull throws synchronously from its @@asyncDispose method:

{
  using A = syncNonNull;
  await using B = null, C = null, D = undefined, E = asyncNonNull;
}

As we unwind disposal, we do the following:

Async disposal of E (0-1 Awaits depending on whether it throws synchronously)
Deterministic collapse of D, C, and B, indicates Await is needed
transition from await using to using, so we Await if we indicated it was needed in the previous step.
Sync disposal of A.

rbuckton · 2024-04-10T18:14:40Z

Consensus in plenary was to collapse all Awaits for null/undefined, not just contiguous runs of null/undefined. There was also tentative consensus to enforce an Await if there are a run of null/undefined resources prior to an async resource that throws synchronously, but that requires an update to this PR and further discussion.

rbuckton · 2024-04-10T18:50:19Z

@nicolo-ribaudo, you asked to review the proposed changes here before this could reach consensus. I'd appreciate if you could review these changes as of the most recent commits.

The intent behind these changes is as follows:

null/undefined in an await using does not immediately trigger an Await during cleanup, but instead record that an Await is needed.
When non-null/non-undefined resource is disposed, so long as it does not throw synchronously, we Await the result of disposal and record that an Await has occurred.
Any time we transition from an async resource (i.e., one defined by await using) to a sync resource (i.e., one defined by using), if an Await is needed and has not occurred, we Await.
If DisposeResource exits and the last resource was an async resource (i.e., one defined by await using), if an Await is needed and has not occurred, we Await.
Does not distinguish between await using x = ?, y = ?; and await using x = ?; await using y = ?;

This has the following effects:

Given,

{
    await using x = null, y = null;
}

we record an Await is needed when disposing y and then x. As we exit DisposeResources, we see that an Await is needed but has not occurred, so we perform an Await. (Await count = 1)

Given,

{
  await using x = null, y = asyncDisposable, z = null;
}

we record an Await is needed when disposing z, we record an Await has occurred when disposing y, and record that an Await is neded when disposing x. As we exit DisposeResources, we see that an Await is needed and has occurred, so we do not Await. (Await count = 1)

Given,

{
  await using x = null, y = syncThrowInAsyncDispose, z = null;
}

we record an Await is needed when disposing z, disposing y throws synchronously is not Awaited, and record that an Await is neded when disposing x. As we exit DisposeResources, we see that an Await is needed and has not occurred, so we perform an Await. (Await count = 1)

Given,

{
  await using x = null, y = null, z = asyncDisposable;
}

we record an Await is needed when disposing z, we record an Await has occurred when disposing y, and record that an Await is neded when disposing x. As we exit DisposeResources, we see that an Await is needed and has occurred, so we do not Await. (Await count = 1)

Given,

{
  using a = syncDisposable;
  await using x = null, y = null;
}

we record an Await is needed when disposing y and then x. As we transition from the async resource x to the sync resource a, we see that an Await is needed but has not occurred, so we perform an Await. (Await count = 1)

Given,

{
  using a = syncDisposable;
  await using x = null, y = asyncDisposable, z = null;
}

we record an Await is needed when disposing z, we record an Await has occurred when disposing y, and record that an Await is neded when disposing x. As we transition from the async resource x to the sync resource a, we see that an Await is needed and has occurred, so we do not Await. (Await count = 1)

Given,

{
  using a = syncDisposable;
  await using x = null, y = syncThrowInAsyncDispose, z = null;
}

we record an Await is needed when disposing z, disposing y throws synchronously is not Awaited, and record that an Await is neded when disposing x. As we transition from the async resource x to the sync resource a, we see that an Await is needed and has not occurred, so we perform an Await. (Await count = 1)

Given,

{
  using a = syncDisposable;
  await using x = null, y = null, z = asyncDisposable;
}

we record an Await is needed when disposing z, we record an Await has occurred when disposing y, and record that an Await is neded when disposing x. As we transition from the async resource x to the sync resource a, we see that an Await is needed and has occurred, so we do not Await. (Await count = 1)

rbuckton · 2024-04-10T18:52:56Z

I'd also appreciate additional reviews from @syg, @waldemarhorwat, @michaelficarra, and @bakkot.

spec.emu

rbuckton · 2024-04-10T19:22:07Z

Also, if @mhofman and @erights could take a look, I'd appreciate it.

erights · 2024-04-10T19:52:56Z

How can I see a rendered form of this?

rbuckton · 2024-04-10T19:56:19Z

How can I see a rendered form of this?

The first comment on this PR contains a link to the rendered spec text.

The specific section in question is here: https://tc39.es/proposal-explicit-resource-management/pr/219/#sec-disposeresources

nicolo-ribaudo

This looks good to me.

It's not explicit from the examples, but awaits are also collapsed across subsequent separate await using declarations right?

await using x = null;
await using y = null;

Keeps being the same as

await using x = null, y = null;

spec.emu

rbuckton · 2024-04-10T22:10:09Z

It's not explicit from the examples, but awaits are also collapsed across subsequent separate await using declarations right?

Correct. I called this out:

Does not distinguish between await using x = ?, y = ?; and await using x = ?; await using y = ?;

mhofman · 2024-04-11T06:08:46Z

I agree, in principle. My concern is this: should C[@@asyncDispose]() throw synchronously and we do not introduce a synthetic Await for B, then there would be no Await at all between HAPPENS_BEFORE and HAPPENS_AFTER, despite B being "disposed" after C. We could perhaps elide the Await if we are certain that C[@@asyncDispose]() did not throw synchronously.

I keep wondering if it wouldn't be simpler to always Await for an @@asyncDispose that throws synchronously. That way there would be no difference whether there was a null async resource disposed before or not, and you could just always collapse null async resources if any non-null async resource were added to the stack.

rbuckton · 2024-04-11T14:09:58Z

I agree, in principle. My concern is this: should C[@@asyncDispose]() throw synchronously and we do not introduce a synthetic Await for B, then there would be no Await at all between HAPPENS_BEFORE and HAPPENS_AFTER, despite B being "disposed" after C. We could perhaps elide the Await if we are certain that C[@@asyncDispose]() did not throw synchronously.

I keep wondering if it wouldn't be simpler to always Await for an @@asyncDispose that throws synchronously. That way there would be no difference whether there was a null async resource disposed before or not, and you could just always collapse null async resources if any non-null async resource were added to the stack.

That would not simplify the algorithm, it would make it more complex. I also see no reason to break with how for..of handles [Symbol.asyncIterator](), or how await f() works in general, when either function happens to throw synchronously.

rbuckton · 2024-06-15T19:02:54Z

This reached consensus during the June 2024 TC39 plenary session.

rbuckton · 2024-06-18T21:34:43Z

It looks like there may be a case in the algorithm that still introduces an extraneous Await in the case of:

{
  await using x = null;
  using y = res;
  await using z = null;
}

When we dispose z, Step 3.f.ii sets needsAwait to true. Then when we dispose y synchronously, Step 3.d.i performs an Await and Step 3.d.ii sets needsAwait to false. Finally, when we dispose x, Step 3.f.ii sets needsAwait to true again.

I expect we still only really need a single Await in this case, in which case Step 3.d.ii should instead read "Set hasAwaited to true".

This CL reduces unnecessary Awaits for nullish values in blocks containing `await using` and in `AsyncDisposableStack` tc39/proposal-explicit-resource-management#219 Bug: 42203814 Change-Id: Ia94755bd19a217512d308b5b4f524471b8321d63 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5819755 Reviewed-by: Shu-yu Guo <[email protected]> Commit-Queue: Rezvan Mahdavi Hezaveh <[email protected]> Cr-Commit-Position: refs/heads/main@{#95982}

This reverts commit f054729. Reason for revert: https://issues.chromium.org/u/1/issues/365060988 Original change's description: > [explicit-resource-management] Avoid unnecessary awaits > > This CL reduces unnecessary Awaits for nullish values in blocks containing `await using` and in `AsyncDisposableStack` > tc39/proposal-explicit-resource-management#219 > > > Bug: 42203814 > Change-Id: Ia94755bd19a217512d308b5b4f524471b8321d63 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5819755 > Reviewed-by: Shu-yu Guo <[email protected]> > Commit-Queue: Rezvan Mahdavi Hezaveh <[email protected]> > Cr-Commit-Position: refs/heads/main@{#95982} Bug: 42203814 Change-Id: I932341e117fd02299bda224ab5ff694372006ac0 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5840443 Bot-Commit: Rubber Stamper <[email protected]> Owners-Override: Rezvan Mahdavi Hezaveh <[email protected]> Commit-Queue: Rezvan Mahdavi Hezaveh <[email protected]> Cr-Commit-Position: refs/heads/main@{#95998}

This is a reland of commit f054729 `performPromiseThen` was mistakenly attached to the promise.protorype previously. In this cl we fix that issue. Original change's description: > [explicit-resource-management] Avoid unnecessary awaits > > This CL reduces unnecessary Awaits for nullish values in blocks containing `await using` and in `AsyncDisposableStack` > tc39/proposal-explicit-resource-management#219 > > > Bug: 42203814 > Change-Id: Ia94755bd19a217512d308b5b4f524471b8321d63 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5819755 > Reviewed-by: Shu-yu Guo <[email protected]> > Commit-Queue: Rezvan Mahdavi Hezaveh <[email protected]> > Cr-Commit-Position: refs/heads/main@{#95982} Bug: 42203814 Change-Id: Ice3c6438f8c8230c9edd0853a225feb8c6ed592d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5842565 Reviewed-by: Shu-yu Guo <[email protected]> Commit-Queue: Rezvan Mahdavi Hezaveh <[email protected]> Cr-Commit-Position: refs/heads/main@{#96002}

rbuckton added 2 commits March 22, 2024 12:41

Add PromiseCapability wrapper around sync dispose used in an `await u…

f09a8b9

…sing`

Reduce unnecessary Awaits for nullish values in blocks containing `aw…

be91331

…ait using`

rbuckton requested review from bakkot, mhofman, michaelficarra and syg March 22, 2024 17:25

rbuckton mentioned this pull request Mar 22, 2024

Reduce unnecessary Awaits in blocks containing await using #216

Closed

rbuckton added needs-consensus A pull request that needs consensus at TC39 plenary normative Indicates a normative change to the specification labels Mar 22, 2024

rbuckton added the enhancement New feature or request label Mar 22, 2024

nicolo-ribaudo mentioned this pull request Apr 9, 2024

Await nullish async disposable babel/babel#16408

Merged

rbuckton added 2 commits April 10, 2024 14:16

Merge branch 'main' into deterministic-collapse-of-await-nullish

2774af9

Update DisposeResources to match tentative consensus

897cc3d

Match spec text for each

68d3d2f

rbuckton mentioned this pull request Apr 10, 2024

Reduce unnecessary Awaits for nullish values in blocks containing await using rbuckton/ecma262#10

Merged

Fix formatting

5131891

ckknight reviewed Apr 10, 2024

View reviewed changes

spec.emu Outdated Show resolved Hide resolved

Remove extraneous assignment

976d301

rbuckton requested review from erights, nicolo-ribaudo and waldemarhorwat April 10, 2024 19:22

nicolo-ribaudo reviewed Apr 10, 2024

View reviewed changes

spec.emu Outdated Show resolved Hide resolved

fix indent

460d0bc

nicolo-ribaudo mentioned this pull request May 5, 2024

Improve async dispose fallback babel/babel#16409

Closed

rbuckton added the has-consensus Indicates a pull request reached consensus at TC39 plenary. label Jun 13, 2024

rbuckton mentioned this pull request Jun 15, 2024

Explicit Resource Management normative changes for April 2024 microsoft/TypeScript#58876

Closed

rbuckton merged commit e37aa9a into main Jun 15, 2024

rbuckton deleted the deterministic-collapse-of-await-nullish branch June 15, 2024 19:08

rbuckton mentioned this pull request Jun 15, 2024

Write test262 tests for deterministic collapse of Await for nullish values #233

Open

rbuckton mentioned this pull request Jun 19, 2024

Implement deterministic collapse of 'await' in 'await using' microsoft/TypeScript#58929

Merged

nicolo-ribaudo mentioned this pull request Jun 20, 2024

await using normative updates babel/babel#16537

Merged

domenic mentioned this pull request Nov 20, 2024

Clarify stage 3 entrance criteria whatwg/meta#336

Open

Reduce unnecessary Awaits for nullish values in blocks containing await using #219

Reduce unnecessary Awaits for nullish values in blocks containing await using #219

Uh oh!

Conversation

rbuckton commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

github-actions bot commented Mar 22, 2024

Uh oh!

bakkot commented Mar 22, 2024

Uh oh!

rbuckton commented Apr 1, 2024

Uh oh!

rbuckton commented Apr 10, 2024

Uh oh!

rbuckton commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbuckton commented Apr 10, 2024

Uh oh!

Uh oh!

rbuckton commented Apr 10, 2024

Uh oh!

erights commented Apr 10, 2024

Uh oh!

rbuckton commented Apr 10, 2024

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rbuckton commented Apr 10, 2024

Uh oh!

mhofman commented Apr 11, 2024

Uh oh!

rbuckton commented Apr 11, 2024

Uh oh!

rbuckton commented Jun 15, 2024

Uh oh!

rbuckton commented Jun 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Reduce unnecessary Awaits for nullish values in blocks containing `await using` #219

Reduce unnecessary Awaits for nullish values in blocks containing `await using` #219

rbuckton commented Mar 22, 2024 •

edited

Loading

rbuckton commented Apr 10, 2024 •

edited

Loading