Closed
Bug 1315756
Opened 8 years ago
Closed 8 years ago
Do not create throwawayCapability in AsyncFunctionAwait.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
()
Details
Attachments
(1 file)
(deleted),
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
(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•8 years ago
|
Assignee | ||
Comment 1•8 years ago
|
||
it's about AsyncFunctionAwait step 7
Assignee | ||
Comment 2•8 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
Comment 3•8 years ago
|
||
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•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/38f5ec02b1d669c4e2894075da59d3e0d354aeb3
Bug 1315756 - Do not allocate throwawayCapability in AsyncFunctionAwait. r=till
Comment 5•8 years ago
|
||
bugherder |
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.
Description
•