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)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- fixed

People

(Reporter: vaibhav1994, Assigned: ttaubert)

References

Details

Attachments

(1 file)

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
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
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)
TL;DR: I have no idea.
Time for Try pushes with backouts then?
Flags: needinfo?(ttaubert)
OK, I am re-triggering jobs for the push to see if the regressions are still there.
Thanks, I'll wait for you to report back before pushing to try as Gavin suggested.
Flags: needinfo?(ttaubert)
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.
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
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)
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)
Oh, I completely missed those because bug 1156722 was the only one referenced here.
Depends on: 1157220, 1156721
No longer depends on: 1156722
Pushed to try already, waiting for builds to finish and jobs to run.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8600864 - Flags: review?(smacleod)
Attachment #8600864 - Flags: review?(smacleod) → review+
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
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
we really cleaned these regressions up a lot!  Thanks for fixing this.
Thank you for keeping an eye on Talos!
You need to log in before you can comment on or make changes to this bug.