Most scripts loaded in browser.xul could be loaded lazily

RESOLVED FIXED in Firefox 56

Status

()

Firefox
General
P1
normal
RESOLVED FIXED
a month ago
4 days ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-performance])

Attachments

(22 attachments)

2.77 KB, patch
mconley
: review+
Details | Diff | Splinter Review
2.50 KB, patch
mconley
: review+
Details | Diff | Splinter Review
4.50 KB, patch
mconley
: review+
Details | Diff | Splinter Review
4.16 KB, patch
mossop
: review+
mconley
: review+
Details | Diff | Splinter Review
4.39 KB, patch
mconley
: review+
Details | Diff | Splinter Review
9.22 KB, patch
mconley
: review+
Details | Diff | Splinter Review
2.67 KB, patch
mconley
: review+
Details | Diff | Splinter Review
4.81 KB, patch
mconley
: review+
Details | Diff | Splinter Review
3.46 KB, patch
mconley
: review+
Details | Diff | Splinter Review
4.48 KB, patch
mconley
: review+
Details | Diff | Splinter Review
3.75 KB, patch
mconley
: review+
Details | Diff | Splinter Review
6.86 KB, patch
mconley
: review+
Details | Diff | Splinter Review
11.19 KB, patch
mconley
: review+
Details | Diff | Splinter Review
3.84 KB, patch
mconley
: review+
Details | Diff | Splinter Review
5.98 KB, patch
mconley
: review+
Details | Diff | Splinter Review
5.94 KB, patch
mconley
: review+
Details | Diff | Splinter Review
3.96 KB, patch
mconley
: review+
Details | Diff | Splinter Review
4.72 KB, patch
mconley
: review+
Details | Diff | Splinter Review
10.79 KB, patch
mconley
: review+
Details | Diff | Splinter Review
2.29 KB, patch
mconley
: review+
Details | Diff | Splinter Review
1.67 KB, patch
mossop
: review+
Details | Diff | Splinter Review
1.47 KB, patch
mossop
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a month ago
When creating the first browser.xul window during startup, a lot of time is spent loading scripts from:
- http://searchfox.org/mozilla-central/source/browser/base/content/global-scripts.inc
- http://searchfox.org/mozilla-central/rev/88180977d79654af025158d8ebeb8c2aa11940eb/browser/base/content/browser.xul#67

These all have to be loaded before the DOMContentLoaded event and so before we start doing any real initialization work for the window.

This gets even worse when somehow the startup cache doesn't work (bug 1364235).

Most of these scripts aren't needed until at least _delayedStartup, so we cna load them lazily instead.

Updated

a month ago
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Flags: qe-verify-
Priority: -- → P1
The tendency to push code as early is possible might, in part, come from the fact that there's no way to deterministically guarantee that your code runs before first paint and doesn't flicker.

If we implemented something deterministic (for example sth like in bug 1280260), we might be able to better schedule code loading and reduce the temptation to push code into the earliest possible stage.
(Assignee)

Comment 2

a month ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #1)
> The tendency to push code as early is possible might, in part, come from the
> fact that there's no way to deterministically guarantee that your code runs
> before first paint and doesn't flicker.

None of the code I'm touching here is asynchronous, I'm putting in place lazy getters for code that currently gets loaded before first paint, but doesn't run until after first paint.
(Assignee)

Comment 3

29 days ago
Created attachment 8888397 [details] [diff] [review]
add defineLazyScriptGetter on XPCOMUtils.jsm
Attachment #8888397 - Flags: review?(mconley)
(Assignee)

Comment 4

29 days ago
Created attachment 8888398 [details] [diff] [review]
load printUtils.js lazily
Attachment #8888398 - Flags: review?(mconley)
(Assignee)

Comment 5

29 days ago
Created attachment 8888399 [details] [diff] [review]
lazy load viewZoomOverlay.js and browser-fullZoom.js
Attachment #8888399 - Flags: review?(mconley)
(Assignee)

Comment 6

29 days ago
Created attachment 8888402 [details] [diff] [review]
Make defineLazyScriptGetter support multiple symbols

Requesting review from Mossop for the eslint change as Standard8's review queue is currently closed.
Attachment #8888402 - Flags: review?(mconley)
Attachment #8888402 - Flags: review?(dtownsend)
(Assignee)

Comment 7

29 days ago
Created attachment 8888404 [details] [diff] [review]
lazy load browserPlacesViews.js
Attachment #8888404 - Flags: review?(mconley)
(Assignee)

Comment 8

29 days ago
Created attachment 8888405 [details] [diff] [review]
lazy load panelUI.js
Attachment #8888405 - Flags: review?(mconley)
(Assignee)

Comment 9

29 days ago
Created attachment 8888407 [details] [diff] [review]
lazy load viewSourceUtils.js
Attachment #8888407 - Flags: review?(mconley)
(Assignee)

Comment 10

29 days ago
Created attachment 8888410 [details] [diff] [review]
lazy load browser-addons.js
Attachment #8888410 - Flags: review?(mconley)
(Assignee)

Comment 11

29 days ago
Created attachment 8888411 [details] [diff] [review]
lazy load browser-ctrlTab.js
Attachment #8888411 - Flags: review?(mconley)
(Assignee)

Comment 12

29 days ago
Created attachment 8888412 [details] [diff] [review]
lazy load browser-customization.js
Attachment #8888412 - Flags: review?(mconley)
(Assignee)

Comment 13

29 days ago
Created attachment 8888413 [details] [diff] [review]
lazy load browser-fullScreenAndPointerLock.js
Attachment #8888413 - Flags: review?(mconley)
(Assignee)

Comment 14

29 days ago
Created attachment 8888414 [details] [diff] [review]
lazy load browser-gestureSupport.js
Attachment #8888414 - Flags: review?(mconley)
(Assignee)

Comment 15

29 days ago
Created attachment 8888415 [details] [diff] [review]
Remove browser-refreshblocker.js
Attachment #8888415 - Flags: review?(mconley)
(Assignee)

Comment 16

29 days ago
Created attachment 8888416 [details] [diff] [review]
lazy load browser-safebrowsing.js
Attachment #8888416 - Flags: review?(mconley)
(Assignee)

Comment 17

29 days ago
Created attachment 8888417 [details] [diff] [review]
lazy load browser-social.js

The Utils.jsm import is unfortunate here, it was exposed to the global browser window scope by accident (social API code using defineLazyModuleGetter with 'this' as the first parameter, pointing to the global...), and it's used by tabbrowser.xml. I think this would be worth cleaning up in a follow-up, maybe after 57 so that we don't have to worry about add-ons that may be using Utils.* in the browser window's scope.
Attachment #8888417 - Flags: review?(mconley)
(Assignee)

Comment 18

29 days ago
Created attachment 8888419 [details] [diff] [review]
lazy load browser-sync.js
Attachment #8888419 - Flags: review?(mconley)
(Assignee)

Comment 19

29 days ago
Created attachment 8888420 [details] [diff] [review]
lazy load browser-thumbnails.js
Attachment #8888420 - Flags: review?(mconley)
(Assignee)

Comment 20

29 days ago
Created attachment 8888421 [details] [diff] [review]
lazy load nsContextMenu.js
Attachment #8888421 - Flags: review?(mconley)
(Assignee)

Comment 21

29 days ago
Created attachment 8888423 [details] [diff] [review]
lazy load downloads scripts
Attachment #8888423 - Flags: review?(mconley)
(Assignee)

Comment 22

29 days ago
Created attachment 8888424 [details] [diff] [review]
lazy load editBookmarkOverlay.js

Last patch of the series... and this one is pretty trivial :-).
Attachment #8888424 - Flags: review?(mconley)

Updated

29 days ago
Attachment #8888397 - Flags: review?(mconley) → review+
Comment on attachment 8888402 [details] [diff] [review]
Make defineLazyScriptGetter support multiple symbols

Review of attachment 8888402 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing until I hear more about the multiple script loads.

::: js/xpconnect/loader/XPCOMUtils.jsm
@@ +234,5 @@
> +        get: function() {
> +          for (let n of aName) {
> +            delete aObject[n];
> +          }
> +          Services.scriptloader.loadSubScript(aResource, aObject);

Maybe I'm not reading this right, but supposing some script exposes 2 symbols (SymbolA and SymbolB), then doesn't accessing SymbolA and then SymbolB then result in _two_ calls to loadSubScript?

I imagine we only want to load the script once... shouldn't we populate all symbols as soon as the first one is requested?
Attachment #8888402 - Flags: review?(mconley)
Comment on attachment 8888398 [details] [diff] [review]
load printUtils.js lazily

Review of attachment 8888398 [details] [diff] [review]:
-----------------------------------------------------------------

\o/
Attachment #8888398 - Flags: review?(mconley) → review+
Comment on attachment 8888399 [details] [diff] [review]
lazy load viewZoomOverlay.js and browser-fullZoom.js

Review of attachment 8888399 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-fullZoom.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +// This file is loaded into the browser window scope.
> +/* eslint-env mozilla/browser-window */

I guess viewZoomOverlay.js doesn't need this?

::: browser/base/content/browser.js
@@ +102,5 @@
>  }
>  
>  XPCOMUtils.defineLazyScriptGetter(this, "PrintUtils",
>                                    "chrome://global/content/printUtils.js");
> +XPCOMUtils.defineLazyScriptGetter(this, "ZoomManager",

Is it worth trying to develop a pattern of putting these in alphabetical order? I'm fine to do that in a follow-up so that I don't mess up your patch stack.
Attachment #8888399 - Flags: review?(mconley) → review+
Comment on attachment 8888404 [details] [diff] [review]
lazy load browserPlacesViews.js

Review of attachment 8888404 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +11,5 @@
>  var Cr = Components.results;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/AppConstants.jsm");

Should we make this a lazyModuleGetter? Or are we sure it's already been imported by this point?
Attachment #8888404 - Flags: review?(mconley) → review+
Comment on attachment 8888405 [details] [diff] [review]
lazy load panelUI.js

Review of attachment 8888405 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +33,5 @@
>            LoginManagerParent:false, NewTabUtils:false, PageThumbs:false,
>            PluralForm:false, Preferences:false, PrivateBrowsingUtils:false,
>            ProcessHangMonitor:false, PromiseUtils:false, ReaderMode:false,
> +          ReaderParent:false, RecentWindow:false, SafeBrowsing: false,
> +          SessionStore:false,

I wonder if each of these should be on their own line.

At any rate, a nit that it's kinda awkward to have some lines be single entries and some not.
Attachment #8888405 - Flags: review?(mconley) → review+

Updated

29 days ago
Attachment #8888407 - Flags: review?(mconley) → review+

Updated

29 days ago
Attachment #8888410 - Flags: review?(mconley) → review+

Updated

29 days ago
Attachment #8888411 - Flags: review?(mconley) → review+

Updated

29 days ago
Attachment #8888412 - Flags: review?(mconley) → review+

Updated

29 days ago
Attachment #8888413 - Flags: review?(mconley) → review+

Updated

29 days ago
Attachment #8888414 - Flags: review?(mconley) → review+

Updated

29 days ago
Attachment #8888415 - Flags: review?(mconley) → review+

Updated

29 days ago
Attachment #8888416 - Flags: review?(mconley) → review+
Comment on attachment 8888417 [details] [diff] [review]
lazy load browser-social.js

Review of attachment 8888417 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +37,5 @@
>            SessionStore:false,
>            ShortcutUtils:false, SimpleServiceDiscovery:false, SitePermissions:false,
>            Social:false, TabCrashHandler:false, Task:false, TelemetryStopwatch:false,
> +          Translation:false, UITour:false, Utils: false, UpdateUtils:false,
> +          Weave:false,

Same as the earlier patch with the odd single entry here.
Attachment #8888417 - Flags: review?(mconley) → review+
Comment on attachment 8888419 [details] [diff] [review]
lazy load browser-sync.js

Review of attachment 8888419 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +1972,5 @@
>  
>      // initialize the sync UI
> +    requestIdleCallback(() => {
> +      gSync.init();
> +    }, {timeout: 1000 * 5});

I assume this isn't going to break any tests? Maybe we should ensure kitcambridge, markh et al knows about this.
Attachment #8888419 - Flags: review?(mconley) → review+

Updated

29 days ago
Attachment #8888420 - Flags: review?(mconley) → review+

Updated

29 days ago
Attachment #8888421 - Flags: review?(mconley) → review+
Comment on attachment 8888423 [details] [diff] [review]
lazy load downloads scripts

Review of attachment 8888423 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/content/downloads.js
@@ +81,5 @@
>  
>  /**
>   * Main entry point for the downloads panel interface.
>   */
> +var DownloadsPanel = {

Out of curiosity, why is it necessary to change these from consts?
Attachment #8888423 - Flags: review?(mconley) → review+
Comment on attachment 8888424 [details] [diff] [review]
lazy load editBookmarkOverlay.js

Review of attachment 8888424 [details] [diff] [review]:
-----------------------------------------------------------------

I assume you've gotten a green try run with all of these?
Attachment #8888424 - Flags: review?(mconley) → review+
(Assignee)

Comment 32

29 days ago
(In reply to Mike Conley from comment #23)
> Comment on attachment 8888402 [details] [diff] [review]
> Make defineLazyScriptGetter support multiple symbols
>
> Clearing until I hear more about the multiple script loads.
> 
> ::: js/xpconnect/loader/XPCOMUtils.jsm
> @@ +234,5 @@
> > +        get: function() {
> > +          for (let n of aName) {
> > +            delete aObject[n];
> > +          }
> > +          Services.scriptloader.loadSubScript(aResource, aObject);
> 
> Maybe I'm not reading this right, but supposing some script exposes 2
> symbols (SymbolA and SymbolB), then doesn't accessing SymbolA and then
> SymbolB then result in _two_ calls to loadSubScript?
> 
> I imagine we only want to load the script once... shouldn't we populate all
> symbols as soon as the first one is requested?

Loading the script into the scope will load all symbols at once. We want to get rid of all the getters first so that they don't get in the way of creating the actual symbols. This is the reason why the "delete" call is in a loop: we need to delete one getter per symbol.

(In reply to comment #25)
> Comment on attachment 8888399 [details] [diff] [review]
> lazy load viewZoomOverlay.js and browser-fullZoom.js
> 
> > +// This file is loaded into the browser window scope.
> > +/* eslint-env mozilla/browser-window */
> 
> I guess viewZoomOverlay.js doesn't need this?

It's needed only when the script uses globals that are defined in browser.js

> ::: browser/base/content/browser.js
> @@ +102,5 @@
> >  }
> >  
> >  XPCOMUtils.defineLazyScriptGetter(this, "PrintUtils",
> >                                    "chrome://global/content/printUtils.js");
> > +XPCOMUtils.defineLazyScriptGetter(this, "ZoomManager",
> 
> Is it worth trying to develop a pattern of putting these in alphabetical
> order? I'm fine to do that in a follow-up so that I don't mess up your patch
> stack.

The list got long enough that it would probably make sense to put everything in an array, and iterate through it with a .forEach, like we do for module getters. Unfortunately, that makes detecting the symbols from eslint much harder.


(In reply to comment #26)
> Comment on attachment 8888404 [details] [diff] [review]
> lazy load browserPlacesViews.js
>
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >  Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource://gre/modules/AppConstants.jsm");
> 
> Should we make this a lazyModuleGetter? Or are we sure it's already been
> imported by this point?

I am 100% sure :-). These 3 modules are the very first imports during app-startup. And because it's trivial to show a proof: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/modules/Services.jsm#11


(In reply to comment #27)
> ::: browser/base/content/browser.js
> @@ +33,5 @@
> >            LoginManagerParent:false, NewTabUtils:false, PageThumbs:false,
> >            PluralForm:false, Preferences:false, PrivateBrowsingUtils:false,
> >            ProcessHangMonitor:false, PromiseUtils:false, ReaderMode:false,
> > +          ReaderParent:false, RecentWindow:false, SafeBrowsing: false,
> > +          SessionStore:false,
> 
> I wonder if each of these should be on their own line.
> 
> At any rate, a nit that it's kinda awkward to have some lines be single
> entries and some not.

I didn't bother making this comment look pretty, because I really want this whole comment block to disappear; it's awful. That's bug 1325373, and to be able to make eslint work with my multi-symbol getter, I had to learn enough about how eslint detects global that I think I can now easily fix that one too... it's the next bug I intend to take after this one.
Comment on attachment 8888402 [details] [diff] [review]
Make defineLazyScriptGetter support multiple symbols

Review of attachment 8888402 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Florian Quèze [:florian] [:flo] from comment #32)
> Loading the script into the scope will load all symbols at once. We want to
> get rid of all the getters first so that they don't get in the way of
> creating the actual symbols. This is the reason why the "delete" call is in
> a loop: we need to delete one getter per symbol.

Ah, of course. Thanks for clearing that up!
Attachment #8888402 - Flags: review+
(Assignee)

Comment 34

29 days ago
(In reply to comment #29)

> ::: browser/base/content/browser.js
> @@ +1972,5 @@
> >  
> >      // initialize the sync UI
> > +    requestIdleCallback(() => {
> > +      gSync.init();
> > +    }, {timeout: 1000 * 5});
> 
> I assume this isn't going to break any tests?

I think this was an accident. Bug 1369539 made this change to the initialization code path that happens in normal browser windows, but forgot to take care of the gBrowserInit.nonBrowserWindowDelayedStartup case.

> Maybe we should ensure
> kitcambridge, markh et al knows about this.

Good point, I'll add a comment in bug 1369539 to let them know.


(In reply to comment #30)

> ::: browser/components/downloads/content/downloads.js
> @@ +81,5 @@
> >  
> >  /**
> >   * Main entry point for the downloads panel interface.
> >   */
> > +var DownloadsPanel = {
> 
> Out of curiosity, why is it necessary to change these from consts?

The JS engine won't let us access a const before it's defined. The problem we run into here is that we access the symbol (ie. the getter) and then attempt to define a const with the same name. That throws a JS error.
Comment on attachment 8888402 [details] [diff] [review]
Make defineLazyScriptGetter support multiple symbols

Review of attachment 8888402 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/loader/XPCOMUtils.jsm
@@ +214,5 @@
>     * be loaded until first use.
>     *
>     * @param aObject
>     *        The object to define the lazy getter on.
>     * @param aName

Please rename this argument to aNames.
Attachment #8888402 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 36

29 days ago
Created attachment 8888558 [details] [diff] [review]
Make browser mochitests ignore global properties added by XPCOMUtils.defineLazyScriptGetter

My try run has plenty of "test left unexpected property on window" failures, which is a good thing, it proves that we are _actually_ loading things lazily. But I'll need the tests to be green, so let's ignore these successful 'failures'.
Attachment #8888558 - Flags: review?(dtownsend)
Comment on attachment 8888558 [details] [diff] [review]
Make browser mochitests ignore global properties added by XPCOMUtils.defineLazyScriptGetter

Review of attachment 8888558 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!
Attachment #8888558 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 38

28 days ago
Created attachment 8888962 [details] [diff] [review]
fix Talos sessionrestore_no_auto_restore test

Talos sessionrestore startup tests were already intermittently failing (bug 1343532, bug 1109124), but my patches here make this a perma fail on Windows.

There's a race here between the sessionStartup.onceInitialized promise resolving, and the browser window loading. The existing code works well if:
- the promise resolves at a time when the window doesn't exist yet, or hasn't finished loading all its scripts (ie. DOMContentLoaded hasn't been fired yet)
- the promise resolves at a time when the browser window has finished delayed startup.

The existing code breaks if the promise resolves at a point where we have already loaded all scripts, but haven't reached the delayedStartupFinished point yet. This case becomes very frequent with the patches here, as reducing the amount of scripts we load makes the browser window reach DOMContentLoaded faster.
Attachment #8888962 - Flags: review+
(In reply to Florian Quèze [:florian] [:flo] from comment #32)
> (In reply to comment #26)
> > Comment on attachment 8888404 [details] [diff] [review]
> > lazy load browserPlacesViews.js
> >
> > >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > >  Cu.import("resource://gre/modules/Services.jsm");
> > > +Cu.import("resource://gre/modules/AppConstants.jsm");
> > 
> > Should we make this a lazyModuleGetter? Or are we sure it's already been
> > imported by this point?
> 
> I am 100% sure :-). These 3 modules are the very first imports during
> app-startup. And because it's trivial to show a proof:
> http://searchfox.org/mozilla-central/rev/
> 3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/modules/Services.jsm#11

If we made Services.jsm a preprocessed file, we could avoid loading AppConstants.jsm there. I'm not sure how far forward it would push AppConstants.jsm loading though.
(Assignee)

Comment 40

28 days ago
(In reply to Marco Castelluccio [:marco] from comment #39)
> (In reply to Florian Quèze [:florian] [:flo] from comment #32)
> > (In reply to comment #26)
> > > Comment on attachment 8888404 [details] [diff] [review]
> > > lazy load browserPlacesViews.js
> > >
> > > >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > > >  Cu.import("resource://gre/modules/Services.jsm");
> > > > +Cu.import("resource://gre/modules/AppConstants.jsm");
> > > 
> > > Should we make this a lazyModuleGetter? Or are we sure it's already been
> > > imported by this point?
> > 
> > I am 100% sure :-). These 3 modules are the very first imports during
> > app-startup. And because it's trivial to show a proof:
> > http://searchfox.org/mozilla-central/rev/
> > 3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/modules/Services.jsm#11
> 
> If we made Services.jsm a preprocessed file, we could avoid loading
> AppConstants.jsm there. I'm not sure how far forward it would push
> AppConstants.jsm loading though.

Preprocessed JS files are a pain for linters, but at some point I had a similar idea: since XPCOMUtils, Services and AppConstants are imported very early during startup, and used more or less everywhere, should we merge these 3 modules into a single one? But I was told it wouldn't really help performance.
(Assignee)

Comment 41

28 days ago
(In reply to Florian Quèze [:florian] [:flo] from comment #38)
> Created attachment 8888962 [details] [diff] [review]
> fix Talos sessionrestore_no_auto_restore test
> 
> Talos sessionrestore startup tests were already intermittently failing (bug
> 1343532, bug 1109124), but my patches here make this a perma fail on Windows.
> 
> There's a race here between the sessionStartup.onceInitialized promise
> resolving, and the browser window loading. The existing code works well if:
> - the promise resolves at a time when the window doesn't exist yet, or
> hasn't finished loading all its scripts (ie. DOMContentLoaded hasn't been
> fired yet)
> - the promise resolves at a time when the browser window has finished
> delayed startup.
> 
> The existing code breaks if the promise resolves at a point where we have
> already loaded all scripts, but haven't reached the delayedStartupFinished
> point yet. This case becomes very frequent with the patches here, as
> reducing the amount of scripts we load makes the browser window reach
> DOMContentLoaded faster.

This patch fixes the talos sessionrestore_no_auto_restore failure for me locally, but on try it's still as red as before:

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

I started wondering if my fix had been applied, so I then pushed to try a simple talos patch (without the rest of the patch queue) that very clearly breaks this test... and it's green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45e60c2ae23001851734957b0226bae4305d21cc

So try indeed doesn't seem to pickup talos changes.

How do one verify that a talos failure is fixed? And how can I proceed to land this patch queue?
Flags: needinfo?(mconley)
Flags: needinfo?(jmaher)
(Assignee)

Comment 42

27 days ago
(In reply to Florian Quèze [:florian] [:flo] from comment #41)

> How do one verify that a talos failure is fixed? And how can I proceed to
> land this patch queue?

Forget that, I just needed to repack and resign the sessionrestore-signed.xpi add-on file.

Here is what the talos comparison looks like for that test on Windows now: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b1488fa9a209fb9b0bfd4ded5ed0aab5c2ce90d3&newProject=try&newRevision=798a2106d066a85b7b029b0a1ecbe9bc165872c0&framework=1&showOnlyImportant=0
Flags: needinfo?(mconley)
Flags: needinfo?(jmaher)

Comment 43

27 days ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e323bc846da0
teach Talos how to correctly wait for the end of delayed startup, r=Mossop.

Comment 44

27 days ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86169e904b4d
add defineLazyScriptGetter on XPCOMUtils.jsm, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c31eeb2adfc4
load printUtils.js into the browser window lazily, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d831fc2b2b
load viewZoomOverlay.js and browser-fullZoom.js into the browser window lazily, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e738363519e
Make defineLazyScriptGetter support lazy loading scripts exposing several symbols to the global scope, r=mconley,Mossop.
https://hg.mozilla.org/integration/mozilla-inbound/rev/527d222ce02b
lazy load browserPlacesViews.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc56d937ce45
lazy load panelUI.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b370041728d7
lazy load viewSourceUtils.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8290dcdbd045
lazy load browser-addons.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b34c1e730dd7
lazy load browser-ctrlTab.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8f162c4ea1b
lazy load browser-customization.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbcb866f2e4e
lazy load browser-fullScreenAndPointerLock.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/664bdf175333
lazy load browser-gestureSupport.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/85e90150f820
Remove browser-refreshblocker.js and move its code to tabbrowser.xml, avoiding the RefreshBlocked event completely, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3349552837
lazy load browser-safebrowsing.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/004272a8e098
lazy load browser-social.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/422d4e124e4d
lazy load browser-sync.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/214b6f71a6d5
lazy load browser-thumbnails.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/171e3671ef3d
lazy load nsContextMenu.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fd61b3209f6
lazy load downloads scripts into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a5168e0b5e4
lazy load editBookmarkOverlay.js into the browser window, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ece30d321d35
Make browser mochitests ignore global properties added by XPCOMUtils.defineLazyScriptGetter, r=Mossop.
https://hg.mozilla.org/mozilla-central/rev/e323bc846da0
https://hg.mozilla.org/mozilla-central/rev/86169e904b4d
https://hg.mozilla.org/mozilla-central/rev/c31eeb2adfc4
https://hg.mozilla.org/mozilla-central/rev/f6d831fc2b2b
https://hg.mozilla.org/mozilla-central/rev/2e738363519e
https://hg.mozilla.org/mozilla-central/rev/527d222ce02b
https://hg.mozilla.org/mozilla-central/rev/bc56d937ce45
https://hg.mozilla.org/mozilla-central/rev/b370041728d7
https://hg.mozilla.org/mozilla-central/rev/8290dcdbd045
https://hg.mozilla.org/mozilla-central/rev/b34c1e730dd7
https://hg.mozilla.org/mozilla-central/rev/e8f162c4ea1b
https://hg.mozilla.org/mozilla-central/rev/fbcb866f2e4e
https://hg.mozilla.org/mozilla-central/rev/664bdf175333
https://hg.mozilla.org/mozilla-central/rev/85e90150f820
https://hg.mozilla.org/mozilla-central/rev/8f3349552837
https://hg.mozilla.org/mozilla-central/rev/004272a8e098
https://hg.mozilla.org/mozilla-central/rev/422d4e124e4d
https://hg.mozilla.org/mozilla-central/rev/214b6f71a6d5
https://hg.mozilla.org/mozilla-central/rev/171e3671ef3d
https://hg.mozilla.org/mozilla-central/rev/5fd61b3209f6
https://hg.mozilla.org/mozilla-central/rev/3a5168e0b5e4
https://hg.mozilla.org/mozilla-central/rev/ece30d321d35
Status: ASSIGNED → RESOLVED
Last Resolved: 27 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
I see ts_paint improvements on windows:
== Change summary for alert #8191 (as of July 22 2017 22:19 UTC) ==

Improvements:

  4%  ts_paint windows10-64 opt e10s     800.54 -> 766.83
  4%  ts_paint windows7-32 opt e10s      925.29 -> 888.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8191
Depends on: 1383908
(Assignee)

Updated

23 days ago
See Also: → bug 1384714

Updated

4 days ago
See Also: → bug 1388583
You need to log in before you can comment on or make changes to this bug.