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

Normative: Reduce the number of ticks in async/await #1250

Open
wants to merge 3 commits into
base: master
from

Conversation

@MayaLekova
Contributor

MayaLekova commented Jun 28, 2018

JavaScript programmers may expect that the following
two programs are largely similar in terms of how they perform with
respect to the ECMAScript job queue (if inside of an async function):

promise.then(f); f(await promise);

However, if promise is a built-in promise, then these two code fragments
will differ in the number of iterations through the job queue are taken: because
await always wraps a Promise with another Promise, there are three job
queue items enqueued and dequeued before calling f in the await example,
whereas there is just a single item for the then usage.

In discussions with JavaScript programmers, the number of job queue items
in the current semantics turns out to be surprising. For example, the difference
has become more visible in conjunction with new V8 features for Promise
visibility and debugging, which are sometimes used in Node.js.

This patch changes the semantics of await to reduce the number of
job queue turns that are taken in the common await Promise case by replacing
the unconditional wrapping with a call to PromiseResolve. Analogous changes
are made in async iterators.

The patch preserves key design goals of async/await:

  • Job queue processing remains deterministic, including both ordering and the number of jobs enqueued (which is observable by interspersing other jobs)
  • Userland Promise libraries with "then" methods ("foreign thenables") are usable within await, and trigger a turn of the job queue
  • Non-Promises can be awaited, and this takes a turn of the native job queue (as do all usages of await)

Reducing the number of job queue turns also improves performance
on multiple highly optimized async/await implementations. In a draft
implementation of this proposal in V8 behind a flag [1]:

  • The doxbee async/await performance benchmark [2] improved with 48%
  • The fibonacci async/await performance benchmark [3] improved with 23%
  • The Hapi throughput benchmark [4] improved with 50% (when async hooks are enabled) and with 20% (when async hooks are disabled)

[1] https://chromium-review.googlesource.com/c/v8/v8/+/1106977
[2] https://github.com/bmeurer/promise-performance-tests/blob/master/lib/doxbee-async.js
[3] https://github.com/bmeurer/promise-performance-tests/blob/master/lib/fibonacci-async.js
[4] https://github.com/fastify/benchmarks

@MayaLekova

This comment has been minimized.

Contributor

MayaLekova commented Jun 28, 2018

Please note a possible issue of this change noted by @zenparsing in the original commit. Any feedback on what's the importance of this issue and suggestions how it can be worked around are more than welcome!

@ljharb

This comment has been minimized.

Member

ljharb commented Jun 28, 2018

I like that this change leaves await robust, and uses the existing PromiseResolve mechanism. I think this also relates to #1118.

@domenic

LGTM, and seems like a great consistency improvement to use PromiseResolve() more. (Compare to Promise.prototype.finally.)

@zenparsing

Thanks for the detailed write-up and benchmarks!

spec.html Outdated
</emu-alg>
<p>mean the same thing as:</p>
<emu-alg>
1. Let _asyncContext_ be the running execution context.
1. Let _promiseCapability_ be ! NewPromiseCapability(%Promise%).
1. Perform ! Call(_promiseCapability_.[[Resolve]], *undefined*, &laquo; _promise_ &raquo;).
1. Let _promise_ be ? PromiseResolve(&laquo; _value_ &raquo;).

This comment has been minimized.

@zenparsing

zenparsing Jun 28, 2018

Member

PromiseResolve currently takes a constructor in the first argument position. I suppose that we'll need to provide it with %Promise%.

This comment has been minimized.

@benjamn

benjamn Jun 28, 2018

Member

What are the chances we could tolerate any Promise subclass here? In other words, pass value.constructor as the first argument to PromiseResolve if value instanceof %Promise% (pardon my hand waving; there may be a better spec notation for this), thereby giving value a chance to be returned as-is by PromiseResolve.

I realize the async function can only return instances of the original, native Promise constructor, but it seems like a good thing for await to treat Promise subclasses the same way it treats ordinary Promises.

Unless part of the deal with subclassing Promise is that you're guaranteed your .prototype.then method will be called by await?

I don't have strong feelings either way, but it seems like a discussion worth having.

This comment has been minimized.

@zenparsing

zenparsing Jun 28, 2018

Member

A Promise subclass would (presumably) fail the SameValue test on 2.b of PromiseResolve, and we would then fallback to creating a new promise (exactly as we currently do).

This comment has been minimized.

@ljharb

ljharb Jun 28, 2018

Member

I believe it was an intentional design decision, discussed in committee, not to support subclassed Promises in await.

This comment has been minimized.

@ljharb

ljharb Jun 28, 2018

Member

(Specifically, using .constructor here would mean that any object coercible value in the language would pass the test in PromiseResolve, which would break the semantics)

This comment has been minimized.

@benjamn

benjamn Jun 28, 2018

Member

So it's exactly as if await calls Promise.resolve, which passes this as the constructor argument to PromiseResolve. Although this could be a subclass of Promise in arbitrary user code, it would always be %Promise% in an async function.

This comment has been minimized.

@benjamn

benjamn Jun 28, 2018

Member

(Specifically, using .constructor here would mean that any object coercible value in the language would pass the test in PromiseResolve, which would break the semantics)

That's why we would have to perform an external check that .constructor is a Promise subclass before calling PromiseResolve(_value_.constructor, _value_), or perhaps modify PromiseResolve to enforce the subclass relationship.

How do folks feel about introducing a single-argument PromiseResolve variant that does the right thing (whatever we decide that is), so that this code (i.e. Let _promise_ be ? PromiseResolve(&laquo; _value_ &raquo;)) works as written?

This comment has been minimized.

@zenparsing

zenparsing Jun 28, 2018

Member

I would probably prefer the explicit constructor argument to PromiseResolve for now. And I believe it should be %Promise% in any case: subclasses should go through the slower but semantically more flexible path of calling their overridden "then" method from a new tick.

This comment has been minimized.

@ljharb

ljharb Jun 28, 2018

Member

I also think it's useful to keep the explicit %Promise%.

This comment has been minimized.

@MayaLekova

MayaLekova Jun 29, 2018

Contributor

Thanks for the discussion!
Added %Promise% as a parameter in a follow-up commit.

@zenparsing

This comment has been minimized.

Member

zenparsing commented Jun 28, 2018

I'm curious about what the group thinks about the issue that @MayaLekova referenced upthread: with these new semantics "then" will not be called on a native, non-subclassed promise when awaited (because we are doing PerformPromiseThen directly):

async function f() {
  let promise = Promise.resolve(1);
  promise.then = function(...args) {
    print('then called');
    return Promise.prototype.then.apply(this, args);
  };
  let result = await promise;
  return result;
}

f().then(v => print(v));

Currently, we log "then called", but I think after the change we will not. Is this edge case a problem that we should worry about?

@ljharb

This comment has been minimized.

Member

ljharb commented Jun 28, 2018

Since it’s only observable if you monkeypatch the then method, and since Promise.resolve behaves this way already, and since await is often described as conceptually wrapping its operand in Promise.resolve, it seems like the proper choice to me - and anyone relying on this behavior already can’t necessarily rely on it happening.

@benjamn

This comment was marked as resolved.

Member

benjamn commented Jun 28, 2018

Will this behave differently for thenables that are not Promises? Presumably .then would have to be called by await for thenable objects?

Edit: hiding my own comment after reading the PR

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 28, 2018

@ljharb You're right, monkey-patching Promise.prototype.then is already broken in the sense that .then doesn't get called by await reliably enough to do anything useful, such as automatically propagating some sort of async context data.

An alternative approach is to wrap await expressions within the async function body (or use a transform that does the wrapping automatically). For example, await <expr> could be transformed to captureContext()(await <expr>), where captureContext is something like:

function captureContext() {
  const context = getCurrentContext();
  return function restoreContext(value) {
    setCurrentContext(context);
    return value;
  };
}

I hope we (TC39) can keep entertaining potential solutions for this kind of use case, such as Node's AsyncResource API, but I very much agree that wrapping Promise.prototype.then is not the way to get there, so it's fine that this PR slightly interferes with that strategy.

@ljharb ljharb added the needs tests label Jun 29, 2018

@MayaLekova MayaLekova force-pushed the MayaLekova:optimize-await branch from 33b6432 to 5ae98c0 Jul 12, 2018

@MayaLekova

This comment has been minimized.

Contributor

MayaLekova commented Jul 12, 2018

Rebased the commits on top of master.

Here are the links to the original changes, for the purpose of comment tracking:
Version 1
Version 2

Big thanks to @littledan for wording the commit message!

@tolmasky

This comment was marked as resolved.

tolmasky commented Jul 25, 2018

Am I understanding correctly that in a subclassed Promise with a custom then, you can expect the custom then to be called with await promiseSubclassInstance?

For some context, I've considered this as a way to allow chaining methods to serve "double duty" as awaitable or chainable:

const frame = await page.frame(0);
await page.frame(0).click("selector");

Here, frame is a custom "LazyPromiseSubclass" that does not actually commit to looking for the frame until then is called (either through await, explict then, or eventually through something like Promise.all). That way, on the second line, the work is never actually done (and immediately lost) to grab the frame.

EDIT:

LazyPromise might be defined as such:

class LazyPromise extends Promise
{
    constructor(f)
    {
        this._f = f;
        this._internalPromise = null;
    }

    _committed() 
    {
        if (!this._internalPromise)
            this._internalPromise = new Promise(this._f);

        return this._internalPromise;
    }

    then(...args) { this._committed().then(...args); }
    catch(...args) { this._commited().catch(...args); }
} 
@zenparsing

This comment was marked as resolved.

Member

zenparsing commented Jul 25, 2018

@tolmasky Yes, that's right. According to this PR, if the p in await p is not a built-in Promise whose "constructor" property is the built-in Promise, await would execute "then" on p.

MayaLekova added some commits Jun 8, 2018

Normative: Reduce the number of ticks in async/await
JavaScript programmers may expect that the following
two programs are largely similar in terms of how they perform with
respect to the ECMAScript job queue (if inside of an async function):

promise.then(f);                f(await promise);

However, if `promise` is a built-in promise, then these two code fragments
will differ in the number of iterations through the job queue are taken: because
`await` always wraps a Promise with another Promise, there are three job
queue items enqueued and dequeued before calling `f` in the `await` example,
whereas there is just a single item for the `then` usage.

In discussions with JavaScript programmers, the number of job queue items
in the current semantics turns out to be surprising. For example, the difference
has become more visible in conjunction with new V8 features for Promise
visibility and debugging, which are sometimes used in Node.js.

This patch changes the semantics of await to reduce the number of
job queue turns that are taken in the common `await` Promise case by replacing
the unconditional wrapping with a call to PromiseResolve. Analogous changes
are made in async iterators.

The patch preserves key design goals of async/await:
- Job queue processing remains deterministic, including both ordering and the number of jobs enqueued (which is observable by interspersing other jobs)
- Userland Promise libraries with "then" methods ("foreign thenables") are usable within `await`, and trigger a turn of the job queue
- Non-Promises can be awaited, and this takes a turn of the native job queue (as do all usages of `await`)

Reducing the number of job queue turns also improves performance
on multiple highly optimized async/await implementations. In a draft
implementation of this proposal in V8 behind a flag [1]:
- The doxbee async/await performance benchmark [2] improved with 48%
- The fibonacci async/await performance benchmark [3] improved with 23%
- The Hapi throughput benchmark [4] improved with 50% (when async hooks are enabled) and with 20% (when async hooks are disabled)

[1] https://chromium-review.googlesource.com/c/v8/v8/+/1106977
[2] https://github.com/bmeurer/promise-performance-tests/blob/master/lib/doxbee-async.js
[3] https://github.com/bmeurer/promise-performance-tests/blob/master/lib/fibonacci-async.js
[4] https://github.com/fastify/benchmarks

@ljharb ljharb removed the needs consensus label Sep 25, 2018

@ljharb ljharb requested a review from tc39/ecma262-editors Sep 25, 2018

@zenparsing

This comment has been minimized.

Member

zenparsing commented Sep 26, 2018

@MayaLekova This change achieved consensus at the September meeting. Thank you!

@MayaLekova

This comment has been minimized.

Contributor

MayaLekova commented Sep 26, 2018

@zenparsing Thank you a lot for presenting it! How should we proceed with merging the change?

@ljharb

This comment has been minimized.

Member

ljharb commented Sep 26, 2018

@MayaLekova the PR still needs test262 tests; once those are written and merged (or at least, an approved PR), this will be eligible for merging.

@gsathya

This comment has been minimized.

Member

gsathya commented Sep 27, 2018

How should we proceed with merging the change?

@MayaLekova Apart from what @ljharb said, we'd also need to wait until there's implementation feedback confirming that this change is web compatible.

@littledan

This comment has been minimized.

Member

littledan commented Sep 30, 2018

About web compatibility, I am not so worried about this change, given the experience with Edge and the lack of visible complaints in the upgrade from Babel to native async/await. What additional sort of evidence are you looking for, @gsathya?

@hax

This comment has been minimized.

hax commented Sep 30, 2018

I believe this change will affect order of async operations. I used to see questions about such issue, for example: https://www.zhihu.com/question/268007969 (in Chinese). But I don't think it have web compatibility issue, because old versions of Node/Chrome already use such semantic, and the older versions of Node, browsers, promise polyfills never have consistent async order, see old issue: promises-aplus/promises-spec#183 . So I guess real world codes never rely on that.

@gsathya

This comment has been minimized.

Member

gsathya commented Oct 1, 2018

What additional sort of evidence are you looking for, @gsathya?

I don't think there's enough evidence to show that there isn't going to be breakage. I don't consider upgrading from Babel (fast path) to native (slow path) as enough evidence that the reverse is fine too.

When Maya implemented this in V8, we had several tests breaking inadvertently (because they relied on the ordering) which gives me some concern.

I don't see any reason to prematurely merge this into the spec and then running into web compat problems later, especially when we're so close to having two shipping implementations.

MayaLekova added a commit to MayaLekova/test262 that referenced this pull request Oct 8, 2018

AsyncFunction: Add tests ensuring the new 1-tick await behaviour
This commit adds 3 tests ensuring the optimized behaviour of await
(see tc39/ecma262#1250) in the following cases:
- async functions
- async generator functions
- yielding from async generator functions

MayaLekova added a commit to MayaLekova/test262 that referenced this pull request Oct 8, 2018

AsyncFunction: Add tests ensuring the new 1-tick await behaviour
This commit adds 3 tests ensuring the optimized behaviour of await
(see tc39/ecma262#1250) in the following cases:
- async functions
- async generator functions
- yielding from async generator functions

MayaLekova added a commit to MayaLekova/test262 that referenced this pull request Oct 8, 2018

AsyncFunction: Add tests ensuring the new 1-tick await behaviour
This commit adds 3 tests ensuring the optimized behaviour of await
(see tc39/ecma262#1250) in the following cases:
- async functions
- yielding from async generator functions
- for-await-of loops
@littledan

This comment has been minimized.

Member

littledan commented Oct 8, 2018

OK, as long as the delay in merging the patch isn't blocking implementations from collecting further evidence, @gsathya's plan sounds good to me.

MayaLekova added a commit to MayaLekova/test262 that referenced this pull request Oct 9, 2018

AsyncFunction: Add tests ensuring the monkey-patched promises behaviour
This commit adds 2 more tests ensuring the optimized behaviour of await
(see tc39/ecma262#1250) in the following cases:
- awaiting on a native promise with monkey-patched "then"
- awaiting on a non-native promise (a "thenable" object)

devsnek added a commit to devsnek/engine262 that referenced this pull request Oct 13, 2018

@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Oct 14, 2018

Should the new semantics also be applied to the return value of async functions?

For example, the program

const p = Promise.resolve();
(async () => { await p; print('after:await'); })();

p
  .then(() => print('tick'))
  .then(() => print('tick'));

Prints "tick tick after:await" today, but soon, it will print "after:await tick tick" instead. Compare with the program

const p = Promise.resolve();
(async () => p)().then(() => print('after:return'));

p
  .then(() => print('tick'))
  .then(() => print('tick'));

Today, that prints "tick tick after:return", and this patch doesn't change that. For consistency, ought it to print "tick after:return tick" instead? If so, would a similar change be appropriate for YieldExpressions in async generators?

@hax

This comment has been minimized.

hax commented Oct 15, 2018

@jugglinmike

For consistency, ought it to print "tick after:return tick" instead?

Old v8 (node 8, 9) output "after:return tick tick".
Current v8 (node 10) output "tick tick after:return"

😂 Oh, async order always make me confused!

@Kovensky

This comment has been minimized.

Kovensky commented Oct 15, 2018

The first program does read to me like the correct output should be after:return tick tick.

I don't understand, though why the second one doesn't print tick after:return tick.

MayaLekova added a commit to MayaLekova/test262 that referenced this pull request Oct 15, 2018

AsyncFunction: Add tests ensuring the non-native promises behaviour
This commit adds 1 more tests ensuring the optimized behaviour of await
(see tc39/ecma262#1250) in the following cases:
- awaiting on a non-promise, non-thenable object

It also renames the previous test for non-promise (a "thenable" object)
to distinguish from the new case.

The commit adds checks for proper await/promises interleaving in the
aforementioned cases and includes a small code clean-up.

leobalter added a commit to tc39/test262 that referenced this pull request Oct 17, 2018

AsyncFunction: Add tests ensuring the new 1-tick await behaviour (#1843)
* AsyncFunction: Add tests ensuring the new 1-tick await behaviour

This commit adds 3 tests ensuring the optimized behaviour of await
(see tc39/ecma262#1250) in the following cases:
- async functions
- yielding from async generator functions
- for-await-of loops

* AsyncFunction: Add tests ensuring the monkey-patched promises behaviour

This commit adds 2 more tests ensuring the optimized behaviour of await
(see tc39/ecma262#1250) in the following cases:
- awaiting on a native promise with monkey-patched "then"
- awaiting on a non-native promise (a "thenable" object)

* AsyncFunction: Add tests ensuring the non-native promises behaviour

This commit adds 1 more tests ensuring the optimized behaviour of await
(see tc39/ecma262#1250) in the following cases:
- awaiting on a non-promise, non-thenable object

It also renames the previous test for non-promise (a "thenable" object)
to distinguish from the new case.

The commit adds checks for proper await/promises interleaving in the
aforementioned cases and includes a small code clean-up.

* AsyncFunction: Refactor tests ensuring the new 1-tick await behaviour

Gather all the tests to their appropriate folder and update copyright header.

@ljharb ljharb added has tests and removed needs tests labels Oct 17, 2018

@MayaLekova

This comment has been minimized.

Contributor

MayaLekova commented Oct 18, 2018

@jugglinmike Thank you for raising this question!

This PR already takes care for async functions which include the await statement, but doesn't change the behaviour of the implicitly created promise (step 1) once the async function starts executing. Furthermore any promise resolving with another promise gets an implicit tick (step 13) on the microtask queue, meaning promise chaining is deferred. That's where the 2 ticks come from.

For better understanding, the line

(async () => p)()
  .then(() => console.log('after:return'));  // 1

from your second example is currently equivalent to

(new Promise(resolve => resolve(p)))
  .then(() => console.log('after:return'));  // 2

If we want to get rid of these 2 ticks, the line in question would become equivalent to

(function() { return Promise.resolve(p); })()
  .then(() => console.log('after:return'));  // 3

Let's discuss removing each of the 2 ticks separately.

Deferring the implicit creation of the wrapper promise inside async functions in case we actually need to await on an asynchronous task (which excludes async functions without await or with awaits only on resolved promises) will indeed remove the performance overhead of turning a synchronous function to asynchronous. It will though introduce the possibility to create starvation, for instance if the await statement is in a loop. This possibility outweighs the performance gain IMO, so it's better not to do it.

Second, if we want to remove the extra tick for chaining native, non-patched promises (introduce "eager" chaining), this will effectively change observable behaviour for applications already shipping with native promises. Think of it as changing the behaviour of line 2 from above to be equivalent to line 3, which will actually introduce further inconsistency.

As @hax already noted, Node 8 shipped with this behaviour after optimization in V8, but that resulted in a bug in the async hooks in Node.

I hope this answers the question why we have the current behaviour from the second example and also shows the downsides of the possible changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment