Closed Bug 1316098 Opened 3 years ago Closed 3 years ago

Optimize out result object allocation in await.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- wontfix
firefox54 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file)

(separated from bug 1314055)

currently async function's unwrapped function is star generator, that creates result object {value,done} per each await, but it's exposed only to engine,
so we could optimize out the allocation, and pass value/done in more efficient way.
just as a simple solution, we could, create the result object at the top of unwrapped function, and reuse it for all await.
that way the other parts won't change.

any case, we need to return 2 values, `value` and `done`, so we need an object, or another storage, maybe function's slot,
or, just use some sentinel value for done==true case.
uh, actually, we need separate object for return and await, in the following case:
  async function f() {
    try {
      return 10;
    } finally {
      await 30
    }
  }

at least, each await can share result object.
unfortunately, just reusing result object in await doesn't improve performance.
it might be nice for GC tho...
with test patch that returns await operand directly, and return undefined as a sentinel value for `return` (this is just for test, it's of course incorrect), there's 4% performance improvement in the following code.
So I think that's the maximum performance improvement we can get with modifying result object, and of course we need to do some more operation to make it correct, so the amount of improvement will reduce from that.

code:
  var start = Date.now();
  async function a() {
    var x = 0;
    for (let i = 0; i < 100000; i++) {
      x += await 1;
      x += await Promise.resolve(10);
    }
    return x;
  };
  var V = 0;
  a().then(x => V = x);
  drainJobQueue();
  var end = Date.now();
  print(end - start, "ms");

result (average of 20 runs):
  before: 891 [ms]
  after:  855 [ms]
in bug 1331092, I'm about to add JSOP_AWAIT, and make it detectable what was the last opcode when the generator yields/returns,
with that, we could just return the await operand without wrapping it with result object.
Depends on: 1343481
bug 1343481 added JSOP_AWAIT, and GeneratorObject::isAfterAwait.
now we can detect the previous opcode after the generator suspends.

removed result object for await and return (finalyield) for async function.

BytecodeEmitter::emitAwait emits JSOP_AWAIT with raw operand.
BytecodeEmitter::emitReturn and BytecodeEmitter::emitFunctionBody emits JSOP_FINALYIELDRVAL with raw operand, controlled by FunctionBox::needsIteratorResult, that now returns false for async function.

In AsyncFunctionResume, it checks the last opcode, and if it was await, AsyncFunctionAwait is called with the returned value.
otherwise, AsyncFunctionReturned is called with returned value.

Also, reverted the restriction for debugger, added in bug 1317824.
previously debugger was checking the resumption value for async function, but now it don't have to.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #8842821 - Flags: review?(till)
Comment on attachment 8842821 [details] [diff] [review]
Optimize out result object allocation for await/return in async function.

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

This is great! An optimization that is also a substantial code cleanup *and* simplifies tests. What's not to love?

::: js/src/frontend/BytecodeEmitter.cpp
@@ -8354,5 @@
>  bool
>  BytecodeEmitter::emitYield(ParseNode* pn)
>  {
>      MOZ_ASSERT(sc->isFunctionBox());
> -    MOZ_ASSERT(pn->getOp() == JSOP_YIELD || pn->getOp() == JSOP_AWAIT);

Should this retain the assert and just remove the JSOP_AWAIT part?

@@ +8396,1 @@
>      return true;

I wouldn't mind for all of these to be combined into one return:
return emitTree(pn->pn_kid) &&
       emitGetDotGenerator() &&
       emitYieldOp(JSOP_AWAIT);

But I guess the style in this file is for it to be individual if statements.
Attachment #8842821 - Flags: review?(till) → review+
Thank you for reviewing :)

(In reply to Till Schneidereit [till] from comment #8)
> @@ +8396,1 @@
> >      return true;
> 
> I wouldn't mind for all of these to be combined into one return:
> return emitTree(pn->pn_kid) &&
>        emitGetDotGenerator() &&
>        emitYieldOp(JSOP_AWAIT);
> 
> But I guess the style in this file is for it to be individual if statements.

Yeah, also having separate if statements is better for debugging.
https://hg.mozilla.org/integration/mozilla-inbound/rev/37802af7d64b2721f92c547b2dbc076492c8be4a
Bug 1316098 - Optimize out result object allocation for await/return in async function. r=till
https://hg.mozilla.org/mozilla-central/rev/37802af7d64b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.