Closed Bug 1314055 Opened 3 years ago Closed 3 years ago

Port async/await implementation from self-hosted JS to c++.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: arai, Assigned: arai)

References

()

Details

Attachments

(3 files, 2 obsolete files)

Since Promise implementation is ported to c++ in bug 1313049,
async/await implementation should also use c++, for performance.
Summary: Port async/await implementation fro self-hosted JS to c++. → Port async/await implementation from self-hosted JS to c++.
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]
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
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"
Attachment #8807473 - Flags: review?(till)
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)
Attachment #8807473 - Flags: review?(till) → review+
thanks!
addressed review comments.
Attachment #8807473 - Attachment is obsolete: true
Attachment #8807505 - Flags: review+
Support JSOP_TOASYNC in Baseline.
that just calls WrapAsyncFunction, via BaselineToAsync that changes signature.
Attachment #8807506 - Flags: review?(jdemooij)
Support JSOP_TOASYNC in Ion,
that just calls WrapAsyncFunction directly.
Attachment #8807507 - Flags: review?(jdemooij)
See Also: → 1315559
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.
See Also: → 1315620
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);
Attachment #8807506 - Flags: review?(jdemooij)
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)?
Attachment #8807507 - Flags: review?(jdemooij) → review+
Thank you for reviewing :)
removed BaselineToAsync.
Attachment #8807506 - Attachment is obsolete: true
Attachment #8808145 - Flags: review?(jdemooij)
Comment on attachment 8808145 [details] [diff] [review]
Part 2: Support JSOP_TOASYNC in Baseline.

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

Thanks!
Attachment #8808145 - Flags: review?(jdemooij) → review+
See Also: → 1316098
See Also: → 1316141
Duplicate of this bug: 1316166
Depends on: 1317460
Depends on: 1317824
You need to log in before you can comment on or make changes to this bug.