Closed Bug 1303795 Opened 5 years ago Closed 5 years ago

Improve performance of Reflect.apply/construct

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- affected
firefox52 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Reflect.apply:
The self-hosted version of Reflect.apply should be small enough to make it applicable for inlining. Combined with the existing inlining support for Function.prototype.apply, we get a noticeable speed-up (up to 10x in micro benchmarks).

Reflect.construct:
The self-hosted version is at least 2x faster than the existing native version, so let's switch it too. Unfortunately it's necessary to write the same ugly switch-statement code like in Function.prototype.apply to avoid regressions when only few arguments are passed to construct(). Therefore I've added a SPREAD macro to builtin/SelfHostingDefines.h to improve readability, I hope this is okay with you.
Attached patch reflect-perf.patch (obsolete) — Splinter Review
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8792589 - Flags: review?(till)
I've used this micro benchmark to test the performance changes:
---
function bench_apply() {
    function f(){}
    function g(){}
    var args = [];
    var t = dateNow();
    for (var i = 0; i < 1000000; ++i) {
        Reflect.apply(f, null, args);
        Reflect.apply(g, null, args);
    }
    return dateNow() - t;
}

function bench_construct() {
    function f(){}
    function g(){}
    var args = [];
    var t = dateNow();
    for (var i = 0; i < 1000000; ++i) {
        Reflect.construct(f, args);
        Reflect.construct(g, args);
    }
    return dateNow() - t;
}

print("Reflect.apply:")
for(var i = 0; i < 10; ++i) print(bench_apply());

print("Reflect.construct:")
for(var i = 0; i < 10; ++i) print(bench_construct());
---
Comment on attachment 8792589 [details] [diff] [review]
reflect-perf.patch

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

This is great, thank you so much!

::: js/src/builtin/Reflect.js
@@ +65,5 @@
> +    // Step 4.
> +    if (!IsObject(argumentsList))
> +        ThrowTypeError(JSMSG_NOT_NONNULL_OBJECT, DecompileArg(1, argumentsList));
> +
> +    var args = CreateListFromArrayLikeForArgs(argumentsList);

Would it make sense to inline the IsPackedArray check here? In the vast majority of cases that'd mean CreateListFromArrayLikeForArgs never gets called and doesn't need to be inlined.

Something like
var args = IsPackedArray(argumentsList) ? argumentsList : CreateListFromArrayLikeForArgs(argumentsList);
Attachment #8792589 - Flags: review?(till) → review+
(In reply to Till Schneidereit [:till] from comment #3)
> Would it make sense to inline the IsPackedArray check here? In the vast
> majority of cases that'd mean CreateListFromArrayLikeForArgs never gets
> called and doesn't need to be inlined.

I can test this and if there's an observable difference, I'll update the patch.
Attached patch bug1303795.patchSplinter Review
Updated per review comments and to apply cleanly on inbound. Carrying r+ from Till.
Attachment #8792589 - Attachment is obsolete: true
Attachment #8801765 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17973c94a5ab
Self-host Reflect.apply and Reflect.construct. r=till
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/17973c94a5ab
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1358939
You need to log in before you can comment on or make changes to this bug.