Closed
Bug 1159248
Opened 9 years ago
Closed 9 years ago
9.5-10.8% Linux*/Win* sessionrestore regression on Fx-Team-Non-PGO (v. 40) on April 27, 2015 from push fafd4e0ba1bf
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: vaibhav1994, Assigned: ttaubert)
References
Details
Attachments
(1 file)
1.56 KB,
patch
|
smacleod
:
review+
|
Details | Diff | Splinter Review |
Talos has detected a Firefox performance regression from your commit fafd4e0ba1bf in bug 1156722. 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=fafd4e0ba1bf&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#sessionrestore.2Fsessionrestore_no_auto_restore 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,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 sessionrestore 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
Reporter | ||
Comment 1•9 years ago
|
||
Tim, could you see why these regressions are being caused?
Flags: needinfo?(ttaubert)
Summary: 9.5-10.8% Linux*/Win* sessionrestore regression on Fx-Team-Non-PGO on April 27, 2015 from push fafd4e0ba1bf → 9.5-10.8% Linux*/Win* sessionrestore regression on Fx-Team-Non-PGO (v. 40) on April 27, 2015 from push fafd4e0ba1bf
Assignee | ||
Comment 2•9 years ago
|
||
Alright... let's take a look: https://hg.mozilla.org/mozilla-central/rev/fafd4e0ba1bf This just removes unused code. Shouldn't cause a regression. https://hg.mozilla.org/mozilla-central/rev/218472f29153 Removes another bit of unused code. Shouldn't cause regressions either. https://hg.mozilla.org/mozilla-central/rev/9202d5cfd9db This just checks for another property, unlikely too. https://hg.mozilla.org/mozilla-central/rev/053a40e84ca5 This doesn't use [].shift() anymore but destructuring when undo-closing a tab. Unlikely too, I'd expect this to be faster as it doesn't have to modify the array that we throw away anyway. https://hg.mozilla.org/mozilla-central/rev/41ac0617ae66 This replaces calls to a method with direct calls to JSON.stringify(), that should make no difference whatsoever.
Flags: needinfo?(ttaubert)
Assignee | ||
Comment 3•9 years ago
|
||
TL;DR: I have no idea.
Reporter | ||
Comment 5•9 years ago
|
||
OK, I am re-triggering jobs for the push to see if the regressions are still there.
Assignee | ||
Comment 6•9 years ago
|
||
Thanks, I'll wait for you to report back before pushing to try as Gavin suggested.
Flags: needinfo?(ttaubert)
Reporter | ||
Comment 7•9 years ago
|
||
Results after retriggering have come in and the patch fafd4e0ba1bf looks like the cause: https://treeherder.mozilla.org/#/jobs?repo=fx-team&fromchange=eb203de9655f&tochange=9c16aed65792&filter-searchStr=Windows%207%2032-bit%20fx-team%20talos%20other sessionrestore on Windows XP opt has gone from 1730-1760 to ~1950 with this patch.
Assignee | ||
Comment 8•9 years ago
|
||
Baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d016a8f2516c (In reply to Tim Taubert [:ttaubert] from comment #2) > https://hg.mozilla.org/mozilla-central/rev/053a40e84ca5 > > This doesn't use [].shift() anymore but destructuring when undo-closing a > tab. Unlikely too, I'd expect this to be faster as it doesn't have to modify > the array that we throw away anyway. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed96bb78679d > https://hg.mozilla.org/mozilla-central/rev/41ac0617ae66 > > This replaces calls to a method with direct calls to JSON.stringify(), that > should make no difference whatsoever. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f2f91245a80
Assignee | ||
Comment 9•9 years ago
|
||
Here are the comparisons of all backouts: http://compare-talos.mattn.ca/?oldRevs=d016a8f2516c&newRev=3ef9607fbc2f&server=graphs.mozilla.org&submit=true http://compare-talos.mattn.ca/?oldRevs=d016a8f2516c&newRev=8d4a165a623e&server=graphs.mozilla.org&submit=true http://compare-talos.mattn.ca/?oldRevs=d016a8f2516c&newRev=fc301dc58115&server=graphs.mozilla.org&submit=true http://compare-talos.mattn.ca/?oldRevs=d016a8f2516c&newRev=ed96bb78679d&server=graphs.mozilla.org&submit=true http://compare-talos.mattn.ca/?oldRevs=d016a8f2516c&newRev=0f2f91245a80&server=graphs.mozilla.org&submit=true None of those yield a sessionrestore test improvement.
Assignee | ||
Comment 10•9 years ago
|
||
Backed out *all* changesets. No improvement: http://compare-talos.mattn.ca/?oldRevs=d016a8f2516c&newRev=97a7c635a2eb&server=graphs.mozilla.org&submit=true This regression looks bogus, how do we proceed now?
Flags: needinfo?(vaibhavmagarwal)
Comment 11•9 years ago
|
||
from the push (https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=fafd4e0ba1bf), there are two remaining revisions that I don't see in the *backout all*: there is still: https://hg.mozilla.org/integration/fx-team/rev/802abda95b78 and: https://hg.mozilla.org/integration/fx-team/rev/46cee22af41f I agree that your numbers do not show an improvement, the try number for backing out all is identical the the new numbers after the push. looks like we narrowed it down to 2 revisions!
Flags: needinfo?(vaibhavmagarwal)
Assignee | ||
Comment 12•9 years ago
|
||
Oh, I completely missed those because bug 1156722 was the only one referenced here.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 13•9 years ago
|
||
Pushed to try already, waiting for builds to finish and jobs to run.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Assignee | ||
Comment 14•9 years ago
|
||
802abda95b78 is the cause: http://compare-talos.mattn.ca/?oldRevs=d016a8f2516c&newRev=7316028a5b57&server=graphs.mozilla.org&submit=true
Assignee | ||
Comment 15•9 years ago
|
||
Let's see. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4474cad1435
Assignee | ||
Comment 16•9 years ago
|
||
Looking good. This is the push compared to current fx-team: http://compare-talos.mattn.ca/?oldRevs=d016a8f2516c&newRev=f4474cad1435&server=graphs.mozilla.org&submit=true This is the push compared to the regressing cset backed out: http://compare-talos.mattn.ca/?oldRevs=7316028a5b57&newRev=f4474cad1435&server=graphs.mozilla.org&submit=true
Assignee | ||
Updated•9 years ago
|
Attachment #8600864 -
Flags: review?(smacleod)
Updated•9 years ago
|
Attachment #8600864 -
Flags: review?(smacleod) → review+
Assignee | ||
Updated•9 years ago
|
Iteration: --- → 40.3 - 11 May
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/ba9bb4a353b4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment 19•9 years ago
|
||
we really cleaned these regressions up a lot! Thanks for fixing this.
Assignee | ||
Comment 20•9 years ago
|
||
Thank you for keeping an eye on Talos!
You need to log in
before you can comment on or make changes to this bug.
Description
•