1214287 - 2-4% linux64/win7/8 ts_paint regression on inbound (v.44) seen on Oct 12, 2015 from rev 3012b7a2c97c

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

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

Talos has detected a Firefox performance regression from your commit 3012b7a2c97c19422775145cad67516d167c16b9 in bug 1210940.  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=3012b7a2c97c19422775145cad67516d167c16b9&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#ts_paint

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 linux64,win64,win32 -u none -t other  # 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 ts_paint

Making a decision:
As the patch author we need your feedback to help us handle this regression.
*** Please let us know your plans by Friday, 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 compare view on the push:
https://treeherder.allizom.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=906f76d5adcf&newProject=mozilla-inbound&newRevision=3012b7a2c97c&e10s=1

you can see the regression for ts_paint, the other regressions appear to be more noise related or not as high confidence.

Since windows is so backlogged, I didn't push to try to bisect the change down to figure out which of the 3 patches is the root cause, ideally we can figure that out- but if folks would wish, I would be happy to do the 4 try pushes.

:oyiptong, can you take a look at this and comment on the regression?

Comment 2

[Olivier Yiptong [:oyiptong]]

I'm taking a look

Comment 3

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33745c371561

Comment 4

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

keep in mind this is e10s only, so in compare view you need to add &e10s=1 in the url to view the e10s results.  We should have that fixed next week to show all the results on the same URL in perfherder, but for now we need the url param.

Comment 5

[Olivier Yiptong [:oyiptong]]

Attachment: MozReview Request: Bug 1214287 - 2-4% linux64/win7/8 ts_paint regression on inbound (v.44) seen on Oct 12, 2015 from rev 3012b7a2c97c r?mconley

Bug 1214287 - 2-4% linux64/win7/8 ts_paint regression on inbound (v.44) seen on Oct 12, 2015 from rev 3012b7a2c97c r?mconley

Comment 6

[Olivier Yiptong [:oyiptong]]

Attachment: Bug 1214287 - 2-4% linux64/win7/8 ts_paint regression on inbound (v.44) seen on Oct 12, 2015 from rev 3012b7a2c97c

Comment 7

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc69c1a42b91

Comment 8

[Olivier Yiptong [:oyiptong]]

What we care about here is ts_paint e10s performance (non-e10s numbers will be wrong because of windows test slave backlog)

talos test here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6681f3b82916

comparison with tree prior to remote new tab landing:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d42ff8b181b4&newProject=try&newRevision=6681f3b82916&showOnlyImportant=0&showOnlyConfident=0

expected: performance is about the same
observed: performance is about the same

comparison with tree after 3012b7a2c97c:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ff9d37dd375&newProject=try&newRevision=6681f3b82916&showOnlyImportant=0&showOnlyConfident=0

expected: performance is improved
observed: performance is improved

Causation:

The issue was that revision 8ea636dce7e6 introduced a replica of module DirectoryLinksProvider.
This module does disk I/O on init in nsBrowserGlue.js. The replica doubles the number of disk I/O's.

Removing the replica solves the talos regression

Comment 9

[Olivier Yiptong [:oyiptong]]

Attachment: MozReview Request: Bug 1214287 - 2-4% linux64/win7/8 ts_paint regression on inbound (v.44) seen on Oct 12, 2015 from rev 3012b7a2c97c r?mconley

Bug 1214287 - 2-4% linux64/win7/8 ts_paint regression on inbound (v.44) seen on Oct 12, 2015 from rev 3012b7a2c97c r?mconley

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682125 [details]
MozReview Request: Bug 1214287 - 2-4% linux64/win7/8 ts_paint regression on inbound (v.44) seen on Oct 12, 2015 from rev 3012b7a2c97c r?mconley

https://reviewboard.mozilla.org/r/23981/#review21401

Good job hunting this down - thanks oyiptong!

Comment 11

[Olivier Yiptong [:oyiptong]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce12a70a081a64662f4fabaa5cc6a730874b60d3
Bug 1214287 - 2-4% linux64/win7/8 ts_paint regression on inbound (v.44) seen on Oct 12, 2015 from rev 3012b7a2c97c r=mconley

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce12a70a081a

Comment 13

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

thanks, I see an improvement in the graphs!

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ce12a70a081a

Comment 15

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 16

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8682125 [details]
MozReview Request: Bug 1214287 - 2-4% linux64/win7/8 ts_paint regression on inbound (v.44) seen on Oct 12, 2015 from rev 3012b7a2c97c r?mconley

Approval Request Comment
[Feature/regressing bug #]:1210940
[User impact if declined]: browser startup performance could decrease by 2-4% on certain machines
[Describe test coverage new/current, TreeHerder]: There was a reported gains in startup performance in Talos, tests look fine: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ce12a70a081a
[Risks and why]: this is a low risk uplift, because most of the code it impacts is pref'ed off. The patch is mostly the removal of code from a new component. There is still risk, because the patch attaches observers to an existing module, that said, the module seems to be working fine in nightly, in a highly visible piece of UI (the newtab page), and all the tests pass.
[String/UUID change made/needed]: none

Comment 17

[Daniel Holbert [:dholbert]]

RE the commit message here:
>  Bug 1214287 - 2-4% linux64/win7/8 ts_paint regression on inbound (v.44) seen on Oct 12, 2015 from rev 3012b7a2c97c r=mconley

For mozilla-central commits in the future, please make commit messages describe the *change*, not the *problem that was fixed*. For more, see:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 18

[Olivier Yiptong [:oyiptong]]

Will do, thanks.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8682125 [details]
MozReview Request: Bug 1214287 - 2-4% linux64/win7/8 ts_paint regression on inbound (v.44) seen on Oct 12, 2015 from rev 3012b7a2c97c r?mconley

This fix has been in Nightly for a week and verified that it fixes the perf regression. It seems safe to uplift to Aurora44.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0da1c27313f1

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0da1c27313f1