Closed Bug 1214287 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: New Tab Page, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: jmaher, Assigned: oyiptong)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 2 obsolete files)

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
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?
Flags: needinfo?(oyiptong)
Whiteboard: [talos_regression] → [talos_regression][e10s]
I'm taking a look
Flags: needinfo?(oyiptong)
Assignee: nobody → oyiptong
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.
Depends on: 1215641
Bug 1214287 - 2-4% linux64/win7/8 ts_paint regression on inbound (v.44) seen on Oct 12, 2015 from rev 3012b7a2c97c r?mconley
Attachment #8681578 - Flags: review?(mconley)
Attachment #8681578 - Attachment is obsolete: true
Attachment #8681578 - Flags: review?(mconley)
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
Bug 1214287 - 2-4% linux64/win7/8 ts_paint regression on inbound (v.44) seen on Oct 12, 2015 from rev 3012b7a2c97c r?mconley
Attachment #8682125 - Flags: review?(mconley)
Attachment #8681584 - Attachment is obsolete: true
Attachment #8681584 - Flags: review?(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!
Attachment #8682125 - Flags: review?(mconley) → review+
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
https://hg.mozilla.org/mozilla-central/rev/ce12a70a081a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
thanks, I see an improvement in the graphs!
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
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
Attachment #8682125 - Flags: approval-mozilla-aurora?
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
Will do, thanks.
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.
Attachment #8682125 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.