Use of the arguments object or spread operator significantly reduces JS performance
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr78 | --- | unaffected |
firefox88 | --- | unaffected |
firefox89 | --- | unaffected |
firefox90 | --- | fixed |
People
(Reporter: bpeiris, Assigned: iain)
References
(Regression)
Details
(Keywords: regression)
Attachments
(3 files)
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•4 years ago
|
||
Performance regressions in arguments
usage is probably related to bug 1700443. Iain can you take a look?
Updated•4 years ago
|
Comment 2•4 years ago
|
||
Comment 3•4 years ago
|
||
Set release status flags based on info from the regressing bug 1700443
Updated•4 years ago
|
Assignee | ||
Comment 4•4 years ago
|
||
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.
Assignee | ||
Comment 5•4 years ago
|
||
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.
Updated•4 years ago
|
Assignee | ||
Comment 6•4 years ago
|
||
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 8•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/de3bd89c5a75
https://hg.mozilla.org/mozilla-central/rev/008b34997180
Reporter | ||
Comment 9•4 years ago
|
||
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?
Assignee | ||
Comment 10•4 years ago
|
||
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!
Description
•