Closed Bug 1386534 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: till, Assigned: till, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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 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?
(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 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? :)
Attachment #8892771 - Flags: review?(jdemooij)
(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);|.
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.)
Attachment #8892771 - Attachment is obsolete: true
Attachment #8899256 - Flags: review?(andrebargull)
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.
Attachment #8899256 - Flags: review?(andrebargull) → review+
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39271d578fa4
Use a C++ version of SpeciesConstructor when calling from C++. r=anba,rs=jonco
(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.
(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
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/184b518bdef8
Backed out changeset 39271d578fa4 for leaks a=backout CLOSED TREE
Blocks: 1393089
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b83ec454fdc8
Use a C++ version of SpeciesConstructor when calling from C++. r=anba,rs=jonco
A try run shows that this isn't responsible for the leaks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=386d468ed719c48a8b80381380600d720884058c
Flags: needinfo?(till)
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")
Flags: needinfo?(till)
Blocks: 1393712
Looks like we could give this one another try, now that bug 1395366 is fixed!
Depends on: 1395366
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7763254023
Use a C++ version of SpeciesConstructor when calling from C++. r=anba,rs=jonco
https://hg.mozilla.org/mozilla-central/rev/6e7763254023
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
See Also: → 1317481
See Also: 1317481
You need to log in before you can comment on or make changes to this bug.