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

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, enhancement)

Description

[André Bargull [:anba]]

---
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

Comment 1

[André Bargull [:anba]]

Attachment: bug1364363.patch

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.

Comment 2

[Tom S [:evilpie]]

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.

Comment 3

[Tom S [:evilpie]]

* "Especially because we should already use a simple bounds-check for "in" with packed arrays"

Comment 4

[André Bargull [:anba]]

Attachment: bug1364363.patch

Thanks for looking into the Ion optimizations! Here's the updated patch which simply removes the IsPackedArray branch.

Comment 5

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=85d65f02ddf1cf5fcd1cc12fc763bf525d2183ca

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/742a51ab32ce