1473970 - Skip creating resolving functions for PromiseResolveThenableJob

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, enhancement)

Description

[André Bargull [:anba]]

Try to implement the idea from bug 1412200 comment #3.

Comment 1

[André Bargull [:anba]]

Attachment: bug1473970-part1-realm-check.patch

Add a realm check to ResolvePromiseInternal to ensure errors are created in the correct realm. Also restrict the fast path to when |resolution| is a PromiseObject, which allows us to skip these lines [1] from Promise_then_impl and therefore enabling us to directly call OriginalPromiseThen. 



[1] https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/js/src/builtin/Promise.cpp#3133-3139,3143-3144,3146-3158

Comment 2

[André Bargull [:anba]]

Attachment: bug1473970-part2-split-jobs.patch

Split the fast path into a different job than PromiseResolveThenableJob.

This allows us to skip some more wrapper juggling and more importantly we can avoid creating the job-arguments array [1] and instead directly store the promise and the thenable objects in the extended slots of the job-function.

[1] https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/js/src/builtin/Promise.cpp#1395

Comment 3

[André Bargull [:anba]]

Attachment: bug1473970-part3-skip-creating-resolving.patch

Because we know that the fast path uses the default resolving functions (ResolvePromise and ResolveReject), we can avoid creating them at all.

For this to work we need to create new versions of OriginalPromiseThen and PerformPromiseThen which don't have |resolve| and |reject| parameters, but instead directly pass the to-be-resolved-promise (|promiseToResolve|). Because we now no longer have resolving functions to store |promiseToResolve|, |promiseToResolve| gets directly stored in the PromiseReactionRecord. For this I've reused the existing ReactionRecordSlot_Generator slot, which should be free for this new reaction record use case. In PromiseReactionJob, the new job type is then delegated to DefaultResolvingPromiseReactionJob, which directly calls the underlying functions used in ResolvePromise (ResolvePromiseInternal) and ResolveReject (RejectMaybeWrappedPromise).

Comment 4

[André Bargull [:anba]]

The changes improve the test case from bug 1317481, comment #0 from 480ms to 380ms for me.

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8990434 [details] [diff] [review]
bug1473970-part2-split-jobs.patch

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

Great optimization :D

::: js/src/builtin/Promise.cpp
@@ +1371,5 @@
> + * Usage of the function's extended slots is as follows:
> + * BuiltinThenableJobSlot_Promise: The Promise to resolve using the given
> + *                                 thenable.
> + * BuiltinThenableJobSlot_Thenable: The thenable to use as the receiver when
> + *                                  calling the built-in `then` function.

can we put these comments in BuiltinThenableJobSlots enum,
or add a comment there to point this function's comment?

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8990437 [details] [diff] [review]
bug1473970-part3-skip-creating-resolving.patch

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

The code change itself looks good :)

I'm confused because of the difference between "resolution function" and "resolving function" terms in the existing code/comment.
(I thought they're same, but now I'm not sure...)

in any case, they need some comment, or use only either of them.
also, "WithoutResolving" sometimes sounds misleading because there's another resolving functions around.
so it would be better adding a comment for which resolving functions are optimized out.

::: js/src/builtin/Promise.cpp
@@ +375,5 @@
>      ReactionRecordSlot_Reject,
>      ReactionRecordSlot_IncumbentGlobalObject,
>      ReactionRecordSlot_Flags,
>      ReactionRecordSlot_HandlerArg,
> +    ReactionRecordSlot_GeneratorOrPromise,

can you add some comment that explains the difference between the object pointed by ReactionRecordSlot_Promise?

@@ +449,5 @@
>      }
>      AsyncGeneratorObject* asyncGenerator() {
>          MOZ_ASSERT(isAsyncGenerator());
> +        return &getFixedSlot(ReactionRecordSlot_GeneratorOrPromise).toObject()
> +                                                                   .as<AsyncGeneratorObject>();

while you're here, can you wrap before ".toObject()" and put it and ".as<AsyncGeneratorObject>();" in the same line?
the indentation is too deep now.

@@ +1463,5 @@
>      RootedValue exception(cx);
>      if (!MaybeGetAndClearException(cx, &exception))
>          return false;
>  
> +    return RejectMaybeWrappedPromise(cx, promise, exception);

now we're skipping this check, and I wonder if that such case happens in practice.

https://searchfox.org/mozilla-central/rev/fd5c37f1dd9a0d1e327a6c6b4d81ea92f52c4330/js/src/builtin/Promise.cpp#538-545
>     // In some cases the Promise reference on the resolution function won't
>     // have been removed during resolution, so we need to check that here,
>     // too.
>     if (promise->is<PromiseObject>() &&
>         promise->as<PromiseObject>().state() != JS::PromiseState::Pending)
>     {
>         return true;
>     }

and this assertion:
https://searchfox.org/mozilla-central/rev/fd5c37f1dd9a0d1e327a6c6b4d81ea92f52c4330/js/src/builtin/Promise.cpp#1055
>     MOZ_ASSERT(promise->state() == JS::PromiseState::Pending);

at least, it can happen with resolvePromise/rejectPromise testing functions.
so they should throw error for this case.

if that's not a problem, maybe we could call ResolvePromise directly?

@@ +2690,5 @@
>  }
>  
> +static MOZ_MUST_USE bool
> +OriginalPromiseThenWithoutResolving(JSContext* cx, Handle<PromiseObject*> promise,
> +                                    Handle<PromiseObject*> promiseToResolve)

maybe we could use template function with enum template parameter to merge optimized case and normal case, to reduce the code duplication?
(not required for this bug tho)

Comment 7

[Tooru Fujisawa [:arai]]

(In reply to André Bargull [:anba] from comment #4)
> The changes improve the test case from bug 1317481, comment #0 from 480ms to
> 380ms for me.

\o/

Comment 8

[André Bargull [:anba]]

Attachment: bug1473970-part2-split-jobs.patch

Update part 2 per review comments to move the documentation to the enum. For consistency applied the same change for ThenableJobSlots and ThenableJobDataIndices. Carrying r+.

Comment 9

[André Bargull [:anba]]

Attachment: bug1473970-part3-docs-assertions-tests.patch

Additional patch in preparation for the review comments for the old part 3, which will now be part 4.

- Add docs for the ReactionRecordSlots enum members.
- Move the REACTION_FLAG_XYZ constants into PromiseReactionRecord, because they're only used within this class.
- Add helper to set flags which are only allowed to be set for a newly created PromiseReactionRecord (REACTION_FLAG_ASYNC_FUNCTION, REACTION_FLAG_ASYNC_GENERATOR, REACTION_FLAG_DEBUGGER_DUMMY).
- Check for wrapped Promise objects in [1,2].
- Check for settled Promises after the GetProperty call in ResolvePromiseInternal.
- Remove assertions to check that the Promises are still pending before calling ResolvePromise, because ResolvePromise also performs this check at the very beginning [3].
- Add assertions when enqueueing and triggering promise reactions to clarify that the targetState must either be |Fulfilled| or |Rejected|.
- Change access to handler-value integer constants to use int32-accessors instead of number-accessors with casting. 
- Fix a bug for the dummy reaction which is added in BlockOnPromise, spotted while writing the docs for PromiseReactionRecord. The dummy reaction must not actually resolve a Promise, it is only added for the debugger, but shouldn't otherwise modify the actual Promise behaviour. Regression test case is jit-test/tests/promise/debugger-reaction-does-not-resolve.js.
- Fix another bug found while writing regression tests. GetResolveFunctionFromPromise didn't cover the case when the RejectFunctionSlot_ResolveFunction slot was already cleared. (Regression test is resolve-promise-scripted-and-api.js)
- Move the GetObjectFromIncumbentGlobal call into NewReactionRecord to reduce code duplication.
- Add missing nullptr-check in PromiseObject::resolve(), also found while writing the regression tests.
- Disallow to call SettlePromiseNow on async function promises, to avoid more possible damage possible by SettlePromiseNow.
- Add lots of tests for SettlePromiseNow, which is a bad function and allows to break all kinds of Promise invariants.


[1] https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/js/src/builtin/Promise.cpp#541-545
[2] https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/js/src/builtin/Promise.cpp#651-655
[3] https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/js/src/builtin/Promise.cpp#787,791

Comment 10

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #6)
> I'm confused because of the difference between "resolution function" and
> "resolving function" terms in the existing code/comment.
> (I thought they're same, but now I'm not sure...)

We seem to use both terms interchangeably, but I don't know the history behind it. Maybe sometimes "resolution functions" is used instead of "resolving functions" to avoid confusion with "Promise Resolve Functions"? Or it's just for historical reasons.


> in any case, they need some comment, or use only either of them.
> also, "WithoutResolving" sometimes sounds misleading because there's another
> resolving functions around.
> so it would be better adding a comment for which resolving functions are
> optimized out.

I'll probably rename it to "WithoutSettleHandlers". (In the spec for PerformPromiseThen, onFulfilled and onRejected are described as "settlement actions" and in the PromiseReactionRecord, they're stored in the [[Handler]] field. Combining both terms gives us "SettleHandlers". Still a bad name, but at least it no longer includes "resolve". :-)


> 
> @@ +1463,5 @@
> >      RootedValue exception(cx);
> >      if (!MaybeGetAndClearException(cx, &exception))
> >          return false;
> >  
> > +    return RejectMaybeWrappedPromise(cx, promise, exception);
> 
> now we're skipping this check, and I wonder if that such case happens in
> practice.
> 
> https://searchfox.org/mozilla-central/rev/
> fd5c37f1dd9a0d1e327a6c6b4d81ea92f52c4330/js/src/builtin/Promise.cpp#538-545

It shouldn't happen normally, at least when the Promise APIs are used correctly. The testing function "settlePromiseNow" for example doesn't use the Promise API properly and therefore we do need this check. :-/  Will fix in the next version of the patch.


> >     // In some cases the Promise reference on the resolution function won't
> >     // have been removed during resolution, so we need to check that here,
> >     // too.
> >     if (promise->is<PromiseObject>() &&
> >         promise->as<PromiseObject>().state() != JS::PromiseState::Pending)
> >     {
> >         return true;
> >     }
> 
> and this assertion:
> https://searchfox.org/mozilla-central/rev/
> fd5c37f1dd9a0d1e327a6c6b4d81ea92f52c4330/js/src/builtin/Promise.cpp#1055
> >     MOZ_ASSERT(promise->state() == JS::PromiseState::Pending);
> 
> at least, it can happen with resolvePromise/rejectPromise testing functions.
> so they should throw error for this case.

I couldn't create a test case using resolvePromise/rejectPromise which triggered this assertion, but then I found the horrible settlePromiseNow() testing function. :-)


> if that's not a problem, maybe we could call ResolvePromise directly?

Actually we can call ResolvePromise instead of RejectMaybeWrappedPromise in a few places, but I planned to do this in a follow-up bug.


> 
> @@ +2690,5 @@
> >  }
> >  
> > +static MOZ_MUST_USE bool
> > +OriginalPromiseThenWithoutResolving(JSContext* cx, Handle<PromiseObject*> promise,
> > +                                    Handle<PromiseObject*> promiseToResolve)
> 
> maybe we could use template function with enum template parameter to merge
> optimized case and normal case, to reduce the code duplication?
> (not required for this bug tho)

The function arity is different between the two versions, so I don't think we can use templates here.

Comment 11

[André Bargull [:anba]]

Attachment: bug1473970-part4-skip-creating-resolving.patch

Update part 4 (was part 3) to address review comments.

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 8990741 [details] [diff] [review]
bug1473970-part3-docs-assertions-tests.patch

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

::: js/src/jit-test/tests/promise/debugger-reaction-does-not-resolve.js
@@ +1,2 @@
> +function newPromiseCapability() {
> +    var resolve, reject, promise = new Promise(function(r1, r2) {

interesting initialization :)

@@ +9,5 @@
> +function neverCalled() {
> +    // Quit with non-zero exit code to ensure a test suite error is shown,
> +    // even when this function is called within promise handlers which normally
> +    // swallow any exceptions.
> +    quit(1);

can you add some comment that explains the relation between debugger and this testcase?
(given that this testcase doesn't touch debugger itself, but the filename contains "debugger", for future reference)

Comment 13

[André Bargull [:anba]]

Attachment: bug1473970-part3-docs-assertions-tests.patch

Update part 3 to add a comment explaining the relationship to the debugger for jit-test/tests/promise/debugger-reaction-does-not-resolve.js.

Also add jit-test/tests/debug/Promise-race-dependent-promises.js to actually have a test case for this dummy promise reaction.

Comment 14

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2133169d4fc2576043c38a0f2f92d2af7b13925a

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92fa9b1525b1
Part 1: Check realm of Promise and "then" property for optimized resolve-thenable-job. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cd3b35b68fe
Part 2: Add a different PromiseResolveThenableJob kind for the default Promise.prototype.then case. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b2b4b7fec99
Part 3: Add documentation for PromiseReaction fields, strengthen assertions, and add tests. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/8719529f5d4d
Part 4: Skip creating the resolving functions in EnqueuePromiseResolveThenableBuiltinJob. r=arai

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/92fa9b1525b1
https://hg.mozilla.org/mozilla-central/rev/7cd3b35b68fe
https://hg.mozilla.org/mozilla-central/rev/0b2b4b7fec99
https://hg.mozilla.org/mozilla-central/rev/8719529f5d4d