Lazy load of builtin global and modules is broken

RESOLVED FIXED in Firefox 53

Status

defect
P1
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Blocks 1 bug)

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Everytime we require a pseudo module or access a magic global, the getters defined in builtin-modules are called. This is no longer memoized.
It is especially visible on CSS global when opening the inspector.
And is really bad as we spawn a new sandbox for each call and so a new compartment.
Tentatively assigning to :ochameau, since he appears to be investigating.
Assignee: nobody → poirot.alex
Priority: -- → P1
I'm moving forward with this one, without real proof of improvement as this is clearly broken,
and creating new compartment is obviously going to cost a lot in term of memory!

I just saw a possible improvement from 7.3 to 6.9seconds to load a toolbox with the inspector.
This isn't as obvious as bug 1320149 and bug 1320939.
Comment on attachment 8815250 [details]
Bug 1320793 - Fix lazy loading of pseudo modules in devtools.

https://reviewboard.mozilla.org/r/96260/#review96546

I think I need more explanation for this part.

::: devtools/shared/Loader.jsm:187
(Diff revision 1)
>      let loader = this._provider.loader;
>      for (let id in modules) {
> -      let exports = modules[id];
>        let uri = resolveURI(id, loader.mapping);
> -      loader.modules[uri] = { exports };
> +      loader.modules[uri] = {
> +        get exports() {

Hmm, does this actually have an effect?  It seems like all of things in `exports.modules` of `builtin-modules.js` are eagerly loaded anyway.
Attachment #8815250 - Flags: review?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Comment on attachment 8815250 [details]
> Bug 1320793 - Fix lazy loading of pseudo modules in devtools.
> 
> https://reviewboard.mozilla.org/r/96260/#review96546
> 
> I think I need more explanation for this part.
> 
> ::: devtools/shared/Loader.jsm:187
> (Diff revision 1)
> >      let loader = this._provider.loader;
> >      for (let id in modules) {
> > -      let exports = modules[id];
> >        let uri = resolveURI(id, loader.mapping);
> > -      loader.modules[uri] = { exports };
> > +      loader.modules[uri] = {
> > +        get exports() {
> 
> Hmm, does this actually have an effect?  It seems like all of things in
> `exports.modules` of `builtin-modules.js` are eagerly loaded anyway.

Debugger, timer, xpcInspector and FileReader are lazy getters:
http://searchfox.org/mozilla-central/source/devtools/shared/builtin-modules.js#181-212
and evaluating `modules[id]` right here makes these lazy getter useless as they are all fetched here instead of being fetch on-demand by modules using each of these builtins.
Comment on attachment 8815252 [details]
Bug 1320793 - Prevent loading Debugger module in the parent process.

https://reviewboard.mozilla.org/r/96264/#review96548

::: devtools/server/actors/webbrowser.js
(Diff revision 1)
>  } = require("devtools/server/actors/common");
>  var { DebuggerServer } = require("devtools/server/main");
>  var DevToolsUtils = require("devtools/shared/DevToolsUtils");
>  var { assert } = DevToolsUtils;
>  var { TabSources } = require("./utils/TabSources");
> -var makeDebugger = require("./utils/make-debugger");

What's the evidence around this one?  For a utility named `makeDebugger`, I think it's reasonable to assume it does nothing until called (so just requiring it should be side effect free).

If it's actually doing something bad (all I noticed is `require("Debugger")`, but that doesn't actually _make_ a debugger...), then I think it would be better to change `makeDebugger` to be more lazy inside the module itself so that users aren't surprised like this.
Attachment #8815252 - Flags: review?(jryans)
Comment on attachment 8815250 [details]
Bug 1320793 - Fix lazy loading of pseudo modules in devtools.

https://reviewboard.mozilla.org/r/96260/#review96550

Ah, thanks for the correction!  I guess I skimmed over the lazy ones when reading.
Attachment #8815250 - Flags: review+
Comment on attachment 8815252 [details]
Bug 1320793 - Prevent loading Debugger module in the parent process.

https://reviewboard.mozilla.org/r/96264/#review96552

::: devtools/server/actors/webbrowser.js
(Diff revision 1)
>  } = require("devtools/server/actors/common");
>  var { DebuggerServer } = require("devtools/server/main");
>  var DevToolsUtils = require("devtools/shared/DevToolsUtils");
>  var { assert } = DevToolsUtils;
>  var { TabSources } = require("./utils/TabSources");
> -var makeDebugger = require("./utils/make-debugger");

Ah okay, after your comment I see now that `require("Debugger")` is actually doing some things.

Still, I think we should make that part lazy inside the `makeDebugger` module, instead of this change.
Comment on attachment 8815251 [details]
Bug 1320793 - Fix lazy load of module globals in devtools.

https://reviewboard.mozilla.org/r/96262/#review96554

Great work!  This one seems to remove ~100 compartments in the content process when using the inspector on mozilla.org.
Attachment #8815251 - Flags: review?(jryans) → review+
Comment on attachment 8815252 [details]
Bug 1320793 - Prevent loading Debugger module in the parent process.

https://reviewboard.mozilla.org/r/96264/#review96552

> Ah okay, after your comment I see now that `require("Debugger")` is actually doing some things.
> 
> Still, I think we should make that part lazy inside the `makeDebugger` module, instead of this change.

I could make Debugger being lazy loaded from makeDebugger.js, but still there is no need to load makeDebugger at all.
Honestly I don't care much. Debugger creates a compartment/sandbox whereas makeDebugger doesn't thanks to the shared sandbox trick.
So it is an order of magnitude lower in term of memory. I think there is better laziness to introduce. In the parent process we don't need TabActor which uses makeDebugger, whereas we need BrowserTabActor. But both are in this file.

I'll drop this patch in favor of something more significant and meaningful. The lazyness in make-debugger.js or webbrowser.js I introduced isn't easy to understand.
Attachment #8815252 - Attachment is obsolete: true
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5bb7341ad8a
Fix lazy loading of pseudo modules in devtools. r=jryans
https://hg.mozilla.org/integration/autoland/rev/5c3271cb35c2
Fix lazy load of module globals in devtools. r=jryans
cc: gabor, I think you will be happy to hear about comment 12.
Blocks: 1321096
https://hg.mozilla.org/mozilla-central/rev/b5bb7341ad8a
https://hg.mozilla.org/mozilla-central/rev/5c3271cb35c2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(In reply to Alexandre Poirot [:ochameau] from comment #17)
> Tried to see if DAMP reports any win, but no.
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=f637870222111ca8e90da97c9d38df11
> 0c0d902b&newProject=try&newRevision=7b7ca350b63d1688ab4780090a5f69dd3ad2d4b7&
> framework=1&showOnlyImportant=0
> 
> This is sad as this patch clearly fixes a perf issues.

First of all, keep the fixes coming!  If they aren't showing up on DAMP then we should fix DAMP.  In local testing were you seeing an improvement of toolbox startup time with these changes?  That's the primary thing that feeds into the DAMP score, so if we want to track other metrics, like memory usage, we'd have to add them in.
Flags: needinfo?(poirot.alex)
(In reply to Brian Grinstead [:bgrins] from comment #18)
> (In reply to Alexandre Poirot [:ochameau] from comment #17)
> > Tried to see if DAMP reports any win, but no.
> > https://treeherder.mozilla.org/perf.html#/
> > compare?originalProject=try&originalRevision=f637870222111ca8e90da97c9d38df11
> > 0c0d902b&newProject=try&newRevision=7b7ca350b63d1688ab4780090a5f69dd3ad2d4b7&
> > framework=1&showOnlyImportant=0
> > 
> > This is sad as this patch clearly fixes a perf issues.
> 
> First of all, keep the fixes coming!  If they aren't showing up on DAMP then
> we should fix DAMP.  
> In local testing were you seeing an improvement of
> toolbox startup time with these changes?

Individual fixes are rarely big enough to be visible at the eye sight,
but optimization is made of tens of fixes like this. Only then it should become visible.
The only places where I can see a concrete improvement is on cleopatra profiles.
Damp, nor local testing shows any obvious win.
I tried to probe showToolbox when testing locally by opening the inspector manually.
The results are also very unstable. Even after tons of runs, the various kind of meatimes are always fuzzy.
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #19)
> (In reply to Brian Grinstead [:bgrins] from comment #18)
> > (In reply to Alexandre Poirot [:ochameau] from comment #17)
> > > Tried to see if DAMP reports any win, but no.
> > > https://treeherder.mozilla.org/perf.html#/
> > > compare?originalProject=try&originalRevision=f637870222111ca8e90da97c9d38df11
> > > 0c0d902b&newProject=try&newRevision=7b7ca350b63d1688ab4780090a5f69dd3ad2d4b7&
> > > framework=1&showOnlyImportant=0
> > > 
> > > This is sad as this patch clearly fixes a perf issues.
> > 
> > First of all, keep the fixes coming!  If they aren't showing up on DAMP then
> > we should fix DAMP.  
> > In local testing were you seeing an improvement of
> > toolbox startup time with these changes?
> 
> Individual fixes are rarely big enough to be visible at the eye sight,
> but optimization is made of tens of fixes like this. Only then it should
> become visible.
> The only places where I can see a concrete improvement is on cleopatra
> profiles.
> Damp, nor local testing shows any obvious win.
> I tried to probe showToolbox when testing locally by opening the inspector
> manually.
> The results are also very unstable. Even after tons of runs, the various
> kind of meatimes are always fuzzy.

Alright thanks, I wanted to make sure it wasn't missing an obvious local improvement.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.