Do not create throwawayCapability in AsyncFunctionAwait.

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

()

Attachments

(1 attachment)

Assignee

Description

3 years ago
(separated from bug 1315559)

Currently async/await is implemented with promise chain, but it doesn't match to the reference implementation in the spec:
https://tc39.github.io/ecmascript-asyncawait/#desugaring

using promise chain makes it hard to optimize out throwawayCapability,
since we should return promise from handler.

We should use the exactly same way (not promise chain), so that we could remove promise allocation.
Assignee

Updated

3 years ago
See Also: → 1315620
Assignee

Comment 1

3 years ago
it's about AsyncFunctionAwait step 7
Assignee

Comment 2

3 years ago
now this implementation almost follows the informative desugaring
  https://tc39.github.io/ecmascript-asyncawait/#desugaring
that returns nothing from handlers.

here's the change:
  * create Promise in WrappedAsyncFunction, that is returned to caller,
    and resolved when async function returns,
    and rejected when async function throws.
  * moved some functions to Promise.cpp to call internal functions
  * AsyncFunctionAwaitedFulfilled and AsyncFunctionAwaitedRejected return nothing
    it just enqueues another promise, via AsyncFunctionAwait/AsyncFunctionReturned/AsyncFunctionThrown
  * AsyncFunctionAwait now calls NewReactionRecord instead of creating throwawayCapability
    and joins at the middle of PerformPromiseThen, to enqueue the reaction
  * Added REACTION_FLAG_AWAIT flag that indicates the reaction is dedicated for await,
  * Reaction with REACTION_FLAG_AWAIT is handled differently in AwaitPromiseReactionJob, called from PromiseReactionJob
  * AwaitPromiseReactionJob calls AsyncFunctionAwaitedFulfilled or AsyncFunctionAwaitedRejected
    and does nothing else, since handlers don't return anything.

performance comparison with bug 1314055 comment #1 testcases
I hope this time the value is valid...

before (Bug 1314055 + Bug 1315559)
  code 1: 3715 ms
  code 2: 3044 ms

after (Bug 1314055 + Bug 1315559 + Bug 1315756)
  code 1: 2251 ms
  code 2: 1795 ms
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #8808343 - Flags: review?(till)
Comment on attachment 8808343 [details] [diff] [review]
Do not allocate throwawayCapability in AsyncFunctionAwait.

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

This is surprisingly clean, for what it does - nice job! r=me with the tiny nits addressed.

::: js/src/builtin/Promise.cpp
@@ +199,5 @@
>          if (state == JS::PromiseState::Fulfilled)
>              flags |= REACTION_FLAG_FULFILLED;
>          setFixedSlot(ReactionRecordSlot_Flags, Int32Value(flags));
>      }
> +    void setAwait() {

nit: please make this "setIsAwait"

@@ +2112,5 @@
> +                                                        Handle<PromiseObject*> promise,
> +                                                        Handle<PromiseReactionRecord*> reaction);
> +
> +// Some async/await functions are implemented here instead of
> +// js/src/builtin/AsyncFunction.cpp, to call Promise internal functions

nit: "." at the end of full-sentence comments.

::: js/src/builtin/Promise.h
@@ -30,5 @@
>  #define PROMISE_FLAG_HANDLED   0x4
>  #define PROMISE_FLAG_REPORTED  0x8
>  #define PROMISE_FLAG_DEFAULT_RESOLVE_FUNCTION 0x10
>  #define PROMISE_FLAG_DEFAULT_REJECT_FUNCTION  0x20
> -#define PROMISE_FLAG_AWAIT     0x40

uber-nit: please right-align this (i.e., move it one col to the left)

::: js/src/vm/AsyncFunction.cpp
@@ +46,5 @@
>      global->setReservedSlot(ASYNC_FUNCTION_PROTO, ObjectValue(*asyncFunctionProto));
>      return true;
>  }
>  
> +static bool AsyncFunctionStart(JSContext* cx, Handle<PromiseObject*> resultPromise,

Can you make this MOZ_MUST_USE, too?
Attachment #8808343 - Flags: review?(till) → review+
Assignee

Comment 4

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/38f5ec02b1d669c4e2894075da59d3e0d354aeb3
Bug 1315756 - Do not allocate throwawayCapability in AsyncFunctionAwait. r=till

Comment 5

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/38f5ec02b1d6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.