1202011 - 3% linux32 tp5o scroll e10s only on inbound on sept 3 from revision b4711fa5f734

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

Talos has detected a Firefox performance regression from your commit b4711fa5f7348a391f19fbab240c16339d3001b1 in bug 1137944.  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=b4711fa5f7348a391f19fbab240c16339d3001b1&showAll=1

Some links to more data comparing the new revision to the previous one:
e10s: https://treeherder.allizom.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=174ee19829ec&newProject=mozilla-inbound&newRevision=b4711fa5f734&e10s=1
non-e10s: https://treeherder.allizom.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=174ee19829ec&newProject=mozilla-inbound&newRevision=b4711fa5f734

^ * keep in mind it might take a few hours for data to finish coming in.

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#tp5o_scroll

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 g1  # 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 tp5o_scroll

Making a decision:
As the patch author we need your feedback to help us handle this regression.
*** Please let us know your plans by Tuesday, 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

[Jim Mathies [:jimm]]

The way we record timing was modified slightly in cset b4711fa5f734. Unless something really ugly shows up I'd argue this change can be ignored since it's not a real performance regression.

Comment 2

[Jim Mathies [:jimm]]

Now I'm seeing more reports, and there's a lot of apz checkin work here. In my patch I removed an early DidComposite call and instead relied on the RAII helper I added.

https://bugzilla.mozilla.org/attachment.cgi?id=8656145&action=diff

But now I'm wondering if this has something to do with some of this zoom work. Kats, any thoughts?

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/0c678c16f473
    : Kartikaya Gupta <[e-mail]> - Bug 1197824 - Stop propagating the default zoom around unnecessarily. r=snorp

The default zoom value is only used on the Java side to clamp the min/max zoom
values in the case where zooming is disabled. We can do this much earlier in
the flow, when we are computing the metadata, and reduce the amount of
redundant information being passed around.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1197824

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/db5c0d802e67
    : Kartikaya Gupta <[e-mail]> - Bug 1197824 - Constrain the min/max zoom if zooming is not allowed. r=botond

In the C++ APZ implementation this has no effect, because the min/max zoom values
are never even read if zooming is not allowed. When this is hooked up to the
Java implementation though, the code expects the min/max zoom values to be
equal to the default zoom values in the case where zooming is not allowed. This
behaviour change therefore facilitates hooking up the ZoomConstraintsClient
to the Java pan/zoom controller implementation, which happens in a future patch.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1197824

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/fb074ed8557f
    : Kartikaya Gupta <[e-mail]> - Bug 1197824 - Remove unused state and normalize others. r=botond

In the case of DisplayWidthHeight viewports, setting the allowDoubleTapToZoom
flag to true is fine because the ZoomConstraintsClient code will flip it back
to false based on the width of the CSS viewport. Setting it to true in the
GetViewportInfo code allows us to maintain the invariant that the default value
of allowDoubleTapToZoom is the same as the initial value of allowZoom.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1197824

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/62aac0e570e3
    : Kartikaya Gupta <[e-mail]> - Bug 1197824 - Allow zooming in desktop mode. r=botond

This is a long-standing issue that has thus far never been exposed because
the values modified in this patch have not been used (desktop mode only exists
on Fennec, and Fennec does it's own computation for these values in browser.js).
However upcoming patches will change Fennec to use this, and so this issue needs
to be corrected.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1197824

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/2b2a45987233
    : Kartikaya Gupta <[e-mail]> - Bug 1197824 - Support the browser.ui.zoom.force-user-scalable pref in the gecko zoom-constraints codepath. r=botond

The browser.ui.zoom.force-user-scalable pref can be modified by the user from
the Fennec settings screen, and allows them zoom pages despite the meta-viewport
tag that might otherwise restrict zooming. This effectively ignores the effect
of the user-scalable, minimum-scale, and maximum-scale meta-viewport tokens.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1197824

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/730d2acfbdb6
    : Kartikaya Gupta <[e-mail]> - Bug 1197824 - Add a zoom-constraints-updated message. r=snorp

With this patch the ZoomConstraintsClient updates get broadcast via the observer
service on Android, allowing code in browser.js to switch over to using it.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1197824

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/211782e11049
    : Kartikaya Gupta <[e-mail]> - Bug 1197824 - Switch Fennec over to using the zoom constraints from Gecko. r=snorp

Now Fennec can use the message from Gecko with the zoom constraints and we can
delete a bunch of code that did the equivalent job in browser.js
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1197824

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/6605bf7174c8
    : Kartikaya Gupta <[e-mail]> - Bug 1197824 - Remove/simplify more code in browser.js dealing with viewport metadata. r=snorp
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1197824

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/ea9f2295023b
    : Kartikaya Gupta <[e-mail]> - Bug 1197824 - Remove the mAllowDoubleTapZoom field from nsViewportInfo as it is not needed. r=botond
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1197824

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/eaa34e557496
    : Julien Pag?s <[e-mail]> - Bug 1201511 - [mozrunner] require mozprocess >=0.22 and bump mozrunner version to 6.10. r=ahal
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1201511

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/174ee19829ec
    : Carsten "Tomcat" Book <[e-mail]> - Backed out changeset b317ee483a64 (bug 1107372) for causing test failures in test_home_provider.html
    : http://bugzilla.mozilla.org/show_bug.cgi?id=317

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/0325cf8f3cda
    : Jim Mathies <[e-mail]> - Bug 1137944 - Simplify the api associated with hiding e10s plugin widgets. r=roc
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1137944

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/b4711fa5f734
    : Jim Mathies <[e-mail]> - Bug 1137944 - Fire before and after composite events. r=matt.woodrow
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1137944

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/117b337240b8
    : Wes Kocher <[e-mail]> - Backed out changeset eaa34e557496 (bug 1201511) for landing too early CLOSED TREE
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1201511

Comment 3

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

I got some email notices too for stuff in this range, but they were all for improvements, and on PGO builds. And looking at the alertmanager link in comment 0, 75% of stuff reported are improvements. (For the record, I find these alertmanager pages really confusing to decipher - clicking on "toggle view" to get the grid is waaaaay more readable [1]).

So: the only regression reported is TP5 scroll on Ubuntu HW 12.04 (e10s). If we look at the graphserver data for that [2] we can see that the regression happened between 174ee19829e and b4711fa5f73 which points squarely to bug 1137944. The improvements in the other three tests [3][4][5] are also from the same range.

It's also surprising to me that the "TP5 scroll on Ubuntu 12.04 (e10s)" test regressed by 3.02%, while I got an email notice for a 2.91% improvement in the exact same test on PGO builds (slightly wider range which included my changes, since PGO builds didn't run on every push).

I think comment 1 is probably the right answer here.

[1] http://alertmanager.allizom.org:8080/alerts.html?rev=b4711fa5f7348a391f19fbab240c16339d3001b1&table=1&show_improvement=1
[2] http://graphs.mozilla.org/graph.html#tests=[[323,131,41]]&sel=1441252213299.442,1441291746219.541,7.230410102557725,7.799545616709501&displayrange=7&datatype=geo
[3] http://graphs.mozilla.org/graph.html#tests=[[323,131,43]]&sel=1441246201613.8647,1441289354911.162,6.312171954243735,7.209546655914379&displayrange=7&datatype=geo
[4] http://graphs.mozilla.org/graph.html#tests=[[287,131,41]]&sel=1441252112032.1404,1441286333577.298,6.833413062630519,8.26539396955176&displayrange=7&datatype=geo
[5] http://graphs.mozilla.org/graph.html#tests=[[287,131,43]]&sel=1441242273455.5757,1441304793586.1523,6.060143372893049,7.6066827523679885&displayrange=7&datatype=geo

Comment 4

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

This is specifically for the code from bug 1137944.  If you look at:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=0f1dcd03f20a&tochange=dd0900e3e6f1&filter-searchStr=Ubuntu%20HW%2012.04%20mozilla-inbound%20talos%20g1

Then explain the e10s points, you will see the difference in the tp5o_scroll numbers between https://hg.mozilla.org/integration/mozilla-inbound/rev/b4711fa5f734 and https://hg.mozilla.org/integration/mozilla-inbound/rev/174ee19829ec.

Comment 5

[Jim Mathies [:jimm]]

should be fixed by the backout of 5bb965937e24.

Comment 6

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

verified, thanks for updating the bug.