Tooru Fujisawa [:arai] Assignee
	Description • 8 years ago

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

	Tooru Fujisawa [:arai] Assignee
	Comment 1 • 8 years ago

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]

	Till Schneidereit [:till]
	Comment 2 • 8 years ago

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.

	Tooru Fujisawa [:arai] Assignee
	Comment 3 • 8 years ago

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"

	Tooru Fujisawa [:arai] Assignee
	Comment 4 • 8 years ago

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

	Till Schneidereit [:till]
	Comment 5 • 8 years ago

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)

	Tooru Fujisawa [:arai] Assignee
	Comment 6 • 8 years ago

thanks!
addressed review comments.

	Tooru Fujisawa [:arai] Assignee
	Comment 7 • 8 years ago

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

	Tooru Fujisawa [:arai] Assignee
	Comment 8 • 8 years ago

Support JSOP_TOASYNC in Ion,
that just calls WrapAsyncFunction directly.

	Tooru Fujisawa [:arai] Assignee
	Comment 9 • 8 years ago

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.

	Tooru Fujisawa [:arai] Assignee
	Comment 10 • 8 years ago

(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.

	Tooru Fujisawa [:arai] Assignee
	Comment 11 • 8 years ago

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.

	Jan de Mooij [:jandem]
	Comment 12 • 8 years ago

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);

	Jan de Mooij [:jandem]
	Comment 13 • 8 years ago

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)?

	Tooru Fujisawa [:arai] Assignee
	Comment 14 • 8 years ago

Thank you for reviewing :)
removed BaselineToAsync.

	Jan de Mooij [:jandem]
	Comment 15 • 8 years ago

Comment on attachment 8808145 [details] [diff] [review]
Part 2: Support JSOP_TOASYNC in Baseline.

Review of attachment 8808145 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

	Tooru Fujisawa [:arai] Assignee
	Comment 16 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 18 • 8 years ago
bugherder

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