1660409 - Figure out what to do with ArrayJoinResult

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P2)

Description

[Jan de Mooij [:jandem]]

This is a very specific optimization for join on arrays with length <= 1. I'm a bit worried about supporting this in Warp because it feels like over-specialization that could easily result in bailouts. However, if we don't transpile it, we'll use the CallGeneric call path which isn't great. Maybe we should just disable this stub if Warp is enabled...

Comment 1

[Jan de Mooij [:jandem]]

Another option is to call jit::ArrayJoin but that's just calling array_join anyway.

Comment 2

[André Bargull [:anba]]

Bug 1382837 has some numbers to explain why we optimise for length <= 1.

Comment 3

[Iain Ireland [:iain]]

It looks like this was added for Speedometer, but I don't see any perf numbers reported in bug 1382837. If we disable this stub, does it affect Speedometer performance? If it doesn't, then maybe we can just eliminate it entirely.

Comment 4

[André Bargull [:anba]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1382837#c4, https://bugzilla.mozilla.org/show_bug.cgi?id=1382837#c7, and https://bugzilla.mozilla.org/show_bug.cgi?id=1382837#c9 mention this is also useful outside of benchmarks, for example it applies to GDocs, too.

Comment 5

[Jan de Mooij [:jandem]]

Another option is to restrict to packed arrays and do a VM call to ArrayJoinDenseKernel, but that doesn't handle objects/symbols... Handling objects requires AutoCycleDetector and if we add that it's not clear how much faster it still is over array_join.

Comment 6

[André Bargull [:anba]]

What if we change the CacheIR implementation to more closely match the inline code path in CodeGenerator? I.e. handle the zero and one element case inline and for all other cases perform a VM call?
-> https://treeherder.mozilla.org/#/jobs?repo=try&revision=176a02993e373bfa6f7b8c4629b1681ea1348d83

Comment 7

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #6)

What if we change the CacheIR implementation to more closely match the inline code path in CodeGenerator? I.e. handle the zero and one element case inline and for all other cases perform a VM call?
-> https://treeherder.mozilla.org/#/jobs?repo=try&revision=176a02993e373bfa6f7b8c4629b1681ea1348d83

That looks great. Thanks!

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1660409 - Part 1: Change ArrayJoin to an unshared op. r=jandem!

Change to unshared CacheIR op in preparation for part 3.

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1660409 - Part 2: Add LoadConstantString CacheIR op. r=jandem!

Add a new CacheIR op to load a constant string in preparation for part 3.

Depends on D88561

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1660409 - Part 3: Change CacheIR ArrayJoin to use a VMCall instead of a FailurePath. r=jandem!

Use a CallVM instead of a FailurePath to match the Ion optimised path. This
change will allow part 4 to use the existing MArrayJoin call for Warp.

Depends on D88562

Comment 11

[André Bargull [:anba]]

Attachment: Bug 1660409 - Part 4: Transpile ArrayJoin. r=jandem!

Depends on D88564

Comment 12

[Jan de Mooij [:jandem]]

You read my mind :) I was just looking at some micro-benchmarks for this and, not surprisingly, --warp is indeed 6x slower than Baseline (--no-ti) on the one below.

function f() {
    var arr = [];
    var res = "";
    var t = dateNow();
    for (var i = 0; i < 10_000_000; i++) {
        res = arr.join(",");
    }
    print(dateNow() - t);
    print(res);
}
f();

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29976a2bca84
Part 1: Change ArrayJoin to an unshared op. r=jandem
https://hg.mozilla.org/integration/autoland/rev/fa309b2541bf
Part 2: Add LoadConstantString CacheIR op. r=jandem
https://hg.mozilla.org/integration/autoland/rev/bab7bab23941
Part 3: Change CacheIR ArrayJoin to use a VMCall instead of a FailurePath. r=jandem

Comment 14

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/29976a2bca84
https://hg.mozilla.org/mozilla-central/rev/fa309b2541bf
https://hg.mozilla.org/mozilla-central/rev/bab7bab23941

Comment 15

[K Carter]

Just curious, but I see a patch in Lando not showing as landed yet the bug is closed. Is this intended?

Comment 16

[André Bargull [:anba]]

(In reply to K Carter from comment #15)

Just curious, but I see a patch in Lando not showing as landed yet the bug is closed. Is this intended?

Ah, thanks for noticing! I didn't set the "check-in needed" flag at the top of stack, so the last commit wasn't committed.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58bf2354ce80
Part 4: Transpile ArrayJoin. r=jandem

Comment 18

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/58bf2354ce80