1386534 - Add C++ version of SpeciesConstructor to speed up common uses from C++ builtins

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Till Schneidereit [:till]]

SpeciesConstructor is used by various C++ builtins, currently causing a call out to self-hosted code. For ArrayBuffer, there's an optimization to avoid this call in some circumstances. The attached patch adds a C++ port of SpeciesConstructor, generalizes the ArrayBuffer optimizations to be usable by other builtins, and makes use of the optimization for Promises.

The following micro-benchmark goes from ~150ms to ~90ms for me:
function bench() {
  var p;
  var t1 = Date.now();
  var p0 = Promise.resolve(1);
  function cb() {};
  for (var i = 100000; i--;)
    p = p0.then(cb);
  console.log(Date.now() - t1);
  return p;
}

bench();

Comment 1

[Till Schneidereit [:till]]

Attachment: Use a C++ version of SpeciesConstructor when calling from C++

Comment 2

[André Bargull [:anba]]

Comment on attachment 8892771 [details] [diff] [review]
Use a C++ version of SpeciesConstructor when calling from C++

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

Just left some comments here, because I wanted to see what this bug does, being a prerequisite for bug 1342070 and all that, but I didn't do thorough review!

::: js/src/jsobj.cpp
@@ +3996,3 @@
>  bool
> +js::SpeciesConstructor(JSContext* cx, HandleObject obj, HandleValue defaultCtor,
> +                       MutableHandleValue pctor, HandleFunction originalSpeciesFun)

Pre-existing:
I feel like we should change |HandleValue defaultCtor| to |HandleObject defaultCtor| and |MutableHandleValue pctor| to |MutableHandleObject pctor|, so we don't need to convert between object-value-object all the time.

@@ +4006,5 @@
> +    // - obj.constructor[@@species]] can be retrieved without side-effects.
> +    // - obj.constructor[@@species]] is the builtin's original @@species
> +    //   getter.
> +    RootedValue ctor(cx);
> +    if (!IsWrapper(obj)) {

We could drop the extra IsWrapper check, because GetPropertyPure will also fail when any proxies are present.

@@ +4043,5 @@
> +    // Step 5.
> +    RootedObject ctorObj(cx, &ctor.toObject());
> +    RootedValue s(cx);
> +    RootedId speciesId(cx, SYMBOL_TO_JSID(cx->wellKnownSymbols().species));
> +    if (!GetProperty(cx, ctorObj, ctorObj, speciesId, &s))

Nit: |GetProperty(cx, ctorObj, ctor, speciesId, &s)| to avoid rooting |ctorObj| again in GetProperty.

@@ +4066,5 @@
> +//    FixedInvokeArgs<2> args(cx);
> +//    args[0].setObject(*obj);
> +//    args[1].set(defaultCtor);
> +//    return CallSelfHostedFunction(cx, cx->names().SpeciesConstructor, UndefinedHandleValue, args,
> +//                                  pctor);

Left-over comments.

::: js/src/jsobj.h
@@ +1419,5 @@
>  TestIntegrityLevel(JSContext* cx, HandleObject obj, IntegrityLevel level, bool* resultp);
>  
>  extern bool
> +SpeciesConstructor(JSContext* cx, HandleObject obj, HandleValue defaultCtor,
> +                   MutableHandleValue pctor, HandleFunction originalSpeciesFun);

I think I'd prefer to keep |MutableHandleValue pctor| as the last parameter.

::: js/src/vm/TypedArrayObject.cpp
@@ +1085,3 @@
>          return false;
>  
> +    RootedFunction originalSpecies(cx, SelfHostedFunction(cx, cx->names().ArrayBufferSpecies));

How much does it cost to call SelfHostedFunction compared to using a predicate function for SpeciesConstructor?

Comment 3

[Till Schneidereit [:till]]

(In reply to André Bargull from comment #2)
> Comment on attachment 8892771 [details] [diff] [review]
> Use a C++ version of SpeciesConstructor when calling from C++
> 
> Review of attachment 8892771 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just left some comments here, because I wanted to see what this bug does,
> being a prerequisite for bug 1342070 and all that, but I didn't do thorough
> review!

Note that this patch makes both Promise and ArrayBuffer use the new mechanism instead of its own special handling, so it should be possible to judge its merits on their own. Bug 1342070 only builds on this patch because I didn't want to uselessly rebase it now and then rebase it back again once this lands.

> > +    RootedFunction originalSpecies(cx, SelfHostedFunction(cx, cx->names().ArrayBufferSpecies));
> 
> How much does it cost to call SelfHostedFunction compared to using a
> predicate function for SpeciesConstructor?

Can you explain a bit more what the alternative you're proposing is? (I can say right now that SelfHostedFunction is quite cheap: it boils down to a bit of pointer chasing and then a pure shape lookup, at least once the self-hosted function is cloned.)

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8892771 [details] [diff] [review]
Use a C++ version of SpeciesConstructor when calling from C++

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

Sorry for the delay. André is probably more familiar with the details of this and already posted some comments. Maybe post a new patch and r? him? :)

Comment 5

[André Bargull [:anba]]

(I forgot to cc myself to this bug, so I didn't see your question yesterday. Sorry!)


(In reply to Till Schneidereit [:till] from comment #3)
> > > +    RootedFunction originalSpecies(cx, SelfHostedFunction(cx, cx->names().ArrayBufferSpecies));
> > 
> > How much does it cost to call SelfHostedFunction compared to using a
> > predicate function for SpeciesConstructor?
> 
> Can you explain a bit more what the alternative you're proposing is? (I can
> say right now that SelfHostedFunction is quite cheap: it boils down to a bit
> of pointer chasing and then a pure shape lookup, at least once the
> self-hosted function is cloned.)

I was thinking about changing the signature of SpeciesConstructor to:

SpeciesConstructor(JSContext* cx, HandleObject obj, HandleValue defaultCtor,
                   bool (*isDefaultSpecies)(JSContext*, JSFunction*), MutableHandleValue pctor)


TypedArrayObject.cpp would then define this IsArrayBufferSpecies predicate to test for ArrayBuffer[@@species]:

IsArrayBufferSpecies(JSContext* cx, JSFunction* species)
{
    return IsSelfHostedFunctionWithName(species, cx->names().ArrayBufferSpecies);
}

And call SpeciesConstructor with |SpeciesConstructor(cx, wrappedObj, defaultCtor, IsArrayBufferSpecies, ctor);|.

Comment 6

[Till Schneidereit [:till]]

Attachment: Use a C++ version of SpeciesConstructor when calling from C++, v2

Thank you for the review. This new version should address your feedback. It also includes a port of Promise[@@species] to C++, because there really isn't a good reason for having it in JS (and I'll remove Promise.js in bug 1342070.)

Comment 7

[André Bargull [:anba]]

Comment on attachment 8899256 [details] [diff] [review]
Use a C++ version of SpeciesConstructor when calling from C++, v2

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

LGTM, thanks!

::: js/src/builtin/Promise.cpp
@@ +2220,5 @@
>  
> +static bool
> +IsPromiseSpecies(JSContext* cx, JSFunction* species)
> +{
> +    return species && species->maybeNative() == Promise_static_species;

"species &&" could be removed, because it's already tested in js::SpeciesConstructor, right?

::: js/src/jsobj.cpp
@@ +52,4 @@
>  #include "vm/Interpreter.h"
>  #include "vm/ProxyObject.h"
>  #include "vm/RegExpStaticsObject.h"
> +#include "vm/SelfHosting.h"

#include no longer needed?

@@ +4022,5 @@
> +    // Step 1 (implicit).
> +
> +    // Fast-path for steps 2 - 8. Applies if all of the following conditions
> +    // are met:
> +    // - The object isn't a wrapper.

I'd probably remove this bullet point, because it's not explicitly tested in the if-statement, but only checked indirectly through GetPropertyPure.

@@ +4024,5 @@
> +    // Fast-path for steps 2 - 8. Applies if all of the following conditions
> +    // are met:
> +    // - The object isn't a wrapper.
> +    // - obj.constructor can be retrieved without side-effects.
> +    // - obj.constructor[@@species]] can be retrieved without side-effects.

Unbalanced brackets here and below.

@@ +4044,3 @@
>  
> +    // Step 2.
> +    if (!GetProperty(cx, obj, obj, cx->names().constructor, &ctor))

Theoretically we could reuse the result of GetPropertyPure, maybe for a follow-up bug?

@@ +4073,5 @@
> +        return &s.toObject();
> +
> +    // Step 8.
> +    JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_NOT_CONSTRUCTOR,
> +                              "@@species property of object's constructor");

Pre-existing: Maybe we should change the error message to use "[Symbol.species]" instead of "@@species"? Similar to our error message for iterators where we're using "[Symbol.iterator]() returned a non-object value". Maybe also for a follow-up bug?

::: js/src/jsobj.h
@@ +1418,4 @@
>  extern bool
>  TestIntegrityLevel(JSContext* cx, HandleObject obj, IntegrityLevel level, bool* resultp);
>  
> +extern JSObject*

MOZ_MUST_USE here and below.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39271d578fa4
Use a C++ version of SpeciesConstructor when calling from C++. r=anba,rs=jonco

Comment 9

[Till Schneidereit [:till]]

(In reply to André Bargull from comment #7)

Thank you for the review!
> > +    if (!GetProperty(cx, obj, obj, cx->names().constructor, &ctor))
> 
> Theoretically we could reuse the result of GetPropertyPure, maybe for a
> follow-up bug?

I did that and got an rs from jonco for it, so I folded it into this patch.

> > +    JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_NOT_CONSTRUCTOR,
> > +                              "@@species property of object's constructor");
> 
> Pre-existing: Maybe we should change the error message to use
> "[Symbol.species]" instead of "@@species"? Similar to our error message for
> iterators where we're using "[Symbol.iterator]() returned a non-object
> value". Maybe also for a follow-up bug?

I agree, and folded this into the patch, too.

Comment 10

[André Bargull [:anba]]

(In reply to Till Schneidereit [:till] from comment #9)
> I did that and got an rs from jonco for it, so I folded it into this patch.
> 
> [....]
> 
> I agree, and folded this into the patch, too.

U+1F44D (THUMBS UP SIGN)   :-D

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed this out for leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=125342059&repo=mozilla-inbound

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/184b518bdef8
Backed out changeset 39271d578fa4 for leaks a=backout CLOSED TREE

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b83ec454fdc8
Use a C++ version of SpeciesConstructor when calling from C++. r=anba,rs=jonco

Comment 14

[Till Schneidereit [:till]]

A try run shows that this isn't responsible for the leaks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=386d468ed719c48a8b80381380600d720884058c

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out bug 1342050 and bug 1386534 on suspicion of letting cgc's js/src/jit-test/tests/modules/missing-export-offthread.js fail (the Try run with the failure contained both patches):

bug 1342050: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0a60ff0706d347c6156bc956713f25aecd4e60
bug 1386534: https://hg.mozilla.org/integration/mozilla-inbound/rev/443391e0208aff5594b5a9c5a0cc443d5918e3b5

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b9e3244295018feec4c99bd2e51b8374eb0ab44e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://queue.taskcluster.net/v1/task/H0wSUI-iQ6WCcMDc3aUqmg/runs/0/artifacts/public/logs/live_backing.log

[task 2017-08-25T12:24:28.420647Z] TEST-PASS | js/src/jit-test/tests/v8-v5/check-regexp.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off")
[task 2017-08-25T12:24:39.949030Z] Exit code: -9
[task 2017-08-25T12:24:39.949106Z] TIMEOUT - modules/missing-export-offthread.js
[task 2017-08-25T12:24:39.949165Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/modules/missing-export-offthread.js | Timeout (code -9, args "--baseline-eager")

Comment 16

[André Bargull [:anba]]

Looks like we could give this one another try, now that bug 1395366 is fixed!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7763254023
Use a C++ version of SpeciesConstructor when calling from C++. r=anba,rs=jonco

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6e7763254023