1712424 - Use of the arguments object or spread operator significantly reduces JS performance

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Peiris]

Attachment: ff-args.html

If a function uses the special arguments object in one of several ways, or uses the spread operator, its performance is significantly reduced in Firefox, compared to Chrome.

A reduced test case is attached.

In Firefox 88:

iterations 50,000,000
normalResult 27.00ms
leakyArgumentsResult 639.00ms
spreadOpResult 852.00ms
indexedArgumentsResult 456.00ms
argLengthResult 24.00ms

In Firefox Nightly 90:

iterations 50,000,000
normalResult 24.00ms
leakyArgumentsResult 5341.00ms
spreadOpResult 882.00ms
indexedArgumentsResult 4892.00ms
argLengthResult 4591.00ms

In Chrome 90:

iterations 50,000,000
normalResult 38.85ms
leakyArgumentsResult 39.23ms
spreadOpResult 37.88ms
indexedArgumentsResult 39.31ms
argLengthResult 50.96ms

Performance varies when the profiler is enabled:

Firefox 88, profiler on:

https://share.firefox.dev/3uc9avg

iterations 50,000,000
normalResult 486.00ms
leakyArgumentsResult 1144.00ms
spreadOpResult 2529.00ms
indexedArgumentsResult 602.00ms
argLengthResult 606.00ms

Oddly, Nightly is faster in some cases when the profiler is enabled:

Firefox Nightly 90, profiler on:
https://share.firefox.dev/3f6uuhI

normalResult 27.00ms
leakyArgumentsResult 6206.00ms
spreadOpResult 2247.00ms
indexedArgumentsResult 617.00ms
argLengthResult 590.00ms

Comment 1

[André Bargull [:anba]]

Performance regressions in arguments usage is probably related to bug 1700443. Iain can you take a look?

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug is a defect, but please change it back in case of error.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1700443

Comment 4

[Iain Ireland [:iain]]

Thanks for reporting! The benchmark is very helpful.

The problem here is related to on-stack replacement. When I wrote scalar replacement for arguments (bug 1688033) I was concerned about merging the normal entry arguments object with the already-allocated OSR arguments object, so I disabled scalar replacement of arguments in functions using OSR. At the time, my hypothesis was that functions that the overlap between functions that use OSR and functions that benefit from arguments optimization is relatively small. What I didn't account for was inlining: we can OSR into a function that calls a smaller function in a loop, and that smaller function might have its own inlined copy of arguments.

In this set of testcases, we inline each of the tested functions into the event listener callbacks, and then OSR into the for loop. This prevents us from optimizing the inlined use of arguments.

Fortunately, with the benefit of hindsight, I think my concerns were unfounded. Scalar replacement gives up as soon as it sees a phi, so if there are two definitions for the arguments object (one in the normal entry and one in the OSR entry) then we won't try to scalar replace it. We can still go ahead and optimize inlined functions, which is a big performance win:

Before:

normalResult 25.22ms
leakyArgumentsResult 4528.50ms
spreadOpResult 7712.38ms
indexedArgumentsResult 3910.92ms
argLengthResult 3912.03ms

After:

normalResult 25.07ms
leakyArgumentsResult 461.51ms
spreadOpResult 723.75ms
indexedArgumentsResult 34.68ms
argLengthResult 57.15ms

So I think the easiest win here is to just remove that line.

Based on the reported results from Chrome, it looks like V8 is going even further when optimizing inlined arguments (and inlining spread calls) but this should be good enough for now.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1712424: Allow scalar replacement of inlined arguments in scripts with OSR entries r=jandem

When I started writing the arguments scalar replacement code, I was concerned about the interaction with OSR, so I disabled it for scripts with an OSR entry, because I didn't think it would matter. What I didn't think of was that this also prevented us from optimizing arguments for inlined functions.

I'm pretty confident that we could remove the check completely, and everything would still work out just fine. I've run into enough issues with split CFGs, though, that I think it's safer to only relax the check for MCreateInlinedArgumentsObject, and keep skipping MCreateArgumentsObject in compilations with OSR entries.

This gives 100x speedups in some cases on the microbenchmark attached to the bug. I've added a testcase to verify that we scalar-replace inlined arguments, but not the regular arguments.

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1712424: Remove obsolete jitoption r=jandem

I realized while writing the previous patch that --scalar-replace-arguments is no longer needed. (We can still turn off scalar replacement with --ion-scalar-replacement=off.)

Depends on D115952

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de3bd89c5a75
Allow scalar replacement of inlined arguments in scripts with OSR entries r=jandem
https://hg.mozilla.org/integration/autoland/rev/008b34997180
Remove obsolete jitoption r=jandem

Comment 8

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/de3bd89c5a75
https://hg.mozilla.org/mozilla-central/rev/008b34997180

Comment 9

[Brian Peiris]

Thanks for the quick turn-around on this!

I'd be remiss if I didn't mention that the original investigation and benchmark came from @takahirox (https://github.com/takahirox/js-arguments-test)

Out of curiosity, would performance regressions like this one be caught by automated performance tests, or do you primarily rely on users reporting them?

Comment 10

[Iain Ireland [:iain]]

We have a variety of performance benchmarks (eg), but none of them caught this particular regression. User reports, especially such well-documented ones, are always welcome!