1323027 - 2.64% kraken (osx-10-10) regression on push 2f96ca59484d (Thu Dec 8 2016)

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Talos has detected a Firefox performance regression from push 2f96ca59484d. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  3%  kraken summary osx-10-10 opt     1478.84 -> 1517.86


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

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/Buildbot/Talos/Tests

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/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/Buildbot/Talos/RegressionBugsHandling

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

here is a link to the compare view:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=28d3c72eaecdc719fa4a01d5bb2d345b216070a9&newProject=mozilla-inbound&newRevision=2f96ca59484d38b4b6d2f3aca283001bc7f01029&originalSignature=2fbb12cea9acbb6ef8452323642a57a9d993ce79&newSignature=2fbb12cea9acbb6ef8452323642a57a9d993ce79&framework=1

the regression is in:
kraken imaging-gaussian-blur

:h4writer, I see you wrote the patches that caused the regression, is this expected or is there any work we can do to fix the regression?

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

oh, I see 1322724 from https://treeherder.mozilla.org/perf.html#/alerts?id=4482.  Possibly this is fixed?

Comment 3

[Guilherme Lima]

At least on AWFY, the patch from bug 1322724 didn't fix most of the regressions, including the one pointed out in this bug.

Comment 4

[Hannes Verschore [:h4writer]]

Status: Bug 1322724 fixed a little bit of the regression, but there seems to be more left looking at the improvements after the fix. I'll investigate this week. Currently traveling. I should be able to look at it Wednesday.

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Patch

I traced the kraken-gaussian regression to the "FoldTests" not applying cleanly anymore. An extra empty block is added in http://searchfox.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#2832 which "FoldTests" doesn't recognizes.

I could have fixed that logic to support an extra empty block in the true/false path, but I didn't see how that would scale. I've updated that logic before and there are potential other places that depend on it. Therefore I though it would be better to introduce a step that would remove such extra empty blocks.

1) No need to update the FoldTest logic to add support for empty blocks in between
2) Decreases the amount of blocks in a graph. Which means all other passes need to iterate less blocks. We could potentially call this again after GVN to remove even more of those blocks. (We often have a chain of empty blocks at the end of MIR)
3) Having these empty blocks could disable some optimizations because we didn't add code to also match empty blocks.
4) The IonBuilder CFG split caused some other places with extra empty blocks for convenience. This removes those too.
5) We can increase this logic to merge blocks with non-effectfull instructions. A next step would be to merge the "MFilterTypeSet" in the next block and have only one block if possible.

This fixed the regression locally. I even saw an improvement, but I don't think this will translate in a awfy gaussian win.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8818922 [details] [diff] [review]
Patch

Review of attachment 8818922 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0703ad673049
IonMonkey - Remove empty blocks, r=jandem

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0703ad673049

Comment 9

[Alison Shiue]

The improvement shows on https://treeherder.mozilla.org/perf.html#/alerts?id=4560, thanks.