Closed
Bug 1358921
Opened 8 years ago
Closed 8 years ago
Avoid loading and initializing modules in BrowserGlue._finalUIStartup
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
(Blocks 2 open bugs)
Details
Attachments
(11 files)
(deleted),
text/x-review-board-request
|
florian
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
florian
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
florian
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
florian
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
florian
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
florian
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
florian
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
florian
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
florian
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
florian
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
florian
:
review+
|
Details |
I'd like to be able to ship WebExtensions as system add-ons without any regression in startup performance. There aren't a lot of places I can speed things up, but this is one big chunk of startup time that keeps nagging at me. We load a lot of modules just to add listeners that won't be called until well after the browser window is visible.
If we add the listeners in nsBrowserGlue.js instead and only load the modules when they're called, we can put off a lot of this work.
On my machine, these patches save about 10ms from the otherwise 48ms time _finalUIStartup takes to run.
There's more that could be done here, but I think this makes a good start.
Assignee | ||
Updated•8 years ago
|
Blocks: photon-startup
Comment 1•8 years ago
|
||
I was observing something similar in bug 1358798, where components are loaded even earlier during app-startup just to register listeners.
Assignee | ||
Comment 2•8 years ago
|
||
Yeah, it looks like we should easily be able to adapt this code to handle those, just using lazy service getters rather than module getters.
And Weave, in particular, doesn't appear to do anything until after a 10 second timer (SelfSupport does something similar), so we should be able to delay loading that until even later.
Assignee | ||
Comment 3•8 years ago
|
||
Florian, mind if I redirect these reviews to you, given Ehsan's backlog?
Flags: needinfo?(florian)
Comment 4•8 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #3)
> Florian, mind if I redirect these reviews to you, given Ehsan's backlog?
I'm happy to review patches fixing performance issues, but I don't see patches here (yet).
Flags: needinfo?(florian)
Assignee | ||
Comment 5•8 years ago
|
||
Hm. Mozreview... :/ I guess it failed to publish them and I didn't notice.
I'll flag you for review, then. Thanks
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•8 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> (In reply to Kris Maglione [:kmag] from comment #3)
> > Florian, mind if I redirect these reviews to you, given Ehsan's backlog?
>
> I'm happy to review patches fixing performance issues, but I don't see
> patches here (yet).
Thanks. :-)
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8861082 [details]
Bug 1358921: Lazily load LoginManagerParent.jsm when first needed.
https://reviewboard.mozilla.org/r/132722/#review136152
::: browser/components/nsBrowserGlue.js:163
(Diff revision 1)
> + "RemoteLogins:findLogins": ["LoginManagerParent"],
> + "RemoteLogins:findRecipes": ["LoginManagerParent"],
> + "RemoteLogins:onFormSubmit": ["LoginManagerParent"],
> + "RemoteLogins:autoCompleteLogins": ["LoginManagerParent"],
> + "RemoteLogins:removeLogin": ["LoginManagerParent"],
> + "RemoteLogins:insecureLoginFormPresent": ["LoginManagerParent"],
I appriciate the effort to consolidate module initializitian code and reduce boilerplate, but I think it's rather unfortunate that your patches bake module logic into nsBrowserGlue. This is particular problematic for toolkit modules. I think we should aim to keep modules self-contained.
Attachment #8861082 -
Flags: review-
Comment 19•8 years ago
|
||
mozreview-review |
Comment on attachment 8861074 [details]
Bug 1358921: Lazily load webrtcUI.jsm when first needed.
https://reviewboard.mozilla.org/r/132706/#review136154
Great work on this bug, thanks for taking it! :-)
::: browser/components/nsBrowserGlue.js:116
(Diff revision 1)
>
> +const global = this;
> +
> +const listeners = {
> + observers: {
> + "browser-delayed-startup-finished": ["webrtcUI"],
This notification shouldn't trigger loading webrtcUI if it hasn't been initialized yet. And there's no 'observe' method in webrtcUI.jsm.
webrtcUI uses this notification to update menus in new windows if there's an existing window with an active stream.
::: browser/components/nsBrowserGlue.js:120
(Diff revision 1)
> + observers: {
> + "browser-delayed-startup-finished": ["webrtcUI"],
> + },
> +
> + ppmm: {
> + "child-process-shutdown": ["webrtcUI"],
I don't think child-process-shutdown should make us load the module.
::: browser/components/nsBrowserGlue.js:135
(Diff revision 1)
> + "webrtc:StopRecording": ["webrtcUI"],
> + "webrtc:UpdateBrowserIndicators": ["webrtcUI"],
> + },
> +
> + observe(subject, topic, data) {
> + for (let module of this.observers[topic] || []) {
Do we really need this '|| []'? Isn't it a bug of the listeners object if we added an observer for a topic that isn't in the observers object?
::: browser/components/nsBrowserGlue.js:155
(Diff revision 1)
> + }
> + }
> + },
> +
> + init() {
> + for (let observer of Object.keys(this.observers)) {
Could this be simplified to:
for (let topic in this.observers)
?
::: browser/components/nsBrowserGlue.js:160
(Diff revision 1)
> + for (let observer of Object.keys(this.observers)) {
> + Services.obs.addObserver(this, observer);
> + }
> +
> + let receiveMessageMM = this.receiveMessage.bind(this, this.mm);
> + for (let message of Object.keys(this.mm)) {
and similarly:
for (let message in this.mm)
::: browser/modules/webrtcUI.jsm:36
(Diff revision 1)
> - mm.addMessageListener("webrtc:StopRecording", this);
> - mm.addMessageListener("webrtc:CancelRequest", this);
> - mm.addMessageListener("webrtc:UpdateBrowserIndicators", this);
> - },
> -
> uninit() {
Now that webrtcUI won't be loaded unless a page actually uses webrtc, it's sad that nsBrowserGlue.js will load this module unconditionally at http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/browser/components/nsBrowserGlue.js#918
Should webrtcUI.jsm register its own "quit-application-granted" observer so that this uninit call is only executed if webrtcUI was initialized before?
An alternative is to use in nsBrowserGlue.js' _onQuitApplicationGranted method a hack like http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/browser/base/content/browser.js#5080
Maybe implement that hack only once and have an array of modules on which we call an 'uninit' method only if the module is no longer a lazy getter.
Attachment #8861074 -
Flags: review?(florian) → review-
Comment 20•8 years ago
|
||
mozreview-review |
Comment on attachment 8861075 [details]
Bug 1358921: Lazily load AboutHome.jsm when first needed.
https://reviewboard.mozilla.org/r/132708/#review136184
Attachment #8861075 -
Flags: review?(florian) → review+
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8861076 [details]
Bug 1358921: Lazily load ContentSearch.jsm when first needed.
https://reviewboard.mozilla.org/r/132710/#review136146
::: browser/components/nsBrowserGlue.js:129
(Diff revision 1)
>
> const listeners = {
> observers: {
> "browser-delayed-startup-finished": ["webrtcUI"],
> + "browser-search-engine-modified": ["ContentSearch"],
> + "shutdown-leaks-before-check": ["ContentSearch"],
I don't think these 2 notifications should trigger an initialization of ContentSearch; probably better to leave these addObserver calls inside ContentSearch.jsm.
Attachment #8861076 -
Flags: review?(florian) → review-
Comment 22•8 years ago
|
||
mozreview-review |
Comment on attachment 8861077 [details]
Bug 1358921: Lazily load FormValidationHandler.jsm when first needed.
https://reviewboard.mozilla.org/r/132712/#review136186
Attachment #8861077 -
Flags: review?(florian) → review+
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8861078 [details]
Bug 1358921: Lazily load ContentClick.jsm when first needed.
https://reviewboard.mozilla.org/r/132714/#review136188
Attachment #8861078 -
Flags: review?(florian) → review+
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8861079 [details]
Bug 1358921: Lazily load RemotePrompt.jsm when first needed.
https://reviewboard.mozilla.org/r/132716/#review136162
::: browser/modules/RemotePrompt.jsm:22
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUIUtils",
> + "resource:///modules/PlacesUIUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils",
> + "resource://gre/modules/PrivateBrowsingUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> + "resource://gre/modules/Services.jsm");
Please remove this; Services.jsm is already loaded a few lines above (making Services.jsm lazy-loaded isn't really useful anywhere, as it's loaded very early during startup).
::: browser/modules/RemotePrompt.jsm:24
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils",
> + "resource://gre/modules/PrivateBrowsingUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> + "resource://gre/modules/Services.jsm");
> +
> Cu.import("resource://gre/modules/SharedPromptUtils.jsm");
This one could be made lazy, we only use the PromptUtils symbol from it.
Attachment #8861079 -
Flags: review?(florian) → review-
Comment 25•8 years ago
|
||
mozreview-review |
Comment on attachment 8861080 [details]
Bug 1358921: Lazily load Feeds.jsm when first needed.
https://reviewboard.mozilla.org/r/132718/#review136190
Attachment #8861080 -
Flags: review?(florian) → review+
Comment 26•8 years ago
|
||
mozreview-review |
Comment on attachment 8861081 [details]
Bug 1358921: Lazily load ContentPrefServiceParent.jsm when first needed.
https://reviewboard.mozilla.org/r/132720/#review136172
::: browser/components/nsBrowserGlue.js:138
(Diff revision 1)
> + "ContentPrefs:FunctionCall": ["ContentPrefServiceParent"],
> + "ContentPrefs:AddObserverForName": ["ContentPrefServiceParent"],
> + "ContentPrefs:RemoveObserverForName": ["ContentPrefServiceParent"],
> "FeedConverter:addLiveBookmark": ["Feeds"],
> "WCCR:setAutoHandler": ["Feeds"],
> - "child-process-shutdown": ["webrtcUI"],
> + "child-process-shutdown": ["ContentPrefServiceParent", "webrtcUI"],
Similar to my comment for webrtcUI, I think child-process-shutdown should not cause us to load the module.
::: toolkit/components/contentprefs/ContentPrefServiceParent.jsm:108
(Diff revision 1)
> this._observers.delete(msg.target);
> }
> }
> },
>
> + // Listeners are added in nsBrowserGlue.js
Looks like you forgot to cleanup the init method in this file. And we can probably completely get rid of it if you add a lazy service getter for nsIContentPrefService2.
But we need to be careful about not breaking android here, so by "get rid of it" I mostly mean "not call it on desktop".
Attachment #8861081 -
Flags: review?(florian) → review-
Comment 27•8 years ago
|
||
mozreview-review |
Comment on attachment 8861082 [details]
Bug 1358921: Lazily load LoginManagerParent.jsm when first needed.
https://reviewboard.mozilla.org/r/132722/#review136180
The r- here is just about improving the comment to partially address Dão's concern from comment 18.
::: toolkit/components/passwordmgr/LoginManagerParent.jsm:40
(Diff revision 1)
> * @deprecated
> */
> _recipeManager: null,
>
> + // This should only be called on Android. Listeners are added in
> + // nsBrowserGlue.js on desktop.
Please mention in this comment that the list of message listeners added in this method need to stay in sync with the list from nsBrowserGlue.js (it's already implied by your current comment, but I think we should make it explicit).
Attachment #8861082 -
Flags: review?(florian) → review-
Comment 28•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8861082 [details]
Bug 1358921: Lazily load LoginManagerParent.jsm when first needed.
https://reviewboard.mozilla.org/r/132722/#review136152
> I appriciate the effort to consolidate module initializitian code and reduce boilerplate, but I think it's rather unfortunate that your patches bake module logic into nsBrowserGlue. This is particular problematic for toolkit modules. I think we should aim to keep modules self-contained.
This keeps the module self-contained enough in my opinion, in that the module is still usable without the nsBrowserGlue logic. We may need to think about how to ensure the two initialization paths stay in sync... but I think for now just a comment is enough.
Comment 29•8 years ago
|
||
mozreview-review |
Comment on attachment 8861083 [details]
Bug 1358921: Lazily load ReaderParent.jsm when first needed.
https://reviewboard.mozilla.org/r/132724/#review136196
Attachment #8861083 -
Flags: review?(florian) → review+
Comment 30•8 years ago
|
||
mozreview-review |
Comment on attachment 8861084 [details]
Bug 1358921: Lazily load SelfSupportBackend.jsm when first needed.
https://reviewboard.mozilla.org/r/132726/#review136182
::: browser/modules/SelfSupportBackend.jsm:78
(Diff revision 1)
> _progressListener: null,
>
> // Whether we're invited to let test code talk to our frame.
> _testing: false,
>
> + _enabled: false,
I don't understand what you are doing or trying to do with this _enabled field. Can you explain with a comment above this line?
::: browser/modules/SelfSupportBackend.jsm:105
(Diff revision 1)
> this._log.config("init - Disabling SelfSupport because UITour is disabled.");
> return;
> }
>
> // Check the preferences to see if we want this to be active.
> if (!Preferences.get(PREF_ENABLED, true)) {
This file uses Preferences.jsm near startup just to have support for default values :-(. We should clean this up, but you don't have to do it in this bug; it should probably be a separate bug blocking bug 1357517).
::: browser/modules/SelfSupportBackend.jsm:158
(Diff revision 1)
> + // Observers are added in nsBrowserGlue.js
> observe(aSubject, aTopic, aData) {
> this._log.trace("observe - Topic " + aTopic);
>
> if (aTopic === "sessionstore-windows-restored") {
> Services.obs.removeObserver(this, "sessionstore-windows-restored");
This will throw because we are not registering the observer on 'this' anymore.
::: browser/modules/SelfSupportBackend.jsm:160
(Diff revision 1)
> this._log.trace("observe - Topic " + aTopic);
>
> if (aTopic === "sessionstore-windows-restored") {
> Services.obs.removeObserver(this, "sessionstore-windows-restored");
> + if (this._enabled) {
> - this._delayedLoadTimerId = setTimeout(this._loadSelfSupport.bind(this), STARTUP_DELAY_MS);
> + this._delayedLoadTimerId = setTimeout(this._loadSelfSupport.bind(this), STARTUP_DELAY_MS);
Getting rid of this timer likely deserves its own bug (blocking bug 1354723). There's nothing that guarantees startup will be done after 5s.
Attachment #8861084 -
Flags: review?(florian) → review-
Comment 31•8 years ago
|
||
I think the work done here will help to do something like bug 1354820 in the future. It'll become easier to unload modules that haven't been used in a while and to reload them later once we have the knowledge about the observer topics and messages each js module cares about in a central place.
Comment 32•8 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #28)
> Comment on attachment 8861082 [details]
> Bug 1358921: Lazily load LoginManagerParent.jsm when first needed.
>
> https://reviewboard.mozilla.org/r/132722/#review136152
>
> > I appriciate the effort to consolidate module initializitian code and reduce boilerplate, but I think it's rather unfortunate that your patches bake module logic into nsBrowserGlue. This is particular problematic for toolkit modules. I think we should aim to keep modules self-contained.
>
> This keeps the module self-contained enough in my opinion, in that the
> module is still usable without the nsBrowserGlue logic.
That's a weird definition of self-containedness. Any code can be made usable if you add enough code around it. :p
Can we have the perf gains without having nsBrowserGlue manage all the notification topics that a module needs internally?
Comment 33•8 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #32)
> Can we have the perf gains without having nsBrowserGlue manage all the
> notification topics that a module needs internally?
The gain here is due to not reading the module's file (and its dependencies) until we actually need it, so you can't get that gain without losing a bit of self-containedness. If you think what nsBrowserGlue does here should be moved to toolkit/ and nsBrowserGlue should only extend it... that would be a reasonable idea; but I'm not going to require Kris to do it here. I think that would be more for a low priority follow-up.
Comment 34•8 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #33)
> (In reply to Dão Gottwald [::dao] from comment #32)
>
> > having nsBrowserGlue manage all the
> > notification topics that a module needs internally?
Also, nsBrowserGlue shouldn't manage all the notification topics that these modules need internally, but only the topics that indicate we need that module. A few of my review comments above are about not triggering a load of the module for topics that are only used to uninitialize modules.
Comment 35•8 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #34)
> (In reply to Florian Quèze [:florian] [:flo] from comment #33)
> > (In reply to Dão Gottwald [::dao] from comment #32)
> >
> > > having nsBrowserGlue manage all the
> > > notification topics that a module needs internally?
>
> Also, nsBrowserGlue shouldn't manage all the notification topics that these
> modules need internally, but only the topics that indicate we need that
> module.
The patches here seem to go further in that they move the registration for topics that will only be needed when the module was already initialized.
(In reply to Florian Quèze [:florian] [:flo] from comment #33)
> (In reply to Dão Gottwald [::dao] from comment #32)
>
> > Can we have the perf gains without having nsBrowserGlue manage all the
> > notification topics that a module needs internally?
>
> The gain here is due to not reading the module's file (and its dependencies)
> until we actually need it, so you can't get that gain without losing a bit
> of self-containedness. If you think what nsBrowserGlue does here should be
> moved to toolkit/ and nsBrowserGlue should only extend it... that would be a
> reasonable idea; but I'm not going to require Kris to do it here. I think
> that would be more for a low priority follow-up.
I think ideally this info would come directly from the module or a manifest (e.g. moz.build).
Assignee | ||
Comment 36•8 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #18)
> I appriciate the effort to consolidate module initializitian code and reduce
> boilerplate, but I think it's rather unfortunate that your patches bake
> module logic into nsBrowserGlue. This is particular problematic for toolkit
> modules. I think we should aim to keep modules self-contained.
I don't particularly like it either, and I say this as a toolkit peer and someone who spends most of my time working in toolkit code. But there aren't many places where we can cut more than 11ms (on a fast machine) off of startup time with so little effort or disruption. And I think we could easily cut another 20-30ms off by extending this approach to other modules.
Ideally, I'd like this information to be stored in something like a per-module manifest, and then a final manifest generated in a build step, but I don't think these changes should block on that.
We're going to be shipping system add-ons as WebExtensions in production very soon. We're already shipping the screenshots add-on with a delayed initialization of its WebExtension code. But when we ship add-ons that need to be initialized at startup, we're going to need to initialize a fair amount of extra framework code. I've done just about as much as I can to make that startup process as efficient and lazy as possible, but in the end, it's still a lot of extra code we're loading and startup work we're doing.
So the extra startup time has to come from somewhere, and it has to come from somewhere soon, or we're going to wind up with a startup perf regression at a time when we're aiming for the exact opposite.
(In reply to Dão Gottwald [::dao] from comment #35)
> The patches here seem to go further in that they move the registration for
> topics that will only be needed when the module was already initialized.
If that's the case, I'm happy to change them. There are definitely one or two that shouldn't be needed until a module has been loaded, and I agree that those shouldn't be registered in nsBrowserGlue, even if only because they would force loading the module at shutdown when it isn't needed.
> (In reply to Florian Quèze [:florian] [:flo] from comment #33)
> I think ideally this info would come directly from the module or a manifest
> (e.g. moz.build).
I'd be happy to work on that as a follow-up, but I (along with pretty much everyone else) have a lot of work to get done before 57, so I'd really rather not block on it.
Comment 37•8 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #36)
> (In reply to Dão Gottwald [::dao] from comment #35)
> > The patches here seem to go further in that they move the registration for
> > topics that will only be needed when the module was already initialized.
>
> If that's the case, I'm happy to change them. There are definitely one or
> two that shouldn't be needed until a module has been loaded, and I agree
> that those shouldn't be registered in nsBrowserGlue, even if only because
> they would force loading the module at shutdown when it isn't needed.
I was looking at AboutHome.jsm. Only AboutHome:RequestUpdate and AboutHome:MaybeShowAutoMigrationUndoNotification should initialize the module, although I think it could be considered a bug that AboutHome:MaybeShowAutoMigrationUndoNotification is sent before AboutHome:RequestUpdate, or that it exists at all besides AboutHome:RequestUpdate.
Assignee | ||
Comment 38•8 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #37)
> I was looking at AboutHome.jsm. Only AboutHome:RequestUpdate and
> AboutHome:MaybeShowAutoMigrationUndoNotification should initialize the
> module, although I think it could be considered a bug that
> AboutHome:MaybeShowAutoMigrationUndoNotification is sent before
> AboutHome:RequestUpdate, or that it exists at all besides
> AboutHome:RequestUpdate.
It feels safer to assume that any of those messages can initialize the module, in case some changes down the road cause one of the others to happen earlier. If you feel particularly strongly about it, though, I can move the others to AboutHome.jsm.
Comment 39•8 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #37)
> (In reply to Kris Maglione [:kmag] from comment #36)
> > (In reply to Dão Gottwald [::dao] from comment #35)
> > > The patches here seem to go further in that they move the registration for
> > > topics that will only be needed when the module was already initialized.
> >
> > If that's the case, I'm happy to change them. There are definitely one or
> > two that shouldn't be needed until a module has been loaded, and I agree
> > that those shouldn't be registered in nsBrowserGlue, even if only because
> > they would force loading the module at shutdown when it isn't needed.
>
> I was looking at AboutHome.jsm. Only AboutHome:RequestUpdate and
> AboutHome:MaybeShowAutoMigrationUndoNotification should initialize the
> module, although I think it could be considered a bug that
> AboutHome:MaybeShowAutoMigrationUndoNotification is sent before
> AboutHome:RequestUpdate, or that it exists at all besides
> AboutHome:RequestUpdate.
I could have pointed out some webrtc related messages that can never be the cause of initializing webrtcUI (eg. messages that happen when a stream ends), but I didn't because I think it's clearer to keep all the related listeners defined in a single place.
Comment 40•8 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #38)
> (In reply to Dão Gottwald [::dao] from comment #37)
> > I was looking at AboutHome.jsm. Only AboutHome:RequestUpdate and
> > AboutHome:MaybeShowAutoMigrationUndoNotification should initialize the
> > module, although I think it could be considered a bug that
> > AboutHome:MaybeShowAutoMigrationUndoNotification is sent before
> > AboutHome:RequestUpdate, or that it exists at all besides
> > AboutHome:RequestUpdate.
>
> It feels safer to assume that any of those messages can initialize the
> module, in case some changes down the road cause one of the others to happen
> earlier. If you feel particularly strongly about it, though, I can move the
> others to AboutHome.jsm.
AboutHome:RequestUpdate is needed to initialize the page. It's basically the load event listener. AboutHome:Downloads and friends respond to clicks in the page. I don't think it makes sense to already prepare for unknown changes that would somehow change this.
Updated•8 years ago
|
Whiteboard: [qf] → [qf:p1]
Assignee | ||
Comment 41•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8861074 [details]
Bug 1358921: Lazily load webrtcUI.jsm when first needed.
https://reviewboard.mozilla.org/r/132706/#review136154
> Do we really need this '|| []'? Isn't it a bug of the listeners object if we added an observer for a topic that isn't in the observers object?
Probably not. I tend to just be paranoid about this sort of thing.
> Could this be simplified to:
> for (let topic in this.observers)
> ?
It could, and that would be slightly faster, but `Object.keys` is slightly safer because it doesn't look at prototypes.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Comment 53•8 years ago
|
||
mozreview-review |
Comment on attachment 8861074 [details]
Bug 1358921: Lazily load webrtcUI.jsm when first needed.
https://reviewboard.mozilla.org/r/132706/#review138602
::: browser/components/nsBrowserGlue.js:990
(Diff revision 2)
> SelfSupportBackend.uninit();
> PageThumbs.uninit();
> NewTabMessages.uninit();
> AboutNewTab.uninit();
> - webrtcUI.uninit();
> +
> + if (initializedModules.webrtcUI) {
I expected this to be more generalized using a loop, eg.
for (let module in initializedModules) {
if (initializedModules[module].uninit)
initializedModules[module].uninit()
}
But it's fine to keep for a follow-up bug where we'll convert more modules.
Attachment #8861074 -
Flags: review?(florian) → review+
Comment 54•8 years ago
|
||
mozreview-review |
Comment on attachment 8861076 [details]
Bug 1358921: Lazily load ContentSearch.jsm when first needed.
https://reviewboard.mozilla.org/r/132710/#review138604
Attachment #8861076 -
Flags: review?(florian) → review+
Comment 55•8 years ago
|
||
mozreview-review |
Comment on attachment 8861079 [details]
Bug 1358921: Lazily load RemotePrompt.jsm when first needed.
https://reviewboard.mozilla.org/r/132716/#review138606
Attachment #8861079 -
Flags: review?(florian) → review+
Comment 56•8 years ago
|
||
mozreview-review |
Comment on attachment 8861081 [details]
Bug 1358921: Lazily load ContentPrefServiceParent.jsm when first needed.
https://reviewboard.mozilla.org/r/132720/#review138608
Attachment #8861081 -
Flags: review?(florian) → review+
Comment 57•8 years ago
|
||
mozreview-review |
Comment on attachment 8861082 [details]
Bug 1358921: Lazily load LoginManagerParent.jsm when first needed.
https://reviewboard.mozilla.org/r/132722/#review138610
Attachment #8861082 -
Flags: review?(florian) → review+
Comment 58•8 years ago
|
||
mozreview-review |
Comment on attachment 8861084 [details]
Bug 1358921: Lazily load SelfSupportBackend.jsm when first needed.
https://reviewboard.mozilla.org/r/132726/#review138612
Attachment #8861084 -
Flags: review?(florian) → review+
Comment 59•8 years ago
|
||
I'm looking forward to profiling startup in a nightly containing these patches :-).
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 71•8 years ago
|
||
Hm. Well, trees are closed, so I tried to use autoland for once, and it decided to do the thing where it says that I can't land because there are open issues, while also insisting that there are no open issues. So I guess it can wait until tomorrow, and hopefully doesn't sprout new conflicts while I'm asleep.
Assignee | ||
Comment 72•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb1d6244cff421aebfe96027d9b0cb617a9c7e2c
Bug 1358921: Lazily load webrtcUI.jsm when first needed. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/92cca411de48b934d171f3e6201064dea77cf24c
Bug 1358921: Lazily load AboutHome.jsm when first needed. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/84909c2a4dbba816efd70963b01cc5e757cb0d76
Bug 1358921: Lazily load ContentSearch.jsm when first needed. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7b4d6fa41ed8accca24641ef513b6636eb31bb6
Bug 1358921: Lazily load FormValidationHandler.jsm when first needed. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/0136cdc5153b2198bfbef5656ba0877288f1a803
Bug 1358921: Lazily load ContentClick.jsm when first needed. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/03bca4633fa48af23c2f0817ad3b29ab16149baf
Bug 1358921: Lazily load RemotePrompt.jsm when first needed. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/52784c836d17ee7b79b94ff6edf1536d99a05113
Bug 1358921: Lazily load Feeds.jsm when first needed. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e73eea7f9c7721e5a59b2834eed8584564d335c
Bug 1358921: Lazily load ContentPrefServiceParent.jsm when first needed. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef15bcf434bb935086282405ffa21a0eee68736
Bug 1358921: Lazily load LoginManagerParent.jsm when first needed. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/34d02221bd0496a1238261570a4fb14c4b462628
Bug 1358921: Lazily load ReaderParent.jsm when first needed. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/234aa4131c4e293147e8e7d74fa631fc3080db86
Bug 1358921: Lazily load SelfSupportBackend.jsm when first needed. r=florian
Comment 73•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eb1d6244cff4
https://hg.mozilla.org/mozilla-central/rev/92cca411de48
https://hg.mozilla.org/mozilla-central/rev/84909c2a4dbb
https://hg.mozilla.org/mozilla-central/rev/d7b4d6fa41ed
https://hg.mozilla.org/mozilla-central/rev/0136cdc5153b
https://hg.mozilla.org/mozilla-central/rev/03bca4633fa4
https://hg.mozilla.org/mozilla-central/rev/52784c836d17
https://hg.mozilla.org/mozilla-central/rev/8e73eea7f9c7
https://hg.mozilla.org/mozilla-central/rev/3ef15bcf434b
https://hg.mozilla.org/mozilla-central/rev/34d02221bd04
https://hg.mozilla.org/mozilla-central/rev/234aa4131c4e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 74•8 years ago
|
||
there is a windows startup performance improvement from this patch:
== Change summary for alert #6351 (as of May 03 2017 16:23 UTC) ==
Improvements:
4% ts_paint windows7-32 pgo e10s 896.89 -> 860.25
3% ts_paint windows8-64 opt 939.42 -> 914.75
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6351
Updated•8 years ago
|
Blocks: webext-perf
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•