1597988 - 2.03 - 2.24% kraken (windows10-64-shippable, windows7-32-shippable) regression on push 21f755c04005255c5305a13ad9087420e4489b7b (Mon November 18 2019)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Marian Raiciof [:marauder]]

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=21f755c04005255c5305a13ad9087420e4489b7b

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

2% kraken windows10-64-shippable opt e10s stylo 928.01 -> 948.81
2% kraken windows7-32-shippable opt e10s stylo 1,004.42 -> 1,026.45
2% kraken windows7-32-shippable opt e10s stylo 1,004.35 -> 1,024.78

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=24005

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/TestEngineering/Performance/Talos

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/TestEngineering/Performance/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/TestEngineering/Performance/Talos/RegressionBugsHandling

Comment 1

[Chris Fallin [:cfallin]]

I'll take a look at this. RIght now, (i) Perfherder is giving me 503s or just not loading; I think they have DB issues, and (ii) I don't have a Windows setup, but a colleague who managed to load the perf results told me which benchmark (imaging-darkroom in Kraken) is at issue, so I'll see what I can do, starting with setting up a Windows VM and build environment.

Comment 2

[Chris Fallin [:cfallin]]

Adding a blocking dependency on a Perfherder bug -- I can't view the subtests on the results above, so I can't compare to the earlier Linux tests we ran.

Comment 3

[Chris Fallin [:cfallin]]

So the problem is in (at least) imaging-darkroom. It appears to be an additional GC happening with the new change. Also visible on Linux, just (apparently) below threshold to trigger an alert (10% delta on individual test, vs. 16.4% on Win10 as seen here).

After building locally (linux/x86-64), here are two profiles:

• Commit just prior to the change: https://perfht.ml/3485GxI
• Commit in question: https://perfht.ml/2qn0tnm

The latter has a major GC; the former does not.

The change adds some memory overhead during parse, because it builds an extra intermediate data structure; this is fundamental to the approach and necessary for what we wanted to accomplish with the change (avoid GC alloc during parse by allocating some intermediate thing instead, and GC-alloc'ing later).

However, this overhead should go away after parse. I'll look into this more. If it has to do with the high-water mark of memory allocation, there is one thing we might be able to do (indirectly hold the ObjLiteralCreationData via pointer in the GC-things variant, so the variant doesn't grow as much).

Comment 4

[Chris Fallin [:cfallin]]

(For completeness, I should note the above is running a local checkout of Kraken that has been modified to only run the imaging-darkroom test; edit make-hosted.py to choose only kraken-1.1 suite, and edit tests/kraken-1.1/LIST to choose only one benchmark, then run make-hosted.py to build the local copy.)

Comment 5

[Chris Fallin [:cfallin]]

Attachment: Bug 1597988: Reduce memory overhead of ObjLiteralCreationData to avoid GCs. r=mgaudet

The patch associated with bug 1580246 created a small performance
regression on Kraken (imaging-darkroom test) due to memory overhead:

• Without b1580246 patch: https://perfht.ml/3485GxI
• With b1580246 patch: https://perfht.ml/2qn0tnm

The main difference above is the major GC that occurs. This appears to
be because of higher memory overhead during parse because the GC-things
list is bloated by including the large ObjLiteralCreationData in the
variant type.

This patch adds some indirection, storing a UniquePtr in the variant
instead. This is more in line with the other types in the variant (e.g.,
BigIntCreationData also just stores a pointer internally).

The resulting browser has the following profile on the same test:

• With this patch: https://perfht.ml/35dlsYq

Note that the major GC is avoided while the test is running.

Comment 6

[Chris Fallin [:cfallin]]

Try runs: baseline, just prior to b1580246 commit and with D54042 patch. Will post perfherder comparisons once they're done.

Comment 7

[Chris Fallin [:cfallin]]

Hmm -- the patch didn't seem to help.

Here is a Perfherder comparison between parent of original (b1580246) commit, and original commit + the patch here: Kraken, Raptor, Octane, AWSY. Kraken is still showing 2.28% worse on win10-64-shippable and 1.90% on win7-32-shippable (and 1.94% on linux64-shippable). As with the original patch, no other benchmarks are affected, though: Speedometer is 0.15% better (win10-64-shippable) or 0.01% worse (linux64-shippable), Octane is 0.30% worse (linux64), and AWSY shows JS heap 0.09% smaller (win10-64-shippable), 0.06% smaller (win7-64-shippable), or 0.16% smaller (linux64-shippable).

Overall, based on the profiler runs in comment 3 / comment 5 above, this seems to be a GC-timing issue. Locally, running in the js shell, the imaging-darkroom benchmark shows no regression at all (on linux64), which indicates that the core data structure effects (the way in which the object-literal code allocates objects and type-inference ObjectGroups) is unaffected. Rather, something about the slightly different timing, or longer parse time (which the benchmark doesn't count), or temporary allocations, is causing a major GC to start during the benchmark run, and once that happens, all bets are off.

As a result of that conclusion, and based on some discussion with my team yesterday, and also because this patch takes us closer to being able to do JS parses off-thread (which is a potentially big win for latency!) I think it might be best to simply accept this regression and leave the patch in. However, I'm open to other suggestions.

Comment 8

[(Away)]

As with the original patch, no other benchmarks are affected, though: Speedometer is 0.15% better (win10-64-shippable) or 0.01% worse (linux64-shippable), Octane is 0.30% worse (linux64), and AWSY shows JS heap 0.09% smaller (win10-64-shippable), 0.06% smaller (win7-64-shippable), or 0.16% smaller (linux64-shippable).

As a general practice I'd suggest reporting these as merely "uncertain", when the Perfherder indicator says low confidence. These values are so much within their respective margins of error, that sharing any number at all might be misinterpreted by a reader.

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

I think at this point I'd abandon the patch attached to this revision, just because I don't think it's the direction we want to go.

Comment 10

[Julien Cristau [:jcristau]]

Sounds like we won't have a fix for this in 72.

Comment 11

[Chris Fallin [:cfallin]]

It appears that the inlining regression in bug 1598347 might have been the culprit, as fixing it led to a 2% improvement in kraken (comment link).

Here's a Perfherder comparison between the baseline commit of this regression and the after-patch runs of the above:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=autoland&originalRevision=91697065e99f5b88ceaaf92af99ebdbbfc1dda88&newProject=autoland&newRevision=10b295d8c610b7807f45b04c7a20d9e387874b65&framework=1

So Kraken seems to be back to where it was before; caveat that there is a three-week gap between runs so the baseline may have moved a bit, but the sudden 2% improvement on the inlining fix appears to be real, and corresponds to this regression.

Comment 12

[Nicolas B. Pierron [:nbp] {away}]

https://phabricator.services.mozilla.com/D54042#1758792 :

Abandoning, as we decided not to go this route (and the regression seems to have been fixed by the later inlining patch in any case).

Based on the comment from Chris on the obsolete patch, it seems that we can close this bug.