990084 - tresize regression on linux64 from bug 477948

Status: RESOLVED WONTFIX

(Firefox :: Theme, defect)

Description

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

when bug 477948 landed, linux64 tresize had a 12% regression:
http://graphs.mozilla.org/graph.html#tests=[[254,132,35]]&sel=1395855528000,1396028328000&displayrange=7&datatype=running

this rolled through the branches and has been seen on other branches as well.  I have done retriggers and this sticks as the point where the regression was introduced:
https://tbpl.mozilla.org/?tree=Fx-Team&fromchange=5d6abe1396fb&tochange=98ac1097f8e2&jobname=Ubuntu%20HW%2012.04%20x64%20fx-team%20talos%20chromez

Comment 1

[Mike de Boer [:mikedeboer]]

I did a Try push with Aurora as the base - since that branch contains Australis and no other patches that might interfere with Talos test results in their own way.

baseline Try push: https://tbpl.mozilla.org/?tree=Try&rev=fcf193449da4
keyhole Try push: https://tbpl.mozilla.org/?tree=Try&rev=32c9583cea1c

http://compare-talos.mattn.ca/?oldRevs=fcf193449da4&newRev=32c9583cea1c&server=graphs.mozilla.org&submit=true

This shows a considerable tresize regression of 8%, which will need to be investigated, dissected and fixed.

Comment 2

[Mike de Boer [:mikedeboer]]

Locally, I could reproduce regression of 4%. This seems good enough to work with and bisect.

Comment 3

[Mike de Boer [:mikedeboer]]

Baseline try push, based on latest fx-team tip:

https://tbpl.mozilla.org/?tree=Try&rev=88be43e745a5

Next I'll be pushing some pushes to track down the cause of this regression.

Comment 4

[Mike de Boer [:mikedeboer]]

Full keyhole patch, compared to baseline:

https://tbpl.mozilla.org/?tree=Try&rev=6c9da65aaa5c

http://compare-talos.mattn.ca/?oldRevs=88be43e745a5&newRev=6c9da65aaa5c&server=graphs.mozilla.org&submit=true

Comment 5

[Mike de Boer [:mikedeboer]]

First off, we see that the keyhole patch applied on top of the full backout in comment 4 shows a tresize regression of 9.39%, which is more or less what we're fighting here.

1) Only DOM SVG Changes, compared to baseline:

https://tbpl.mozilla.org/?tree=Try&rev=e51c06af61f9

http://compare-talos.mattn.ca/?oldRevs=88be43e745a5&newRev=e51c06af61f9&server=graphs.mozilla.org&submit=true

In words: when you apply ONLY the DOM SVG changes on top of the full keyhole backout, we see a 4.38% tresize regression. This constitutes about 53% of the total regression (9.39%).

2) Only DOM and Win theme changes, compared to baseline:

https://tbpl.mozilla.org/?tree=Try&rev=5ca574ae12c2

http://compare-talos.mattn.ca/?oldRevs=88be43e745a5&newRev=5ca574ae12c2&server=graphs.mozilla.org&submit=true

In words: when you apply ONLY the DOM SVG AND Windows theme changes on top of the full keyhole backout, we see a 2.85% win. This is baffling to me, since I can not explain it.

3) Full keyhole patch with only clip-path CSS rules disabled:

https://tbpl.mozilla.org/?tree=Try&rev=a56c635aeb44

http://compare-talos.mattn.ca/?oldRevs=6c9da65aaa5c&newRev=a56c635aeb44&server=graphs.mozilla.org&submit=true

In words: when you disable ONLY the clip-path rules that apply the SVG clip paths effectively on the relevant nav-bar areas and compare it to the fully re-applied keyhole patch, we see a win of 10.36%. This means that NOT applying SVG clip-paths effectively cancels out the regression.

-----

Avih, what are your thoughts on my analysis above? To me, it seems to prove that SVG (clip-paths) are the main culprit here, but not using them is not an option; we need this for the keyhole UI.

Comment 6

[Avi Halachmi (:avih)]

That's some good data. Thanks for looking into this.

The numbers suggest, to me as well, that the SVG clip path rules are an expensive part of whatever causes the regression. jaws has a similar sentiment which he expressed in bug 477948 comment 45 (though WRT SVG opacity rather than tresize).

So, I've asked around a bit, and here's the info I gathered:

1. BenWa says that invalidating only the parts of the chrome which are changing is going to be hard to optimize, and that he thinks the general sentiment among layers/gfx guys is that we're not particularly optimizing for resize. Currently, the entire chrome seems to redraw on each resize frame (tested nightly on windows with and without the titlebar).

2. jaws examined the patch briefly and nothing obvious came up as potentially unoptimized. There's also no simple dirty trick to workaround the SVG clip paths for this case. He also says it could be possible to disable the clip paths during resizing, but we don't have code which does similar things, and he thinks the goal probably doesn't justify the means.

My view on this, considering the above, is that the keyhole patch is inherently more complex than the previous back/forward buttons, with no obvious approach to work around it or mitigate its cost.

Considering the efforts which have been put into this already, and the fact that it's important to ship Australis complete on Linux as well, and that we don't want big changes (beyond the patch itself) for an uplift to Firefox 29, I'd say we'll need to live with this regression, and land this. I can live with this.

Dao, what do you think?

Comment 7

[Dão Gottwald [:dao]]

fine by me. I don't think not applying the clip-path during resize is an option, as it would look broken.

Comment 8

[Avi Halachmi (:avih)]

Resolving as wontfix per comment 6 and comment 7.

If this is the last regression blocking us, let's nominate to uplift?

Comment 9

[:Gijs (he/him)]

(In reply to Avi Halachmi (:avih) from comment #8)
> Resolving as wontfix per comment 6 and comment 7.
> 
> If this is the last regression blocking us, let's nominate to uplift?

Sadly, there is also still bug 989466.