Improve performance of Reflect.apply/construct

RESOLVED FIXED in Firefox 52

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: anba, Assigned: anba)

Tracking

(Depends on: 1 bug)

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 affected, firefox52 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8792589 [details] [diff] [review]
reflect-perf.patch
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8792589 - Flags: review?(till)
(Assignee)

Comment 2

2 years ago
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+
(Assignee)

Comment 4

2 years ago
(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.
(Assignee)

Comment 5

2 years ago
Created attachment 8801765 [details] [diff] [review]
bug1303795.patch

Updated per review comments and to apply cleanly on inbound. Carrying r+ from Till.
Attachment #8792589 - Attachment is obsolete: true
Attachment #8801765 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 6

2 years ago
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

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/17973c94a5ab
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1358939
You need to log in before you can comment on or make changes to this bug.