Closed Bug 1295379 Opened 5 years ago Closed 5 years ago

Improve performance of various TypedArray built-ins


(Core :: JavaScript: Standard Library, defect)

Not set



Tracking Status
firefox51 --- wontfix
firefox52 --- fixed


(Reporter: anba, Assigned: anba)



(1 file, 3 obsolete files)

I've identified two performance problems affecting the self-hosted TypedArray methods:
- Calling Array.prototype.push on List objects is slow.
- Functions which use default arguments and also access |arguments| are slow.

Micro benchmark:
function test(fn) {
    var t = 0;
    for (var i = 0; i < 1000; ++i) {
        var d = dateNow();
        t += (dateNow() - d);
    return t;

function testProto(fn) {
    var ta = new Int8Array(10000);
    return test(() => fn(ta));

function testFrom() {
    var elements = new Array(10000);
    return test(() => Int8Array.from(elements));

for (var i = 0; i < 10; ++i) {
    // print(testProto(ta => ta.filter(() => true)));
    // print(testProto(ta =>, v) => v)));
    // print(testProto(ta => ta.forEach(() => {})));
    // print(testProto(ta => ta.every(() => true)));
    // print(testProto(ta => ta.some(() => false)));
    // print(testProto(ta => ta.find(() => false)));
    // print(testProto(ta => ta.findIndex(() => false)));

  before: ~2500 ms
  after: ~230 ms

  before: ~3450 ms
  after: ~180 ms
  before: ~350 ms
  after: ~40 ms

TypedArray.prototype.{forEach, every, some, find, findIndex}:
  before: ~300 ms
  after: ~20 ms
Assignee: nobody → andrebargull
Attached patch Part 1 - Workaround perf issues (obsolete) — Splinter Review
This patch provides workarounds for the problems identified in comment #0. Fixing the underlying issues is probably harder and out of my league, but if it's okay to workaround the current performance weak spots, we could apply this patch until a proper fix is available.
Attachment #8781309 - Flags: feedback?(evilpies)
Comment on attachment 8781309 [details] [diff] [review]
Part 1 - Workaround perf issues

Review of attachment 8781309 [details] [diff] [review]:

Was just gonna driveby the next paragraph, then I started looking, might as well just stamp, this is simple.  :-)

For what it's worth, explicitly having List as a thing is stupid and grody.  It was more or less fine when just Intl used it, because that needn't be super-fast and it was a first-cut implementation anyway.  But it's been awhile, and I guess actually-more-important stuff is now using it.  It's probably time to replace it with something real, tho I'm not sure what that should be.

V8 has intrinsics to get particular numeric arguments, rather than use |arguments|.  That lets them avoid some slowness that |arguments| might induce, such as what occurs when defaults/rests are present.  I would be in favor of having similar intrinsics in SpiderMonkey.
Attachment #8781309 - Flags: feedback?(evilpies) → feedback+
Attachment #8781309 - Flags: feedback+ → review+
(To be clear, on the policy question of taking this versus fixing underlying issues, and f+ versus r+, I think we take this.  And as far as arguments+defaults go, my recollection of semantics is that once you have that, things *necessarily* get slower because you *have* to reify an arguments object as a general rule, because the two occupy separate storage.  So that problem possibly isn't fixable.)
Comment on attachment 8781308 [details] [diff] [review]
Part 0 - Update TypedArray.from to match current draft spec

Remove part 0 - Updating the implementation to match the current spec should be done as part of bug 1122396.
Attachment #8781308 - Attachment is obsolete: true
Attached patch Workaround perf issues (obsolete) — Splinter Review
Updated patch so it longer depends on part 0, and also fixed the JS_SELF_HOSTED_FN length parameters in vm/TypedArrayObject.cpp. 

Carrying r+ from Waldo. (Someone needs to push the patch to Try for me, because I still haven't requested try-access...)
Attachment #8781639 - Flags: review+
Attachment #8781309 - Attachment is obsolete: true
Attached patch bug1295379.patchSplinter Review
Patch needed bit-unrotting, carrying r+ from Waldo.

Attachment #8781639 - Attachment is obsolete: true
Attachment #8798111 - Flags: review+
Keywords: checkin-needed
Pushed by
Avoid performance pitfalls in TypedArray built-ins. r=Waldo
Keywords: checkin-needed
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.
You need to log in before you can comment on or make changes to this bug.