Closed
Bug 1303795
Opened 8 years ago
Closed 8 years ago
Improve performance of Reflect.apply/construct
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: anba, Assigned: anba)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
27.33 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
Assignee | ||
Comment 2•8 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 3•8 years ago
|
||
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•8 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•8 years ago
|
||
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•8 years ago
|
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
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/17973c94a5ab
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•