1001131 - Reduce devtools footprint at browser startup

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

There are a lot of devtools modules loaded at start up that don't need to be. That increases the browsers memory footprint for users that aren't even using devtools. We should fix this.

Comment 1

[Girish Sharma [:Optimizer]]

This is the complete list of devtools related code being loaded at firefox startup

browser/components/devtools-clhandler.js
resource:///modules/devtools/ToolboxProcess.jsm
resource:///modules/devtools/ViewHelpers.jsm
resource:///modules/devtools/gDevTools.jsm
resource:///modules/devtools/main.js (from: resource://gre/modules/commonjs/tool
kit/loader.js:232)
resource:///modules/devtools/scratchpad-manager.jsm
resource:///modules/devtools/shared/telemetry.js (from: resource://gre/modules/c
ommonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/Console.jsm
resource://gre/modules/devtools/DevToolsUtils.js (from: resource://gre/modules/c
ommonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/DevToolsUtils.jsm
resource://gre/modules/devtools/Loader.jsm
resource://gre/modules/devtools/Require.jsm
resource://gre/modules/devtools/SourceMap.jsm
resource://gre/modules/devtools/event-emitter.js
resource://gre/modules/devtools/event-emitter.js (from: resource://gre/modules/c
ommonjs/toolkit/loader.js:232)
resource://gre/modules/jsdebugger.jsm

only 4 files are laoded via the devtools loader (that have a bracket from:... with them)

Others are directly loaded via Cu.import

I am trying to put the dependency of these files in tree form:

content/browser.js - on delayed startup method
├──resource:///modules/devtools/ToolboxProcess.jsm - Used in xul command key - so loaded asap whatsoever.
│  └──resource:///modules/devtools/ViewHelpers.jsm - can be lazy loaded
│     └──resource://gre/modules/devtools/DevToolsUtils.jsm - can be lazy loaded (but not needed if viewhelpers is lazy loaded)
├──resource:///modules/devtools/scratchpad-manager.jsm - same as toolboxprocess.jsm
└──resource:///modules/devtools/gDevTools.jsm - same as toolboxprocess.jsm
   ├──resource://gre/modules/devtools/event-emitter.js - can be lazy loaded
   └──resource://gre/modules/devtools/Loader.jsm - can be lazy loaded
      ├──resource:///modules/devtools/main.js
      ├──resource://gre/modules/devtools/Require.jsm
      └──resource://gre/modules/devtools/SourceMap.jsm

As for Console, there are so many non-devtools files importing it too, and maybe using it too. So I think that is a can't-remove.

I have no idea where devtools-clhandler.js and devtools/shared/telemetry.js is being imported from. While clhandler seems required, no idea about telemetry.js

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to Girish Sharma [:Optimizer] from comment #1)

> ├──resource:///modules/devtools/ToolboxProcess.jsm - Used in xul command key
> - so loaded asap whatsoever.
> ├──resource:///modules/devtools/scratchpad-manager.jsm - same as
> toolboxprocess.jsm
> └──resource:///modules/devtools/gDevTools.jsm - same as toolboxprocess.jsm

These all look like they're only accessed if the relevant commands are invoked, so lazy-importing them would be fine.

> I have no idea where devtools-clhandler.js

That's an XPCOM command line handler. Probably not an easy way to get rid of that, unless we can merge it with the default browser command line handler in nsBrowserContentHandler.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Oh, I was wrong about gDevTools.jsm, that one is used on window open to call registerBrowserWindow.

Comment 4

[Girish Sharma [:Optimizer]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #2)
> (In reply to Girish Sharma [:Optimizer] from comment #1)
> 
> > ├──resource:///modules/devtools/ToolboxProcess.jsm - Used in xul command key
> > - so loaded asap whatsoever.
> > ├──resource:///modules/devtools/scratchpad-manager.jsm - same as
> > toolboxprocess.jsm
> > └──resource:///modules/devtools/gDevTools.jsm - same as toolboxprocess.jsm
> 
> These all look like they're only accessed if the relevant commands are
> invoked, so lazy-importing them would be fine.
> 

They already are lazy imported. No idea why they are loaded asap.

Anyways, I was thinking of creating a common file for shortcuts, which will then in turn lazy load these files. This way, there will be only single very light weight file loaded from browser.js meant for shortcuts.

Comment 5

[Girish Sharma [:Optimizer]]

Well, actually, in latest fx-team, the devtools related list is at least double that size.

bug 942756 made it so that all of non-required devtools server related code is also loaded at firefox startup. Thus at least 12 more modules are added into the list in comment 1 :(

Comment 6

[Girish Sharma [:Optimizer]]

Joe, I am also seeing around 10+ gcli related files, including gcli.jsm, at startup of firefox (new profile).

Are they required to be loaded at startup ?

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

No devtools code should ideally be loaded at startup or at any other time really, until the user tries to open one of the tools.

Comment 8

[Girish Sharma [:Optimizer]]

Okay then I will change the question.

Joe, do you know the entry point of the gcli files ?

Comment 9

[Girish Sharma [:Optimizer]]

(In reply to Girish Sharma [:Optimizer] from comment #5)
> Well, actually, in latest fx-team, the devtools related list is at least
> double that size.
> 
> bug 942756 made it so that all of non-required devtools server related code
> is also loaded at firefox startup. Thus at least 12 more modules are added
> into the list in comment 1 :(

Paul said he will take care of that.

Comment 10

[Girish Sharma [:Optimizer]]

gDevTools.jsm cannot be avoided to load.
Which in turns make event-emitter and Loader.jsm and browser/devtools/main.js unavoidable.

SourceMap.jsm on the other hand should not be loaded at all. But it is being referenced in Loader.jsm

function BuiltinProvider() {}
BuiltinProvider.prototype = {
  load: function() {
    this.loader = new loader.Loader({
      modules: {
        "Debugger": Debugger,
        "Services": Object.create(Services),
        "Timer": Object.create(Timer),
        "toolkit/loader": loader,
        "source-map": SourceMap,

Nick, can that be removed, and where ever this is actually required, we import SourceMap.jsm there itself ?

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Yeah you can just use XPCOMUtils.defineLazyModuleGetter

Comment 12

[Girish Sharma [:Optimizer]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> Yeah you can just use XPCOMUtils.defineLazyModuleGetter

If you mean, lazy-load SourceMap.jsm in main.js, then it does not help, as the reference is called on startup itself.

What I meant was, to remove that key-value pair from DevToolsLoader completely and import SourceMap.jsm wherever required, like pretty-fast.js, etc.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I think it can be.

I was going to say that third party code depended on the source map module that we didn't want to maintain patches against, but that hasn't been true since we stopped using escodegen for pretty printing. I think it can go from Loader.jsm.

Comment 14

[Girish Sharma [:Optimizer]]

ToolboxProcess.jsm is loaded via XPIProvider.jsm, for which I have a patch to fix it.

The final bit, sctarchpad-manager.jsm is loaded via  a reference in SessionStore.jsm while trying to save the current session.

Comment 15

[Girish Sharma [:Optimizer]]

(For reference, these are all the gcli related/loaded 52 files:)

resource://gre/modules/devtools/gcli.jsm
resource://gre/modules/devtools/gcli/api.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/cli.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/commands/clear.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/commands/commands.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/commands/context.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/commands/global.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/commands/help.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/commands/lang.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/commands/pref.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/connectors/connectors.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/converters/basic.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/converters/converters.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/converters/terminal.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/fields/delegate.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/fields/fields.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/fields/selection.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/index.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/languages/command.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/languages/javascript.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/languages/languages.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/settings.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/array.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/boolean.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/command.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/date.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/delegate.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/file.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/fileparser.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/javascript.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/node.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/number.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/resource.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/selection.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/setting.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/string.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/types/types.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/ui/focus.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/ui/intro.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/ui/menu.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/ui/view.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/util/domtemplate.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/util/fileparser.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/util/filesystem.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/util/host.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/util/l10n.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/util/prism.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/util/promise.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/util/spell.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/gcli/util/util.js (from: resource://gre/modules/commonjs/toolkit/loader.js:232)
resource://gre/modules/devtools/Templater.jsm
resource://gre/modules/devtools/deprecated-sync-thenables.js

Comment 16

[Girish Sharma [:Optimizer]]

Attachment: part 1

This patch removes ToolboxProcess, ViewHelper and scratchpad-manager from startup.

Comment 17

[Girish Sharma [:Optimizer]]

So I found out what was causing all those gcli files. It was the Firefox OS simulator which was adding some gcli commands and thus triggering the load of gcli.jsm and thus all gcli files.

I guess, my new profile was not that new after all!

Comment 18

[Girish Sharma [:Optimizer]]

Attachment: patch part 1

This patch removes ToolboxProcess.jsm, one of event-emitter, scratchpad-manager, ViewHelpers and DevToolsUtils from startup.

As far as I can see, only one more module can be removed from the list i.e. SourceMap.jsm

The ones that will be remaining will be:

Console.jsm, Loader.jsm, Require.jsm, gDevTools.jsm, main.js, event-emitter.js and cl-handler.js

Asking review from Blaire for XPIProvider bits and Tim for Session restore bits.

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8413354 [details] [diff] [review]
patch part 1

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

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +3882,5 @@
>        }
>        return;
>      }
> +    else if (aTopic == NOTIFICATION_TOOLBOXPROCESS_LOADED) {
> +      XPIProvider._toolboxProcessLoaded = true;

Move this so it gets set only if BrowserToolboxProcess.on(...) succeeds. That way, you can simplify all the other cases by removing the try/catch.

(The try/catch is a bit redundant in this case too, but it's better to be extra paranoid in this module. But the paranoia is only needed for the first time we access BrowserToolboxProcess, and we're now guaranteeing this is the first time the module is imported.)

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8413354 [details] [diff] [review]
patch part 1

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

Can you please split off the sessionstore changes into its own patch? That makes it a lot easier to deal with multiple reviewers, thanks!

I don't really like the general solution here. Listening for a notification to know when a module has been loaded seems error-prone. What happens when something loads the ScratchpadManager before SessionStore is initialized? That would mean we always discard scratchpads. If maybe the mozJSComponentLoader could somehow expose whether a given location is in mImports we would know whether the file has been loaded already and don't try to if not.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +575,5 @@
>        case "gather-telemetry":
>          this.onGatherTelemetry();
>          break;
> +      case "ScratchpadManagerLoaded":
> +        SessionStoreInternal._scratchpadManagerLoaded = true;

this._scratchpadManagerLoaded = true;

@@ +2123,5 @@
>  
>      // get open Scratchpad window states too
> +    let scratchpads = [];
> +
> +    if (SessionStoreInternal._scratchpadManagerLoaded) {

if (this._scratchpadManagerLoaded) {

@@ +2125,5 @@
> +    let scratchpads = [];
> +
> +    if (SessionStoreInternal._scratchpadManagerLoaded) {
> +      scratchpads = ScratchpadManager.getSessionState();
> +    }

I think we should change this code to only add a .scratchpads property to |state| if the scratchpad manager is loaded and if it returns a non-empty array. Otherwise ScratchpadManager.restoreSession() will always be called for automatically restored sessions at startup and invalidate the effort made here.

Comment 21

[Girish Sharma [:Optimizer]]

(In reply to Tim Taubert [:ttaubert] from comment #20)
> Comment on attachment 8413354 [details] [diff] [review]
> patch part 1
> 
> Review of attachment 8413354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you please split off the sessionstore changes into its own patch? That
> makes it a lot easier to deal with multiple reviewers, thanks!

Sure.

> I don't really like the general solution here. Listening for a notification
> to know when a module has been loaded seems error-prone. What happens when
> something loads the ScratchpadManager before SessionStore is initialized?
> That would mean we always discard scratchpads. If maybe the
> mozJSComponentLoader could somehow expose whether a given location is in
> mImports we would know whether the file has been loaded already and don't
> try to if not.

I also wanted to provide a way to know if a module is already loaded at mozJSComponentLoader, but it felt too specific for just these 2 cases. Also, not everything is loaded via Cu.imports (or XCPomUtils.lazy...) , devtools has its own loader, so the mozJSComponentLoader solution won't work there. But anyways, can you point me to implement such feature in mozJSComponentLoader for now ?

And you are right, for session restore service, the race condition can occur.

> @@ +2125,5 @@
> > +    let scratchpads = [];
> > +
> > +    if (SessionStoreInternal._scratchpadManagerLoaded) {
> > +      scratchpads = ScratchpadManager.getSessionState();
> > +    }
> 
> I think we should change this code to only add a .scratchpads property to
> |state| if the scratchpad manager is loaded and if it returns a non-empty
> array. Otherwise ScratchpadManager.restoreSession() will always be called
> for automatically restored sessions at startup and invalidate the effort
> made here.

Ouch.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Girish Sharma [:Optimizer] from comment #12)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> > Yeah you can just use XPCOMUtils.defineLazyModuleGetter
> 
> If you mean, lazy-load SourceMap.jsm in main.js, then it does not help, as
> the reference is called on startup itself.
> 
> What I meant was, to remove that key-value pair from DevToolsLoader
> completely and import SourceMap.jsm wherever required, like pretty-fast.js,
> etc.

Please don't remove this without consulting Eddy first. He specifically added this to make the debugger server worker-friendly and that means no Components use there. My alternative suggestion was to ask Nick if we could just switch from Cu.import("SourceMap.jsm") to require("source-map.js") or if that would require more work.

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I think we can switch to require("source-map")

Comment 24

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #22)
> (In reply to Girish Sharma [:Optimizer] from comment #12)
> > (In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> > > Yeah you can just use XPCOMUtils.defineLazyModuleGetter
> > 
> > If you mean, lazy-load SourceMap.jsm in main.js, then it does not help, as
> > the reference is called on startup itself.
> > 
> > What I meant was, to remove that key-value pair from DevToolsLoader
> > completely and import SourceMap.jsm wherever required, like pretty-fast.js,
> > etc.
> 
> Please don't remove this without consulting Eddy first. He specifically
> added this to make the debugger server worker-friendly and that means no
> Components use there. My alternative suggestion was to ask Nick if we could
> just switch from Cu.import("SourceMap.jsm") to require("source-map.js") or
> if that would require more work.

(In reply to Nick Fitzgerald [:fitzgen] from comment #23)
> I think we can switch to require("source-map")

The purpose here is to lazy load the sourcemap. Even if we use require or cu.import, it will still get loaded as it is being referenced in the main method of the devtools loader.

What I was suggesting is to remove it from the modules list of the loader and require("sourcemap.jsm") in each file that *actually* need it (for example, pretty-fast).

Not sure how this will impact Eddy's work ?

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Girish Sharma [:Optimizer] from comment #24)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #23)
> > I think we can switch to require("source-map")
> 
> The purpose here is to lazy load the sourcemap. Even if we use require or
> cu.import, it will still get loaded as it is being referenced in the main
> method of the devtools loader.
> 
> What I was suggesting is to remove it from the modules list of the loader
> and require("sourcemap.jsm") in each file that *actually* need it (for
> example, pretty-fast).

Yes, I meant in the modules that actually use it. It should actually be the new |devtools.lazyRequireGetter|, so something like |devtools.lazyRequireGetter(this, "sourceMap", "devtools/toolkit/source-map")|.

Comment 26

[Girish Sharma [:Optimizer]]

How will that work ?

Since, the load method on line 62 of Loader.jsm is called ASAP while browser startup.

That load method creates a new loader like this:

    this.loader = new loader.Loader({
      modules: {
        "Debugger": Debugger,
        "Services": Object.create(Services),
        "Timer": Object.create(Timer),
        "toolkit/loader": loader,
        "source-map": SourceMap,
      },
      paths: {
        // When you add a line to this mapping, don't forget to make a
        // corresponding addition to the SrcdirProvider mapping below as well.
        "": "resource://gre/modules/commonjs/",
        "main": "resource:///modules/devtools/main.js",
        "devtools": "resource:///modules/devtools",
        "devtools/toolkit": "resource://gre/modules/devtools",
        "devtools/server": "resource://gre/modules/devtools/server",
        "devtools/toolkit/webconsole": "resource://gre/modules/devtools/toolkit/webconsole",
        "devtools/app-actor-front": "resource://gre/modules/devtools/app-actor-front.js",
        "devtools/styleinspector/css-logic": "resource://gre/modules/devtools/styleinspector/css-logic",
        "devtools/css-color": "resource://gre/modules/devtools/css-color",
        "devtools/output-parser": "resource://gre/modules/devtools/output-parser",
        "devtools/touch-events": "resource://gre/modules/devtools/touch-events",
        "devtools/client": "resource://gre/modules/devtools/client",
        "devtools/pretty-fast": "resource://gre/modules/devtools/pretty-fast.js",
        "devtools/async-utils": "resource://gre/modules/devtools/async-utils",
        "devtools/content-observer": "resource://gre/modules/devtools/content-observer",
        "gcli": "resource://gre/modules/devtools/gcli",
        "acorn": "resource://gre/modules/devtools/acorn",
        "acorn/util/walk": "resource://gre/modules/devtools/acorn/walk.js",

        // Allow access to xpcshell test items from the loader.
        "xpcshell-test": "resource://test"
      },
      globals: loaderGlobals,
      invisibleToDebugger: this.invisibleToDebugger
    });


Now how so ever you lazify the variable "sourceMap" , the module will anyways get loaded at Browser startup.

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

Eddy added SourceMap.jsm as a loader module in bug 859372. My suggestion is to undo that particular change and replace the Cu.imports in the original locations with require("source-map").

Comment 28

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #27)
> Eddy added SourceMap.jsm as a loader module in bug 859372. My suggestion is
> to undo that particular change and replace the Cu.imports in the original
> locations with require("source-map").

hg blame tells me that it was Nick (Bug 908913)

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

Ah, good catch. The idea still stands however, use require, or even better the lazyRequireGetter that Nick mentions in comment 25, in every file that deals with source maps, *not* in the loader.

Comment 30

[Girish Sharma [:Optimizer]]

Opening a new bug (bug 1004485) for the actual s/require/lazyRequire as this bug was mostly meant to reduce the startup footprint of devtools code.

Comment 31

[Girish Sharma [:Optimizer]]

Attachment: mem-sessionstore

Session restore only patch. This patch uses the Cu.isLoaded from bug 1004487.

Comment 32

[Girish Sharma [:Optimizer]]

Attachment: mem-xpiprovider

Made the changes requested. I did not move the flag setting to true inside the .on call as it seemed like that the isLoaded + notification now guarantees the loading of the toolbox process.

Comment 33

[Girish Sharma [:Optimizer]]

Attachment: mem-sourcemap

These are the sourcemap related changes + small change in gDevTools to require event emitter instead of importing it.

Comment 34

[Girish Sharma [:Optimizer]]

try [0] with everything green except X and dt which is fixed in the next try [1]

[0] https://tbpl.mozilla.org/?tree=Try&rev=8026aaf0812d
[1] https://tbpl.mozilla.org/?tree=Try&rev=6c464e889699

Comment 35

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8415891 [details] [diff] [review]
mem-sourcemap

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

Because pretty-fast is a (somewhat) independent project maintained upstream, making changes to our tree's version without landing them upstream is not ideal. It makes it harder to upgrade to newer versions. So if you are going to change the pretty-fast library, you should submit a pull request before or at the same time. However, pretty-fast also works with node and these changes unnecessarily break that. Instead of changing pretty-fast, we should just define the source-map module in the loader as the path to the jsm (instead of as an existing module, like it is now).

::: browser/devtools/framework/gDevTools.jsm
@@ +13,2 @@
>  Cu.import("resource://gre/modules/devtools/Loader.jsm");
> +const EventEmitter = devtools.require("devtools/toolkit/event-emitter");

This doesn't make the EventEmitter lazy, is this just an effort to use require more?

Comment 36

[Girish Sharma [:Optimizer]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #35)
> Comment on attachment 8415891 [details] [diff] [review]
> mem-sourcemap
> 
> Review of attachment 8415891 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Because pretty-fast is a (somewhat) independent project maintained upstream,
> making changes to our tree's version without landing them upstream is not
> ideal. It makes it harder to upgrade to newer versions. So if you are going
> to change the pretty-fast library, you should submit a pull request before
> or at the same time. However, pretty-fast also works with node and these
> changes unnecessarily break that. Instead of changing pretty-fast, we should
> just define the source-map module in the loader as the path to the jsm
> (instead of as an existing module, like it is now).

alright

> ::: browser/devtools/framework/gDevTools.jsm
> @@ +13,2 @@
> >  Cu.import("resource://gre/modules/devtools/Loader.jsm");
> > +const EventEmitter = devtools.require("devtools/toolkit/event-emitter");
> 
> This doesn't make the EventEmitter lazy, is this just an effort to use
> require more?

Event emitter will not benefit even if I make it lazy, as it is accessed instantly. What this change does is that reduce the number of event emitters loaded at startup from 2 (1 from import and 1 from require) to 1 (from require)

Comment 37

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8415886 [details] [diff] [review]
mem-xpiprovider

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

This makes me think we should (eventually - not as a requirement for this bug) have a helper function for this type of pattern, as part of XPCOMUtils.jsm. Filed bug 1004885.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +3891,5 @@
>          this.importPermissions();
>        }
>        return;
>      }
> +    else if (aTopic == NOTIFICATION_TOOLBOXPROCESS_LOADED) {

Should be removing the notification observer here.

Comment 38

[Girish Sharma [:Optimizer]]

Attachment: mem-sessionstore

changed the name to Cu.isModuleLoaded

Comment 39

[Girish Sharma [:Optimizer]]

Attachment: mem-xpiprovider

changed name to Cu.isModuleLoaded, removed the notification observer after one hit.

Comment 40

[Girish Sharma [:Optimizer]]

Attachment: mem-sourcemap

Made the requested change. source-map is now part of paths . No change to any upstream files.

try for all patches : https://tbpl.mozilla.org/?tree=Try&rev=2cf3e7241ee8

Comment 41

[Girish Sharma [:Optimizer]]

Attachment: mem-sourcemap

Forgot to add source-map in the SrcdirProvider loader paths. But its so weird that a oth test will fail. What is a devtools test doing in oth ? :)

new try with oth dt and X https://tbpl.mozilla.org/?tree=Try&rev=b681e0228a1d

Comment 42

[Girish Sharma [:Optimizer]]

(Review ping to Tim)

Comment 43

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8420604 [details] [diff] [review]
mem-sessionstore

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

Thanks!

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +34,5 @@
>    "quit-application-requested", "quit-application-granted",
>    "browser-lastwindow-close-granted",
>    "quit-application", "browser:purge-session-history",
>    "browser:purge-domain-data",
> +  "gather-telemetry", "ScratchpadManagerLoaded",

This can be removed.

@@ +2103,5 @@
>      // get open Scratchpad window states too
> +    let scratchpads = null;
> +
> +    if (this._scratchpadManagerLoaded ||
> +        Cu.isModuleLoaded("resource:///modules/devtools/scratchpad-manager.jsm")) {

I'd rather have us call isModuleLoaded() every time and remove the _scratchpadmanagerLoaded flag. The overhead should be negligible and makes the code a lot more elegant.

@@ +2113,5 @@
>        windows: total,
>        selectedWindow: ix + 1,
>        _closedWindows: lastClosedWindowsCopy,
>        session: session,
>        scratchpads: scratchpads,

We should omit setting the property altogether if there are no scratchpads to store. It would be great to do the same if .getSessionState() returns an empty array.

::: browser/devtools/scratchpad/scratchpad-manager.jsm
@@ +172,5 @@
>      Services.obs.removeObserver(this, "quit-application-granted");
>    }
>  };
> +
> +Services.obs.notifyObservers(null, "ScratchpadManagerLoaded", null);

This can be removed.

Comment 44

[Girish Sharma [:Optimizer]]

Attachment: mem-sessionstore v2

review comments addressed.

Comment 45

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8425115 [details] [diff] [review]
mem-sessionstore v2

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

Great!

Comment 46

[Girish Sharma [:Optimizer]]

Attachment: mem-xpiprovider v2

rebased.

Comment 47

[Girish Sharma [:Optimizer]]

Attachment: mem-sourcemap v2

rebased

new try : https://tbpl.mozilla.org/?tree=Try&rev=5291cd68659a

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/48c4e53b876f
https://hg.mozilla.org/integration/fx-team/rev/61101a4d27b9
https://hg.mozilla.org/integration/fx-team/rev/576a3dc6c7a7

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48c4e53b876f
https://hg.mozilla.org/mozilla-central/rev/61101a4d27b9
https://hg.mozilla.org/mozilla-central/rev/576a3dc6c7a7