Closed Bug 1315756 Opened 8 years ago Closed 8 years ago

Do not create throwawayCapability in AsyncFunctionAwait.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: arai, Assigned: arai)

References

()

Details

Attachments

(1 file)

(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.
it's about AsyncFunctionAwait step 7
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+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: