1169756 - 4% Linux* tsvgx regression on Mozilla-Inbound-Non-PGO on May 26, 2015 from push 507b6aba4555

Status: RESOLVED WONTFIX

(Core :: Panning and Zooming, defect)

Description

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

Talos has detected a Firefox performance regression from your commit 507b6aba4555 in bug 889085.  We need you to address this regression.

This is a list of all known regressions and improvements related to your bug:
http://alertmanager.allizom.org:8080/alerts.html?rev=507b6aba4555&showAll=1

On the page above you can see Talos 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, please see: https://wiki.mozilla.org/Buildbot/Talos/Tests#tsvg.2C_tsvgx

Reproducing and debugging the regression:
If you would like to re-run this Talos test on a potential fix, use try with the following syntax:
try: -b o -p linux -u none -t svgr  # add "mozharness: --spsProfile" to generate profile data

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code

Then run the following command from the directory where you set up Talos:
talos --develop -e <path>/firefox -a tsvgx

Making a decision:
As the patch author we need your feedback to help us handle this regression.
*** Please let us know your plans by Monday, or the offending patch will be backed out! ***

Our wiki page oulines the common responses and expectations:
https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling

Comment 1

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

I did a few retriggers:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=a7c9a6e1394e&tochange=93943a21b457&filter-searchStr=Ubuntu%20HW%2012.04%20x64%20mozilla-inbound%20talos%20svgr

svgx went from 280-289 range up to 288-297 range.

there are a series of patches landed (for the same bug):
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d551aa12ebb1&tochange=507b6aba4555

:dvander, can you take a look at this and see if we can fix this or explain it?

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

These patches only affect APZ, so it seems unlikely any regression here is really related.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

It's still possible though, and I think you are on the hook for proving that they are not assuming the range Joel got is correct. A few try pushes to bisect within the four patches might be a cheap first step.

Comment 4

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

I did a series of pushes (hg update <rev>):

prior to push d551aa12ebb1 : https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1f918e801f4
first patch   caa1f9974731 : https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb95c7e0e8e7
second patch  498b1d8f9d71 : https://treeherder.mozilla.org/#/jobs?repo=try&revision=691afd429c15
third patch   21393b5c6963 : https://treeherder.mozilla.org/#/jobs?repo=try&revision=e38fe4e94c7a
fourth patch  507b6aba4555 : https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cf0599b555e

in this case the fourth patch has everything applied.

we will need to retrigger once the builds are done.  I prefer 6 data points for each one.

Comment 5

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

ok, 498b1d8f9d71 is where we increase our numbers:
http://hg.mozilla.org/integration/mozilla-inbound/rev/498b1d8f9d71

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Certainly looks like that patch might have affected non-APZ codepaths.

Comment 7

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

Can we get a timeline for fixing this?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

dvander, let me know if you have cycles to look into this. If not maybe tn or mstange could, or I can look as well (on Monday).

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

I won't have time until after Whistler, at least. This doesn't look too high priority since it's a small delta, Linux-only.

Comment 10

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

:mstange, could you take a look at this apz patch:
http://hg.mozilla.org/integration/mozilla-inbound/rev/498b1d8f9d71

and see why it might be affecting linux tsvgx performance.  I only call you out as :kats mentioned you might be a good candidate this upcoming week to look at the issue.  Maybe someone else can jump on it.  We should fix this before the next uplift (June 29th)

Comment 11

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

this bug is going silent- I want to make sure we have a decision on this before we uplift to aurora (monday after whistler)

Comment 12

[Kartikaya Gupta (email:[e-mail])]

I'll do some try pushes to see if I can determine the problem.

Comment 13

[Markus Stange [:mstange]]

Thanks! Can you also do two try pushes (before and after) with "mozharness: --spsProfile"? I'm curious whether the profiles show the problem.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

So I looked at the changes in the regressing patch (498b1d8f9d71) and did a couple of try pushes, reverting the main changes in that patch. The revert that I made at [1] was the winner, and fixed the regression (you can compare [2] and [3] to see this; [2] is the try push that joel did at 498b1d8f9d71 and [3] is that plus my revert).

The at some point during the tsvgx test, !mShouldBuildScrollableLayer || mIsScrollableLayerInRootContainer is evaluating to true and the change dvander made causes the *aClipRect out-param to not get populated in this case. However the next patch in the original bug [4] removes that out-param entirely, and moves some of the code around. If my interpretation of this change is correct, then my new try push at [5], based on m-c, should fix the regression.

[1] https://hg.mozilla.org/try/rev/3570daa8fb57
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=691afd429c15
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a786479f4f4
[4] http://hg.mozilla.org/integration/mozilla-inbound/rev/21393b5c6963
[5] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b83738ca10f

Comment 15

[Kartikaya Gupta (email:[e-mail])]

(In reply to Markus Stange [:mstange] from comment #13)
> Thanks! Can you also do two try pushes (before and after) with "mozharness:
> --spsProfile"? I'm curious whether the profiles show the problem.

Before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7ee4b75225f
After: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ce7ab5a1a13

Note that in both of these tsvgx is pretty high (the "before" should really have values < 350), so you may want to do a couple of retriggers to get a good profile of the "before" case.

Comment 16

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> If my interpretation of this
> change is correct, then my new try push at [5], based on m-c, should fix the
> regression.
> [5] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b83738ca10f

That change was deliberate. If we have an abs pos child of the scroll frame with opacity we would wrongly clip it. This could lead to perf regressions because we don't draw something we should be drawing for correctness. I don't know if tsvgx is actually triggering this case though.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tn) from comment #16)
> That change was deliberate. If we have an abs pos child of the scroll frame
> with opacity we would wrongly clip it. This could lead to perf regressions
> because we don't draw something we should be drawing for correctness. I
> don't know if tsvgx is actually triggering this case though.

tsvgx must be triggering that case or the regression wouldn't have gone away when I reverted that bit of the patch. It sounds like the change is intentional then, and the perf regression is acceptable because we get correctness in return. So I think we can close this bug as WONTFIX. Joel, any objections?

Comment 18

[Markus Stange [:mstange]]

(In reply to Timothy Nikkel (:tn) from comment #16)
> That change was deliberate. If we have an abs pos child of the scroll frame
> with opacity we would wrongly clip it. This could lead to perf regressions
> because we don't draw something we should be drawing for correctness. I
> don't know if tsvgx is actually triggering this case though.

So you've fixed bug 1145263 with this? The testcase in there seems to work now.

Comment 19

[Timothy Nikkel (:tnikkel)]

We fixed that specific test case for non-apz cases only.

Comment 20

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

thanks for looking into this guys!