Direct return from async function takes too many ticks

[jridgewell]

Description:

In #1250, we reduced the number of ticks that awaiting a native promise value (from 3 ticks to 1 tick). However, returning from an async function can still take 3 ticks (somehow, I don't really understand it).

async function returnDirectPrimitive() {
  return 1;
}

async function returnAwaitPrimitive() {
  return await 1;
}

async function returnDirectPromisePrimitive() {
  return Promise.resolve(1);
}

async function returnAwaitPromisePrimitive() {
  return await Promise.resolve(1);
}

const resolved = Promise.resolve();

async function test(fn) {
  let done = false;
  let count = 0;
  fn().then(() => { done = true; });

  function counter() {
    if (done) {
      print(`${fn.name} took ${count} ticks`);
    } else {
      resolved.then(() => {
        count++;
        counter();
      });
    }
  }
  counter();
}

async function tests() {
  await resolved;
  await test(returnDirectPrimitive);
  await test(returnAwaitPrimitive);

  await test(returnDirectPromisePrimitive);
  await test(returnAwaitPromisePrimitive);
}

tests();

eshost Output:

#### engine262, Moddable XS, QuickJS, SpiderMonkey, V8
returnDirectPrimitive took 1 ticks
returnAwaitPrimitive took 2 ticks
returnDirectPromisePrimitive took 3 ticks
returnAwaitPromisePrimitive took 2 ticks

#### Hermes
returnDirectPrimitive took 1 ticks
returnAwaitPrimitive took 2 ticks
returnDirectPromisePrimitive took 1 ticks
returnAwaitPromisePrimitive took 2 ticks

#### JavaScriptCore
returnDirectPrimitive took 1 ticks
returnAwaitPrimitive took 2 ticks
returnDirectPromisePrimitive took 3 ticks
returnAwaitPromisePrimitive took 3 ticks

(I believe that Hermes and JSC are incorrect here, and the other impls are technically correct. But none of the impls have the behavior I expect.)

✅ The "returnDirectPrimitive" branch, we have the value immediately and should only be waiting 1 tick for the chained then to fire.

✅ The "returnAwaitPrimitive" branch, we should wait 1 tick to extract the value out of the implicitly created promise of 1, immediately return the value, then 1 tick for the chained then to fire.

❌ The "returnDirectPromisePrimitive" branch. There's disagreement here between impls, and none of them behave like I expected. My thinking is that this should be just like "returnAwaitPrimitive", where we need only 2 ticks (1 tick for the async function to assimilate the value from the promise, and 1 tick for the chained then to fire). But somehow, it seems we should be waiting 3 ticks?! Is this creating a promise, wrapping it with another promise, then assimilating that promise (similar to #1250)? If so, we should fix this to take 2 ticks. (Hermes seems to be directly returning the promise with extracting?)

✅ Finally we have the "returnAwaitPromisePrimitive", and this reenforces my belief that "returnDirectPromisePrimitive" is incorrect. In this branch, we wait 1 tick to extract the value from already resolved promise, immediately return the value, and 1 tick for the chained then to fire. (JSC seems to still have the legacy behavior we changed in #1250?)

[bergus]

I've noticed this as well, would be great if it could be improved.

[jridgewell]

Digging through the fix for this, I've actually found that our Promise implementation subtly differs from the A+ spec. I believe this is the root cause for both this bug and #1250. We can attempt to solve this in the same way #1250 did (by calling PromiseResolve and PerformPromiseThen in AsyncBlockStart). Or, we can fix the root bug:

When you call new Promise((res) => …), that res function is created according to the Promise Resolve Functions. This is intended to follow the Promises A+ spec's Promise Resolution Procedure. But, when handling thenable's, we queue a Job which then wraps the thenable.then(onFulfilled, onRejected)(Steps 13-15 of https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-promise-resolve-functions). This is not in the A+ spec, which says:

If then is a function, call it with x as this, first argument resolvePromise, and second argument rejectPromise…
— https://promisesaplus.com/#point-56

The A+ spec does not mention creating a new job before calling then, it just says to call then. A+ specifically mentions "must not be called until the execution context stack contains only platform code" when it wants to queue a job.

Essentially, we create a Job for wrapping thenable.then(onFulfilled, onRejected). And then also creates a Job for wrapping the onFulfilled/onRejected callbacks passed to it (Steps 2-8 of https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-performpromisethen).

This means that (A) new Promise(res => res(thenable)) and (B) new Promise(res => thenable.then(v => res(v))) behave differently. (A) requires 2 ticks (when it should only require 1), and (B) requires only 1 tick (because we extract out the non-thenable v, and invoke the Promise Resolve Functions with that, which skips the Job it would otherwise create).

This is the reason we had to patch Await in #1250 to use PromiseResolve and PerformPromiseThen. Essentially, we change the spec from using (A) (requiring 2 ticks) to (B) (requiring 1 tick). If we were to remove the Job from the Promise Resolve Functions, then we could revert #1250 without slowing down Await, and it would fix the direct return of async () => { return Promise.resolve(1) } requiring an extra tick.

[bergus]

Oh, I'd love to see that! If you're open to fundamentally changing the promise resolve procedure, can I suggest to also tackle this old problem and basically call PerformPromiseThen(promise, fulfillOuter, rejectOuter) instead of PerformPromiseThen(promise, resolveOuter, rejectOuter) if promise is known to be a builtin promise (not an arbitrary thenable)?

[clshortfuse]

I have a somewhat long breakdown as to why it's 3 ticks in a StackOverflow here

Some key snippets are:

Basically, if you run await on a Promise (a real one, not a polyfill), await doesn't try to wrap it. It executes the promise as is. That skips a tick. This is a "newer" change from 2018. In summary, evaluating await always returns the result of a Promise, not a Promise, while avoiding wrapping Promises when possible. That means await always takes at least one tick.

But that's await and async doesn't actually use this bypass. Now async uses the good ol' PromiseCapability Record. We care about how this resolves promises. The key points are it'll instantly start fulfilling if the resolution is "not an Object" or if .then is not Callable. If neither are true, (you're returning a Promise), it'll perform a HostMakeJobCallback and tack on to the then in the Promise, which basically means, we're adding a tick. Clarified, if you return a Promise in an async function, it'll add on an extra tick, but not if you return a Non-Promise.

[...]

await will convert a Promise to a Non-Promise which is perfect for async. If you call await on a Promise, it'll execute it without tacking any extra ticks (+1). But, await will convert a Non-Promise into a Promise and then run it. That means await always takes a tick, regardless of what you call it against.

If we adopt the same exact logic (optimization) to async as we do await, then we should be able to reduce a tick or two.