Since Promise implementation is ported to c++ in bug 1313049, async/await implementation should also use c++, for performance.

here's try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ef2e8c9d426 and performance comparison. actually I'm not sure how to check proper performance for async functions tho, prepared test code that at least executes async function creation and execution many times. [code 1] create many async functions and run them once per each var start = Date.now(); function f() { return async function () { var a = await 1; var b = await Promise.resolve(10); return a + b; }; } var V = 0; for (let i = 0; i < 100000; i++) { f()().then(x => V += x); } drainJobQueue(); var end = Date.now(); print(end - start, "ms"); [result 1] before: 3890 [ms] after: 3294 [ms] [code 2] create single async functions and run it many times var start = Date.now(); function f() { return async function () { var a = await 1; var b = await Promise.resolve(10); return a + b; }; } var V = 0; var a = f(); for (let i = 0; i < 100000; i++) { a().then(x => V += x); } drainJobQueue(); var end = Date.now(); print(end - start, "ms"); [result 2] before: 2848 [ms] after: 2607 [ms]

Nice improvements! Something that'd really make a difference is if we'd not create Promise objects in cases where we can detect that the Promise itself doesn't escape. I fully expect people to eventually use await in hot loops, at which point this'll become quite relevant. But that's all for another day, let's get these optimizations landed and move on for now.

Added JSOP_TOASYNC and changed bytecode from: getintrinsic // AsyncFunction_wrap undefined // AsyncFunction_wrap undefined lambda // AsyncFunction_wrap undefined unwrapped call 1 // wrapped to: lambda // unwrapped toasync // wrapped needsHomeObject==true case is changed from: lambda // unwrapped getintrinsic // unwrapped AsyncFunction_wrap undefined // unwrapped AsyncFunction_wrap undefined dupat 2 // unwrapped AsyncFunction_wrap undefined unwrapped call 1 // unwrapped wrapped dup // unwrapped unwrapped toasync // unwrapped wrapped to: lambda // unwrapped dup // unwrapped unwrapped toasync // unwrapped wrapped so simple :) Now `wrapped` function is native, that reuses name and length of `unwrapped`, and holds `unwrapped` in extended slot. each self-hosted functions' implementation are move to following c++ functions: wrapper => WrappedAsyncFunction AsyncFunction_start => AsyncFunctionStart AsyncFunction_resume => AsyncFunctionResume, AsyncFunctionThrown, AsyncFunctionReturned, AsyncFunctionAwait onFulfilled in AsyncFunction_resume => AsyncFunctionAwaitedFulfilled onRejected in AsyncFunction_resume => AsyncFunctionAwaitedRejected also, some minor changes: * rename CreateAsyncFunction to WrapAsyncFunction since CreateAsyncFunction sounds like corresponds to AsyncFunctionCreate in the spec, but it doesn't actually * changed WrapAsyncFunction signature to be directly callable Ion * removed JSContext* parameter from IsWrappedAsyncFunction * call WrapAsyncFunction from AsyncFunctionConstructor * changed ASYNC_UNWRAPPED_SLOT from 1 to 0, since it's now a native function * fix GetUnwrappedAsyncFunction's parameter name from "wrapper" to "wrapped"

will ask review for Part 2-3 after Part 1, since they're JIT support for JSOP_TOASYNC

Comment on attachment 8807473 [details] [diff] [review] Part 1: Port async/await implementation from self-hosted JS to C++. Review of attachment 8807473 [details] [diff] [review]: ----------------------------------------------------------------- This is actually nicer than before, thank you! r=me with nits addressed. Perhaps file a bug for the follow-up optimizations I suggested - though it's entirely ok not to do that: we'll get to them eventually once async/await performance becomes important enough. ::: js/src/vm/AsyncFunction.cpp @@ +49,5 @@ > > +static bool AsyncFunctionStart(JSContext* cx, HandleValue generatorVal, MutableHandleValue rval); > + > +#define ASYNC_WRAPPED_SLOT 1 > +#define ASYNC_UNWRAPPED_SLOT 0 nit: To me it's not immediately clear whether these are slots *on* wrapped and unwrapped functions, or *for* them. Turns out they're both, which is slightly unusual. Perhaps this could be UNWRAPPED_ASYNC_SLOT_WRAPPED 0 WRAPPED_ASYNC_SLOT_UNWRAPPED 1 @@ +89,5 @@ > + return true; > +} > + > +// Async Functions proposal 2.1 steps 1, 3 (partially). > +// In the spec it creates single function, but we create 2 functions "creates a" @@ +91,5 @@ > + > +// Async Functions proposal 2.1 steps 1, 3 (partially). > +// In the spec it creates single function, but we create 2 functions > +// `unwrapped` and `wrapped`. `unwrapped` is a generator that corresponds to > +// the async function's body, replacing `await` to `yield`. `wrapped` is a s/to/with/ @@ +92,5 @@ > +// Async Functions proposal 2.1 steps 1, 3 (partially). > +// In the spec it creates single function, but we create 2 functions > +// `unwrapped` and `wrapped`. `unwrapped` is a generator that corresponds to > +// the async function's body, replacing `await` to `yield`. `wrapped` is a > +// function that is visible to outside, and handles yielded values. s/to/to the/ @@ +99,5 @@ > +{ > + MOZ_ASSERT(unwrapped->isStarGenerator()); > + > + // Create a new function with AsyncFunctionPrototype, reusing the name and > + // the length of `unwrapped` function. Either "of the `unwrapped` function" or "of `unwrapped`". (With a slight preference for the latter, but up to you.) @@ +120,5 @@ > + TenuredObject)); > + if (!wrapped) > + return nullptr; > + > + // Link them each other to make GetWrappedAsyncFunction and "to each other" @@ +180,5 @@ > + : cx->names().StarGeneratorThrow; > + FixedInvokeArgs<1> args(cx); > + args[0].set(valueOrReason); > + RootedValue result(cx); > + if (!CallSelfHostedFunction(cx, funName, generatorVal, args, &result)) Eventually it'd be nice to not have to do this JS call, or the creation of the `result` object or reading the `done` and `value` fields from it below. Obviously not in this bug, though. @@ +246,5 @@ > + // Step 8. > + RootedValue onFulfilledVal(cx, ObjectValue(*onFulfilled)); > + RootedValue onRejectedVal(cx, ObjectValue(*onRejected)); > + RootedObject resultPromise(cx, OriginalPromiseThen(cx, resolvePromise, onFulfilledVal, > + onRejectedVal)); We can definitely optimize these function allocations away entirely. The Promise implementation already supports default resolve/reject functions and a `flags` field. Extending this to handle the await case, storing the generator in the PromiseSlot_RejectFunction slot (which we should probably rename in that case) seems fairly straight-forward. But again, not in this bug. @@ +301,2 @@ > { > + JSFunction* unwrapped = &wrapped->getExtendedSlot(ASYNC_UNWRAPPED_SLOT).toObject().as<JSFunction>(); Can you assert IsWrappedAsyncFunction here? ::: js/src/vm/Opcodes.h @@ +1529,5 @@ > * Stack: => new.target > */ \ > macro(JSOP_NEWTARGET, 148, "newtarget", NULL, 1, 0, 1, JOF_BYTE) \ > + /* > + * Pops the top of stack value as 'unwrapped', and converts it to asycn ", converts it to async" (removed "and", fixed spelling of async)

thanks! addressed review comments.

Support JSOP_TOASYNC in Baseline. that just calls WrapAsyncFunction, via BaselineToAsync that changes signature.

Support JSOP_TOASYNC in Ion, that just calls WrapAsyncFunction directly.

uh, the patch part 1 is wrong. It doesn't perform AsyncFunctionAwait steps 2-3 correctly and gets "constructor" in Promise.resolve step 3.a, that shouldn't happen. https://tc39.github.io/ecmascript-asyncawait/#abstract-ops-async-function-await https://tc39.github.io/ecma262/#sec-promise.resolve then, sadly, the performance improvement comes from the wrong path. when I fix it, it doesn't become much faster than before... anyway, will fix it.

(In reply to Tooru Fujisawa [:arai] from comment #9) > then, sadly, the performance improvement comes from the wrong path. > when I fix it, it doesn't become much faster than before... so, I think the performance improvement is possible, when the passed promise object satisfies the condition. maybe, not-modified and not-subclassed Promise object, or similar.

the optimization will require special care about timing, since it's async. I'll fix the part 1's issue in bug 1315559, since it re-writes the AsyncFunctionAwait implementation and it's clear than fixing with current way. after that, will re-think about performance improvement.

Comment on attachment 8807506 [details] [diff] [review] Part 2: Support JSOP_TOASYNC in Baseline. Review of attachment 8807506 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but a suggestion below to get rid of the Baseline-specific wrapper. ::: js/src/jit/BaselineCompiler.cpp @@ +3819,5 @@ > + prepareVMCall(); > + > + pushArg(R0); > + > + if (!callVM(BaselineToAsyncInfo)) Can we call WrapAsyncFunction directly here, without going through a Value-taking/returning wrapper? For the argument, you can do: masm.unboxObject(frame.addressOfStackValue(frame.peek(-1)), R0.scratchReg()); pushArg(R0.scratchReg()); And for the return value: masm.tagValue(JSVAL_TYPE_OBJECT, ReturnReg, R0); frame.push(R0);

Comment on attachment 8807507 [details] [diff] [review] Part 3: Support JSOP_TOASYNC in Ion. Review of attachment 8807507 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonBuilder.cpp @@ +13576,5 @@ > +IonBuilder::jsop_toasync() > +{ > + MDefinition* unwrapped = current->pop(); > + if (unwrapped->type() != MIRType::Object) > + return abort("toasync with non-object"); Can we make this MOZ_ASSERT(unwrapped->type() == MIRType::Object)?

Thank you for reviewing :) removed BaselineToAsync.

Comment on attachment 8808145 [details] [diff] [review] Part 2: Support JSOP_TOASYNC in Baseline. Review of attachment 8808145 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ced693a9b87d2870f980bfd3e0046439b58115c Bug 1314055 - Part 1: Port async/await implementation from self-hosted JS to C++. r=till https://hg.mozilla.org/integration/mozilla-inbound/rev/2b2960ee6b45cf7e4ae227427b26f6c4fa8cc43f Bug 1314055 - Part 2: Support JSOP_TOASYNC in Baseline. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/d1710006b0936f2315997fcc39c092b72220e6eb Bug 1314055 - Part 3: Support JSOP_TOASYNC in Ion. r=jandem

https://hg.mozilla.org/mozilla-central/rev/9ced693a9b87 https://hg.mozilla.org/mozilla-central/rev/2b2960ee6b45 https://hg.mozilla.org/mozilla-central/rev/d1710006b093