933301 - [].push.apply is very slow

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

jQuery does this, see bug 922053. Small shell testcase below.

d8:  35 ms
js: 289 ms

The problem is that we don't inline this push call so we spend a ton of time in the VM, doing TI stuff under setDenseElementWithType.

We can try to speed that up, try to self-host Array.prototype.push or try to inline/optimize push.apply in IonMonkey.

function test() {
    var arr = [];
    for (var i=0; i<100; i++)
        arr[i] = {};
    var t = new Date;
    for (var i=0; i<70000; i++) {
        var res = [];
        res.push.apply(res, arr);
    }
    print(new Date - t);
}
test();

Comment 1

[Till Schneidereit [:till]]

I'll take a stab at self-hosting this.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: WIP

This patch changes setDenseElementWithType to check the new element's type against the previous element's type. The idea is that if they match we don't have to do the slow AddTypePropertyId dance.

This improves our time to 112 ms. Still > 2x slower than Chrome but also > 2x faster than before. I will see if this actually helps jQuery and how often this fast-path hits on other benchmarks/websites.

Comment 3

[Till Schneidereit [:till]]

Attachment: basic self-hosted Array#push, with benchmark

Ok, that proved not to be worth it. With the attached self-hosted version (and integrated benchmark), I see a win of ~10% - far less than jandem's wip patch. I didn't even test normal [].push() usage, which is probably worse with the self-hosted version.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Till Schneidereit [:till] from comment #3)
> Ok, that proved not to be worth it. With the attached self-hosted version
> (and integrated benchmark), I see a win of ~10% - far less than jandem's wip
> patch.

Thanks for testing. I think for this to be fast Ion needs to inline apply(x, array) like it inlines apply(x, arguments)...

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

On Octane this avoids the AddTypePropertyId call in 81% of the cases, for Kraken this is 90% and for Sunspider it's 97%.

I only did this for setDenseElementWithType for now, we can do the same for other methods if needed.

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a99ffa036654

(Try: https://tbpl.mozilla.org/?tree=Try&rev=f43d7e939533)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a99ffa036654