1230448 - 2~5% TP5 scroll regression on inbound (v.45) from bug 1226118

Status: RESOLVED DUPLICATE of bug 1230056

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

Regression: Mozilla-Inbound-Non-PGO - TP5 Scroll - WINNT 5.1 (e10s) - 3.54% increase
------------------------------------------------------------------------------------
    Previous: avg 8.281 stddev 0.091 of 12 runs up to revision f3741d47fb6b7e9eceac5078686707fc8f0136df
    New     : avg 8.573 stddev 0.073 of 12 runs since revision 462a51797af1901934d422606a6a090cdb96bab0
    Change  : +0.293 (3.54% / z=3.204)
    Graph   : http://mzl.la/1O6RZiM

Regression: Mozilla-Inbound-Non-PGO - TP5 Scroll - WINNT 6.2 x64 (e10s) - 4.32% increase
----------------------------------------------------------------------------------------
    Previous: avg 5.474 stddev 0.044 of 12 runs up to revision f3741d47fb6b7e9eceac5078686707fc8f0136df
    New     : avg 5.711 stddev 0.077 of 12 runs since revision 462a51797af1901934d422606a6a090cdb96bab0
    Change  : +0.236 (4.32% / z=5.351)
    Graph   : http://mzl.la/1O6S463

Regression: Mozilla-Inbound-Non-PGO - TP5 Scroll - Ubuntu HW 12.04 x64 (e10s) - 3.1% increase
---------------------------------------------------------------------------------------------
    Previous: avg 6.502 stddev 0.049 of 12 runs up to revision f3741d47fb6b7e9eceac5078686707fc8f0136df
    New     : avg 6.703 stddev 0.034 of 12 runs since revision 462a51797af1901934d422606a6a090cdb96bab0
    Change  : +0.202 (3.1% / z=4.107)
    Graph   : http://mzl.la/1O6S3yR

Regression: Mozilla-Inbound-Non-PGO - TP5 Scroll - WINNT 6.1 (ix) (e10s) - 4.32% increase
-----------------------------------------------------------------------------------------
    Previous: avg 6.950 stddev 0.075 of 12 runs up to revision f3741d47fb6b7e9eceac5078686707fc8f0136df
    New     : avg 7.250 stddev 0.097 of 12 runs since revision 462a51797af1901934d422606a6a090cdb96bab0
    Change  : +0.300 (4.32% / z=4.003)
    Graph   : http://mzl.la/1HJPExn

Regression: Mozilla-Inbound-Non-PGO - TP5 Scroll - WINNT 5.1 (ix) - 3.43% increase
----------------------------------------------------------------------------------
    Previous: avg 4.108 stddev 0.033 of 12 runs up to revision f3741d47fb6b7e9eceac5078686707fc8f0136df
    New     : avg 4.249 stddev 0.025 of 12 runs since revision 462a51797af1901934d422606a6a090cdb96bab0
    Change  : +0.141 (3.43% / z=4.267)
    Graph   : http://mzl.la/1HJSjqZ

Comment 1

[Brian Birtles (:birtles)]

Currently bisecting on try (which would have been a lot faster if I realised tp5o_scroll is *not* in the tp5o suite!)

Comment 2

[Brian Birtles (:birtles)]

The regression seems to be introduced by parts 9 and 10 from bug 1226118.

 https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2043744de4f5&newProject=try&newRevision=1b7eb88a8093

 https://hg.mozilla.org/integration/mozilla-inbound/rev/a23aff82ad55
 https://hg.mozilla.org/integration/mozilla-inbound/rev/fdfecd674725

What's more surprising is that the bug 1230056, which was supposed to *improve* the performance of part 10, makes things even worse (although that Talos run included part 14 from bug 1226118 so it's possible that that is the cause).

Comment 3

[Brian Birtles (:birtles)]

I've pushed a try run with just up to part 9 to try to break this down further.

Possible causes:

* Array allocations are too costly
  - Should we allocate to a likely upper-bound on initialization to save subsequent allocations?
  - Are we not using the Move constructor and creating copies when we return?

* The ordering is different, especially for the no animations case.
  For example, it may be that nsLayoutUtils::AreAsyncAnimationsEnabled() is expensive and with the new implementation of GetAnimationsForCompositor we call that *before* we check if there are any animations or not.

At this stage, I'm inclined to suspect the latter.

Specifically, we're comparing the new EffectCompositor::GetAnimationsForCompositor introduced here:

  https://hg.mozilla.org/try/rev/f938c01b8917

With the old one removed here:

  https://hg.mozilla.org/try/rev/e163a3a1dfb6

If we move the check for an empty effect set to the top (and specifically check for effects->IsEmpty() since we don't currently delete these properties when they are empty like we should), then we should roughly match the existing implementation's performance for the "no animations" case with the exception of initialiazing and moving an empty array object.

Comment 4

[Brian Birtles (:birtles)]

Try run with early-return moved up an applied after part 10:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=370687707363

Try run with the same applied after the optimization from bug 1230056:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7f9b2ead78b

Comment 6

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #4)
> Try run with early-return moved up an applied after part 10:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=370687707363
> 
> Try run with the same applied after the optimization from bug 1230056:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7f9b2ead78b

Trying again with the necessary method added.

Try run with early-return moved up an applied after part 10:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ffeeee08ca1

Try run with the same applied after the optimization from bug 1230056:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0407fceffebe

Comment 7

[Brian Birtles (:birtles)]

So it looks like the proposed patch roughly halves the regression, but it's still significant.

Part 10 without patch (original regression):
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2043744de4f5&newProject=try&newRevision=1b7eb88a8093

Part 10 with patch:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2043744de4f5&newProject=try&newRevision=7ffeeee08ca1

Comparing just the different the patch makes, it gives about a 1.7% improvement:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1b7eb88a8093&newProject=try&newRevision=7ffeeee08ca1

Applied to the bug 1230056, the regression is still quite significant:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2043744de4f5&newProject=try&newRevision=0407fceffebe

So this needs further investigation. I don't think I'll be able to finish it by Monday so we might have to back bug 1226118 out.

Comment 8

[Brian Birtles (:birtles)]

Breaking down the regression, it seems part 9 from bug 1226118 causes a 1.82% regression on windows 7 only (other platforms actually improved), but part 10 causes a further 3~4% regression.

So either we're calling GetMinAndMaxScaleForAnimationProperty a lot (seems unlikely) or nsLayoutUtils::HasAnimationsForCompositor is called a lot (more likely). Bug 1230056 really should have fixed HasAnimationsForCompositor but there seems to be a further regression from that patch (or alternative part 14 of the original bug).

So it doesn't seem like array allocation is the issue. Further things to try:

* Split up the changes in part 10 and see which part is actually causing the regression (in case GetMinAndMaxScaleForAnimationProperty really is the culprit after all).
* Try rewriting HasAnimationsForCompositor (from bug 1230056) without all the Function wrappers, optional callback function etc. and see if there's any improvement.
* Try reordering the conditions in HasAnimationsForCompositor in case there's some other change in order we're hitting (although I don't expect so--I'd expect it to be the "no animations" case that's causing the regression)

Comment 9

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

thanks for filing this- it sounds like you have a decent start on it!

Comment 10

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

one thing to keep in mind, we don't have to go for perfection- if we understand the regression and determine it is worth keeping what remains after the fix that is a fine decision to make.

Comment 11

[Brian Birtles (:birtles)]

Thanks Joel! I think we can fix this, or get close anyway. Applying bug 1230056 along with some fixes seems so far gives a 1.26% regression on windows8 and a 1.32% improvement on linux64 (still waiting for windows7). I still have to work out *why* some of those fixes help. In particular, I'm not sure why removing usage of mozilla::Function made such a big difference.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2043744de4f5&newProject=try&newRevision=f54a3d4a7a81&showOnlyImportant=0

Comment 12

[Brian Birtles (:birtles)]

I've put patches up for review on bug 1230056 to fix this. Initial results look promising.

Performance improvement from bug 12300056:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a8f2bfb7a55a&newProject=try&newRevision=ab4bffaeb0ad&showOnlyImportant=0

Compared to baseline prior to landing bug 1226118:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=312003c340f2&newProject=try&newRevision=c9acffdf0290&showOnlyImportant=0

Comment 13

[Brian Birtles (:birtles)]

Fixed by bug 1230056.

Comparison showing the difference after applying both bug 1226118 and bug 1230056:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=66e520cba75b&newProject=try&newRevision=902bc57e9a60&showOnlyImportant=0