Review Implementation of Async/Await

[mike-kaufman]

This code will log 1, 2, 3 on chakra. On v8, it will log 2, 3, 1. Believe that the difference here is Chakra is not creating a new promise for the await. Spec section on await claims that we should be creating two new promises when we await, but we're only creating one.

async function f2() {
 return 1;
}

async function f1() {
 let i = await f2();
 console.log(i);
}

let p = f1();

Promise.resolve(2).then(() => {
 console.log(2);
 Promise.resolve(3).then(() => {
   console.log(3);
 });
});

Edit: My understanding from talking w/ folks is that Chakra's implementation of async/await predates the spec. It's currently a bit difficult to map the implementation to the spec language.

[MSLaguana]

I just tried this out, and I see some strange behavior. If I copy what you have there into a node (v8) repl environment, then I see the 2, 3, 1 behavior that you mention, but if I put it in a file and run it then I get 1, 2, 3, using the exact same node binary. Trying it out in chrome's devtools I do see the 2, 3, 1 as well, but I haven't tried making a page and running it there yet.

[MSLaguana]

Ah; when I had node v8 print 1, 2, 3 it was with node 9.5, but with node 10.4.0 it consistently prints 2, 3, 1.

[MSLaguana]

This has been bugging me so I've been poking around at it. I think the difference is only if we await a promise. I've been using this snippet as a testing ground:

async function f1() {
    console.log("Start");
    Promise.resolve(0).then(() => {
        console.log(1);
    }).then(() => {
        console.log(2);
    }).then(() => {
        console.log(3);
    }).then(() => {
        console.log(4);
    }).then(() => {
        console.log(5);
    }).then(() => {
        console.log(6);
    }).then(() => {
        console.log(7);
    });
    await 1;//{then: (resolve) => resolve({then: (x) => x(x)})};
    console.log("End");
}

f1()

where I change the thing that is awaited and see how many promise 'ticks' it takes in both chakra and v8. If I use a plain value, or a non-promise thenable, then both chakra and v8 seem to agree on the number of ticks (1 for await 1, 2 for await {then: (r) => r(1)}, 3 for await {then: (r1) => r1({then: (r2) => r2(1)})})

However if I await Promise.resolve(1) then chakra takes 1 tick to print "end", while v8 takes 3 ticks, and if I await Promise.resolve({then: (r) => r(1)}) then chakra takes 2 ticks, while v8 still takes the same 3.

I don't yet fully understand why native promises change things here, and why a native promise resolved with a thenable changes chakra but not v8.

[MSLaguana]

@zenparsing any ideas on whether thenables should be treated differently to promises here, and does v8's behavior where a Promise.resolve(thenable) doesn't take any longer than Promise.resolve(1) make sense?

[zenparsing]

@MSLaguana The spec says here that evaluation of await always creates a new promise that is resolved with the await operand. Something like this:

// await x
new Promise(resolve => {
  resolve(x);
}).then(resumeAsyncFunction);

If x is a thenable (native Promise or otherwise), the code above will enqueue a job to call x.then. That's basically what the spec says should happen, and what v8 is doing.

It appears that Chakra, on the other hand, is doing something like:

// await x
Promise.resolve(x).then(resumeAsyncFunction);

The difference here is that Promise.resolve behaves differently if the argument is a native promise: it simply returns it.

FYI, we are currently looking into whether it would be a good idea to modify the specification to do something more like Promise.resolve for efficiency reasons.

[MSLaguana]

@zenparsing Something that I don't understand is why Promise.resolve(1) and Promise.resolve(thenable) behave the same for v8 but not for chakra. From my reading of the spec, a Promise.resolve(thenable) will need to queue a PromiseResolveThenableJob or something which then calls into the thenable and eventually schedules a PromiseReactionJob, while Promise.resolve(1) directly schedules a PromiseReactionJob, so I thought there should be one "tick" difference between them, which is what I see in chakra, but not in v8.

[zenparsing]

I'm seeing the same behavior for v8 and Chakra for Promise.resolve with respect to native and thenables.

For this test case:

let thenable = {
  then(a, b) {
    console.log('then called');
    a(1);
  },
};

let native = new Promise(resolve => resolve(1));

console.log(0);

Promise.resolve(thenable).then(() => {
  console.log(1);
});

Promise.resolve(native).then(() => {
  console.log(2);
});

both engines log:

0
then called
2
1

We can see that for the thenable case, "then" is getting called in a job, and creates an extra delay relative to a native promise.

But perhaps there's another test case that might show a difference?

[MSLaguana]

I see the difference when using async functions as I showed above:

async function f1() {
    console.log("Start");
    Promise.resolve(0).then(() => {
        console.log(1);
    }).then(() => {
        console.log(2);
    }).then(() => {
        console.log(3);
    }).then(() => {
        console.log(4);
    }).then(() => {
        console.log(5);
    }).then(() => {
        console.log(6);
    }).then(() => {
        console.log(7);
    });
    await Promise.resolve({then: (resolve) => resolve(1)}); // or just Promise.resolve(1)
    console.log("End");
}

f1()

[rhuanjl]

During my current effort on async generators I've gone over async functions quite a bit and noticed a couple of inefficiencies and at least one clear spec violation in addition to the points discussed above:

Spec violation, per https://tc39.github.io/ecma262/#await step 10 await uses PerformPromiseThen, it does not use the then property of the Promise prototype - i.e. it should ignore user modified then functions but currently CC uses the modified version:

Promise.prototype.then = function () { print("This should not be printed"); }

async function test()
{
    await 5;
    print("pass")
}

test();

With eshost:

#### JavaScriptCore, SpiderMonkey, V8 --harmony
pass

#### Chakra (v1.11), chDev (master)
This should not be printed

I'd like to update the async function implementation somewhat thoroughly when I'm done with async generator functions - as I'd like them to share some code that may make implementing for await of easier - if that's ok I'll pick up the promise ordering point as well as this other bug I've noticed at that time? (I may need some help ensuring I don't break TTD support but should be fine otherwise)

[zenparsing]

Hi @rhuanjl!

You are correct regarding Chakra's current await behavior, but be aware of the following spec change that will likely land soon:

https://github.com/tc39/ecma262/pull/1250

Even after this spec update, we'll need to fix the issue you describe so that user-provided "then" functions are not called.

I think this is where the change needs to happen:

https://github.com/Microsoft/ChakraCore/blob/05bcc0b8a621315ae996916f3079516d847a9c12/lib/Runtime/Library/JavascriptPromise.cpp#L1449

[rhuanjl]

Indeed the quick fix to the issue I pointed would be to use the internal method CreateThenPromise on that line instead of using GetProperty for then and calling it.

But what I'd like to do is make a single implementation of await which can be shared between async functions and async generator functions and can be used within the implementation of for await of that I'm hoping to do later. Currently await within async functions is sort of wrapped around other aspects of the implementation of async functions and hence can't be shared easily.

Additionally if there is one implementation rather than several it will be much easier to update for the noted spec change or any future ones.

[rhuanjl]

One more point I thought of here - the current async/await implementation uses Generator.prototype.next() calls to start executing the async function and to move from await to await through it.

These calls are observable in stack traces - but per the spec async/await does not use Generator.prototype.next()

(My suggestion of using the new Op_Await I've written for AsyncGenerators would also remove the use of Generator.prototype.next)

[rhuanjl]

I've been looking more at this - obviously #6312 is a massive improvement.

But even with that each await will still involve:

1. Perform Promise.resolve() on the awaited value
2. Create 2 continuation methods
3. Create an unused promise capability
4. Create 2 PromiseReaction objects
5. Attach the promise reaction objects to the result of 1
6. trigger the correct promise reaction and resolve the unused capability (not exposed anywhere so user cannot detect this)
7. Create a promise reaction task function that wraps the relevant continuation method from 2
8. Pass the promise reaction task function to the host's promise continuation method

Whilst some of those steps are necessary several of them really aren't notably steps 2,3,4,5 and 7 create throw away objects, re-created for every await that are not exposed to the runtime and hence not detectable and that could be eliminated through re-factoring.

I'm looking into a way of doing this that will benefit both AsyncFunctions and AsyncGenerators:

On a by step basis:

1. no change can be made - the use of Promise.resolve() - or an internal equivalent - is mandated by the spec
2. These continuations could be cached
3. This promise capability could be removed entirely
4. These promise reaction objects could be cached
5. This step is necessary
6. The resolution of the unused capability could be eliminated
7. This step could be eliminated all the reaction task function does is get the resolved value of the source promise and then call the continuation method with it as a parameter - as we know what the continuation method is ahead of time we could instead attach the resolved value to it in an internal slot.
8. Here we could pass the continuation method with the resolved value attached instead of a reaction task function

Making this theoretical change the only allocation done for each await would be the Promise from Promise.resolve() if the awaited item is not itself a promise.

On a side note... I'm wondering if the normal (not async function) use .then() can also be optimised slightly - notably if the result of the .then() is unused AND the Promise constructor has not been overwritten then the PromiseCapabiity it allocates could be optimised away. Other optimisations may take more effort; but e.g. a rotating pool of reaction methods could be used instead of creating and destroying them for each .then() - though that may involve some extensive internal machinery and I don't know how beneficial it would be.

[zenparsing]

@rhuanjl I'm really glad to see someone other than myself thinking about this : ) A bunch of the code movement in #6312 was geared toward setting the stage for these kinds of improvements.

It would be great to put together benchmark numbers to guide the optimizations (since it seems like some of the changes you're suggesting could be done separately). Also, I probably wouldn't worry about optimizing "then" for now. The goal should be getting async/await running very efficiently. Which reminds me, I also need to work on getting generator JITing enabled!