1825014 - Consider not allocating arguments objects in self-hosted code in interpreter/Baseline

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Jan de Mooij [:jandem]]

Many of the perf-sensitive builtins like ArrayForEach and ArrayMap use arguments.length and arguments[n] so they currently require us to allocate an arguments object on every call. Ion is able to get rid of those allocations with scalar replacement, but this still adds overhead to the interpreter and Baseline tiers.

I'm prototyping a patch to replace those two uses with separate intrinsics that lower to specialized bytecode ops that just read the values directly out of the frame. Hopefully this will cover all arguments uses in self-hosted code (I converted only the perf-sensitive ones for now) so we can then assert we never need to allocate an arguments object in self-hosted functions.

My prototype patch improves the ArrayMap micro-benchmark below with --no-ion from 258 ms to 188 ms. A similar Reflect.get micro-benchmark is about 3.7x faster with --no-ion. I'm still waiting for Speedometer results.

function f() {
    var arr = [1, 2, 3];
    var add1 = x => x + 1;
    var t = new Date;
    for (var i = 0; i < 1_000_000; i++) {
        arr = arr.map(add1);
    }
    print(new Date - t);
    return arr;
}
f();

Comment 1

[André Bargull [:anba]]

We could also change some built-ins to use default parameters instead of using arguments. I've prototyped a similar change last year (https://treeherder.mozilla.org/jobs?repo=try&revision=2fb8ecd0c61966b3c0f03707ae789ddc01c07c4c), because when we failed to scalar replace arguments in real-world code (twitch, reddit, etc), it was most often caused by OSR entries to self-hosted built-ins like ArrayMap.

Comment 2

[Jan de Mooij [:jandem]]

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

We could also change some built-ins to use default parameters instead of using arguments. I've prototyped a similar change last year (https://treeherder.mozilla.org/jobs?repo=try&revision=2fb8ecd0c61966b3c0f03707ae789ddc01c07c4c), because when we failed to scalar replace arguments in real-world code (twitch, reddit, etc), it was most often caused by OSR entries to self-hosted built-ins like ArrayMap.

Using default parameters would be a good follow-up change I think. I see you added the same ArgumentsLength intrinsic and the code to read an argument is also pretty straight-forward, so I think we should do that now and ban arguments in all self-hosted code.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1825014 part 1 - Assert GetFrameArgument index is in bounds in debug builds. r?iain!

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1825014 part 2 - Implement ArgumentsLength() and GetArgument(i) intrinsics. r?iain!

These can be used to replace uses of arguments.length and arguments[i] in
self-hosted code.

The frontend emits specialized bytecode ops for those instructions that access
the frame directly. This means we no longer have to allocate an arguments object in
the interpreter and Baseline for self-hosted functions. This speeds up many
perf-sensitive builtins such as ArrayMap.

Later patches convert all arguments uses in self-hosted code and add assertions to
ensure we never create an arguments object for any self-hosted function.

Depends on D173951

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1825014 part 3 - Convert self-hosted code to use the new intrinsics. r?iain!

Depends on D173952

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1825014 part 4 - Use a rest parameter instead of arguments for some error-throwing functions. r?iain!

This eliminates the remaining uses of arguments in self-hosted code.

These functions are called from C++ code. I'll see if we can remove them in a follow-up bug.

Depends on D173953

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1825014 part 5 - Assert we never create an arguments object for self-hosted code. r?iain!

Depends on D173954

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd7be783293a
part 1 - Assert GetFrameArgument index is in bounds in debug builds. r=iain
https://hg.mozilla.org/integration/autoland/rev/c10cc8b5969d
part 2 - Implement ArgumentsLength() and GetArgument(i) intrinsics. r=iain
https://hg.mozilla.org/integration/autoland/rev/6e5174d94f03
part 3 - Convert self-hosted code to use the new intrinsics. r=iain
https://hg.mozilla.org/integration/autoland/rev/a3d38cc19d96
part 4 - Use a rest parameter instead of arguments for some error-throwing functions. r=iain
https://hg.mozilla.org/integration/autoland/rev/9eb35ca6f1a3
part 5 - Assert we never create an arguments object for self-hosted code. r=iain

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/cd7be783293a
https://hg.mozilla.org/mozilla-central/rev/c10cc8b5969d
https://hg.mozilla.org/mozilla-central/rev/6e5174d94f03
https://hg.mozilla.org/mozilla-central/rev/a3d38cc19d96
https://hg.mozilla.org/mozilla-central/rev/9eb35ca6f1a3