1169723 - 2-4% Linux*/Win* sessionrestore regression on Fx-Team on May 25, 2015 from push 08dfcb03f55b

Status: RESOLVED FIXED

(Toolkit :: View Source, defect)

Description

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

Talos has detected a Firefox performance regression from your commit 08dfcb03f55b in bug 1164448.  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=08dfcb03f55b&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 Monday, 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)]

this is a series of patches on the same push:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=301df7d05cf4&tochange=08dfcb03f55b

I did a few retriggers and this is showing that the push really is the culprit:
https://treeherder.mozilla.org/#/jobs?repo=fx-team&fromchange=31c58b01300a&tochange=7637b24cd7a5&filter-searchStr=Ubuntu%20HW%2012.04%20x64%20fx-team%20talos%20other

:jryans, can you look into why this is happening?

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Yes, I'll investigate this.  Let's avoid backing out if at all possible.

Comment 3

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

thanks, ask for help as needed!

Comment 4

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

I suggest seeing if removing the loadFrameScript in browser.js's onload makes it go away. Worth testing with a try push. If so, we might be able to load it lazily when a view source tab is first opened.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Mike Conley (:mconley) - PTO until June 2nd from comment #4)
> I suggest seeing if removing the loadFrameScript in browser.js's onload
> makes it go away. Worth testing with a try push. If so, we might be able to
> load it lazily when a view source tab is first opened.

Right, that was my first plan of attack. :)

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Let's see if the frame script is indeed the cause:

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

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Joel, the try syntax you suggested in comment 0 appears to have not run Talos at all.  Is there a different syntax I should use?

Comment 8

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

I have triggered the jobs via mozci (https://github.com/armenzg/mozilla_ci_tools).  I need to look into the talos syntax for other, I believe it should have been other_nol64, so many quirks and details.

sorry for misleading you!

Comment 9

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

looking at the numbers they look better!  I would say this patch fixes the regression.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Joel Maher (:jmaher) from comment #9)
> looking at the numbers they look better!  I would say this patch fixes the
> regression.

Great!  The current patch on try also removes functionality, so I'll now try to craft a lazy version that keeps the correct functionality without affecting the Talos metric.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Testing a potential fix:

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

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> Testing a potential fix:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=554cc51520fc

Joel, this try run looks good to me, do you agree?

Comment 13

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

yes, these numbers look a LOT better!  thanks:)

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try with tests:

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

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1169723 - Load view source frame script lazily. r=mconley

Bug 1169723 - Load view source frame script lazily. r=mconley

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Fixed browser tests:

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

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8614767 [details]
MozReview Request: Bug 1169723 - Load view source frame script lazily. r=mconley

Bug 1169723 - Load view source frame script lazily. r=mconley

Comment 18

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

Comment on attachment 8614767 [details]
MozReview Request: Bug 1169723 - Load view source frame script lazily. r=mconley

https://reviewboard.mozilla.org/r/10035/#review9023

This looks fine to me.

I wonder how much more session restore time we could save by getting rid of more of those loadFrameScript calls. I've filed bug 1171720 to investigate that.

Thanks jryans!

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #18)
> Comment on attachment 8614767 [details]
> MozReview Request: Bug 1169723 - Load view source frame script lazily.
> r=mconley
> 
> https://reviewboard.mozilla.org/r/10035/#review9023
> 
> This looks fine to me.
> 
> I wonder how much more session restore time we could save by getting rid of
> more of those loadFrameScript calls. I've filed bug 1171720 to investigate
> that.
> 
> Thanks jryans!

It even adds a feature: view source tabs restored from session restore did not have working "extra" features like "Go to Line" before this patch.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/173a4adce55e

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

> +    // If the restored browser wants to show view source content, start up a
> +    // view source browser that will load the required frame script.
> +    if (uri && ViewSourceBrowser.isViewSource(uri)) {
> +      new ViewSourceBrowser(browser);
> +    }

This seems less than ideal, why would sessionstore have to know about this special type of browser? Can we please handle this differently? Sessionstore should not have to know about any tab's content/display and treat it differently.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

> +// Keep a set of browsers we've seen before, so we can load our frame script as
> +// needed into any new ones.
> +let gKnownBrowsers = new WeakSet();
>
>   gKnownBrowsers.add(this.browser);

This will break with docShell swapping. If drag&drop a view-source: tab to another window it will load the frame-script there again.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/173a4adce55e

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/173a4adce55e

Comment 25

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

(In reply to Tim Taubert [:ttaubert] from comment #21)
> > +    // If the restored browser wants to show view source content, start up a
> > +    // view source browser that will load the required frame script.
> > +    if (uri && ViewSourceBrowser.isViewSource(uri)) {
> > +      new ViewSourceBrowser(browser);
> > +    }
> 
> This seems less than ideal, why would sessionstore have to know about this
> special type of browser? Can we please handle this differently? Sessionstore
> should not have to know about any tab's content/display and treat it
> differently.

Heh, I guess when it comes to talos regressions, my tolerance for hacks goes up. :)

Do you feel we should back this out while we look for an alternative solution?

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

I think backing out is a little too drastic, I would like to see a follow-up though to make sure we'll replace that with a better solution soon.

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Tim Taubert [:ttaubert] from comment #26)
> I think backing out is a little too drastic, I would like to see a follow-up
> though to make sure we'll replace that with a better solution soon.

Sorry for the trouble!  I do think it's good to keep it in for the Talos improvement, and we can improve the approach in bug 1171986.