Closed Bug 1364363 Opened 3 years ago Closed 2 years ago

Incorrect IsPackedArray optimizations in Array.prototype.indexOf and Array.prototype.lastIndexOf

Categories

(Core :: JavaScript: Standard Library, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file, 1 obsolete file)

---
function makeArray(array) {
    Object.setPrototypeOf(array, new Proxy(Array.prototype, {
        has(t, pk) {
            print(`Has:${String(pk)}`);
            return Reflect.has(t, pk);
        },
        get(t, pk, r) {
            print(`Get:${String(pk)}`);
            return Reflect.get(t, pk, r);
        },
    }));
    return array;
}

var array = makeArray([1, null, 3]);
var r = Array.prototype.indexOf.call(array, 100, {
    valueOf() {
        array.length = 0;
        return 0;
    }
});
print(r);

var array = makeArray([5, undefined, 7]);
var r = Array.prototype.lastIndexOf.call(array, 100, {
    valueOf() {
        array.length = 0;
        return 2;
    }
});
print(r);
---

Expected: Prints "Has"
Actual: Prints "Get"


We may be able to remove the IsPackedArray path, now that the "in" operator is better optimized. Especially for small arrays, the overhead of calling IsPackedArray outweighs the possible performance gains of omitting the "in" operator check.


function f1() {
    var a = Array(1000000).fill(0);
    var t = Date.now();
    for (var i = 0; i < 100;++i) a.indexOf(1);
    return Date.now() - t;
}

function f2() {
    var a = Array(10000).fill(0);
    var t = Date.now();
    for (var i = 0; i < 10000;++i) a.indexOf(1);
    return Date.now() - t;
}

function f3() {
    var a = Array(10).fill(0);
    var t = Date.now();
    for (var i = 0; i < 10000000;++i) a.indexOf(1);
    return Date.now() - t;
}

var s1 = 0, s2 = 0, s3 = 0;
for (var i = 0; i < 10; ++i) s1 += f1();
for (var i = 0; i < 10; ++i) s2 += f2();
for (var i = 0; i < 10; ++i) s3 += f3();
print(s1 / 10, s2 / 10, s3 / 10);

With IsPackedArray: 336.7 308.1 509.5
Without IsPackedArray: 333.7 303.7 331.6
Attached patch bug1364363.patch (obsolete) — Splinter Review
I'll create a follow-up bug to investigate whether or not it makes sense to remove the IsPackedArray fast-path now that the "in"-operator is more optimized.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8874393 - Flags: review?(evilpies)
I am convinced, let's remove the IsPackedArray check. Especially because we should already add use a simple bounds-check for in with pack arrays (http://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/js/src/jit/IonBuilder.cpp#12670), which should be hoisted outside the loop. Additionally we can't optimize away the else branch anymore, because of the additional condition.
* "Especially because we should already use a simple bounds-check for "in" with packed arrays"
Attached patch bug1364363.patchSplinter Review
Thanks for looking into the Ion optimizations! Here's the updated patch which simply removes the IsPackedArray branch.
Attachment #8874393 - Attachment is obsolete: true
Attachment #8874393 - Flags: review?(evilpies)
Attachment #8875187 - Flags: review?(evilpies)
Attachment #8875187 - Flags: review?(evilpies) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/742a51ab32ce
Remove the packed array check for Array.p.indexOf/lastIndexOf to fix a spec compliance issue. r=evilpie
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/742a51ab32ce
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.