1412200 - Optimize the case when "then" property is Promise.prototype.then

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

ResolvePromiseInternal gets "then" property from the thenable object, and passes the value to job, but in most case the "then" property is Promise.prototype.then.

I think, we could skip the following:
  * get "then" property in some case (if the shape is same every time, see also bug 1410695)
  * store "then" callable object to slot (this can avoid post barrier)
  * call on the "then" function with Call+CallArgs

Comment 1

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #0)
> ResolvePromiseInternal gets "then" property from the thenable object, and
> passes the value to job, but in most case the "then" property is
> Promise.prototype.then.

if "then" property exists.
(sorry I forgot to think about other cases)

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Do not store the then property into thenable job if it is the original Promise.prototype.then.

Stopped storing "then" property when it's the original Promise#then from the same compartment,
and instead directly call Promise#then implementation for such case.

performance comparison on bug 1393712's case:
  before: 4745 [ms]
  after:  4545 [ms]

Comment 3

[André Bargull [:anba]]

Comment on attachment 8988413 [details] [diff] [review]
Do not store the then property into thenable job if it is the original Promise.prototype.then.

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

Thanks, looks reasonable!

::: js/src/builtin/Promise.cpp
@@ +27,5 @@
>  #include "vm/NativeObject-inl.h"
>  
>  using namespace js;
>  
> +using mozilla::Maybe;

Please also add |#include "mozilla/Maybe.h"|.

Hmm wait, the local style in Promise.cpp seems to be using the full qualified name when accessing mozilla::Maybe, so maybe stick with that here too instead of adding a |using| declaration?

@@ +40,5 @@
>  enum PromiseHandler {
>      PromiseHandlerIdentity = 0,
>      PromiseHandlerThrower,
>  
> +    // Originall Promise.prototype.then which comes from the same compartment.

Typo: Originall -> Original

Do we need "same compartment" or "same realm"? (Because of bug 1357862.)

@@ +606,5 @@
>      if (!IsCallable(thenVal))
>          return FulfillMaybeWrappedPromise(cx, promise, resolutionVal);
>  
> +    // If the `then` property is the original Promise.prototype.then from the
> +    // same compartment, we skip storing/calling it.

With bug 1357862, this may need to change to check the current realm, but it doesn't seem like :jandem already started tackling the various "if the object is not a wrapper, then it's from the current realm" checks we use all over the place.

@@ +1315,5 @@
>  
>      // Step 1.
>      RootedObject resolveFn(cx);
>      RootedObject rejectFn(cx);
>      if (!CreateResolvingFunctions(cx, promise, &resolveFn, &rejectFn))

This makes me wonder if we can avoid creating the resolving functions, when we know |then| is the original `Promise.prototype.then`. But I'm not sure about it, I'd need to dig deeper into the Promise spec again to say for sure...

@@ +1333,5 @@
> +        RootedValue resolveVal(cx, ObjectValue(*resolveFn));
> +        RootedValue rejectVal(cx, ObjectValue(*rejectFn));
> +
> +        // Same as above, we return immediately on success.
> +        if (Promise_then_impl(cx, thenable, resolveVal, rejectVal, &rval, true))

Is |rvalUsed = true| necessary here, because the returned value |rval| is never used, is it?

Comment 4

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

(In reply to André Bargull [:anba] from comment #3)
> Do we need "same compartment" or "same realm"? (Because of bug 1357862.)

Yeah, it needs same realm (same global object).
fixed the comment.

> @@ +1333,5 @@
> > +        RootedValue resolveVal(cx, ObjectValue(*resolveFn));
> > +        RootedValue rejectVal(cx, ObjectValue(*rejectFn));
> > +
> > +        // Same as above, we return immediately on success.
> > +        if (Promise_then_impl(cx, thenable, resolveVal, rejectVal, &rval, true))
> 
> Is |rvalUsed = true| necessary here, because the returned value |rval| is
> never used, is it?

Good catch.
changed to false.

Comment 5

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9877b8bc201ccdba981383b536bfb017cebb76ff
Bug 1412200 - Do not store the then property into thenable job if it is the original Promise.prototype.then. r=anba

Comment 6

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9877b8bc201c

Comment 7

[Jens Hausdorf :jens1o [ni? me]]

Are there any numbers on how faster it is now?

Comment 8

[Tooru Fujisawa [:arai]]

(In reply to Jens Hausdorf from comment #7)
> Are there any numbers on how faster it is now?

comment #2