Closed Bug 1230448 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1230056
Tracking Flags:
Tracking Status
firefox45 --- affected

People

(Reporter: birtles, Assigned: birtles)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression]) 

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
Currently bisecting on try (which would have been a lot faster if I realised tp5o_scroll is *not* in the tp5o suite!)
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).
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.
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
Keywords: perf, regression
Whiteboard: [talos_regression]
(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
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.
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)
thanks for filing this- it sounds like you have a decent start on it!
Blocks: 1220148
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.
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
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.