All users were logged out of Bugzilla on October 13th, 2018

Prevent loading uncessary modules by lazy loading them

RESOLVED FIXED in Firefox 53

Status

P1
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Firefox 53
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Looking the profiler output, I saw that we are loading various modules in the parent for no real purpose. We do that just because they are loaded in some module headers. Instead we should lazy load them. In some cases lazy loading is not enough and we need to split some modules.

This bug is similar to bug 1321096, but I'll use that bug to tackle multiple spot at once.
(Assignee)

Comment 2

2 years ago
Here is some numbers about protocol.js:
   require(promise) = 0 -- parent
   require(devtools/shared/defer) = 0 -- parent
   require(sdk/core/heritage) = 0 -- parent
     require(../core/namespace) = 0 -- parent
    require(./core) = 6 -- parent
     require(./helpers) = 1 -- parent
    require(../lang/functional/core) = 5 -- parent
    require(../core/heritage) = 0 -- parent
   require(sdk/event/target) = 15 -- parent
   require(sdk/event/core) = 0 -- parent
   require(sdk/util/object) = 1 -- parent
   require(devtools/shared/platform/stack) = 0 -- parent
   require(devtools/shared/DevToolsUtils) = 0 -- parent
  require(devtools/shared/protocol) = 34 -- parent
It just dump the number of milliseconds that require() takes. So 34ms for protocol.js from parent,
which is about half spent in one of its dependency: sdk/event/target (15ms)

But I realized my current patches are doomed. We need protocol.js for the specs which are going to be used in the parent process anyway.
The number may look small, but it has a pretty deeper impact on performance, I imagine loading module blocks the thread or makes parallel operation slower as html-editor.js is reported to take 16ms whereas DAMP report 4 to 7% win on inpesctor.OPEN (bug 1323550).

We may tweak protocol.js by at least preventing loader EventTarget (the sdk one from the parent (it doesn't seem to be used anywhere else from parent). But given that Actor inherits from it, we can't easily lazify it. I think we could do something by splitting protocol.js in two with the common part and pull out the actor/backend part. Thoughts?
(Assignee)

Comment 3

2 years ago
Lazy loading view-source and hudservice isn't significant on DAMP, it is actually pretty small, but I think it is worth given they are really both used occasianally and are very easy to lazify.
You can easily spot them in this profile:
  https://clptr.io/2hwG20N
This is a profile of inspector opening on current master.
Search for "view-source" and "hudservice". you will see the result on graph and can confirm it is related to these module loading by expanding the call tree a lot.
Assignee: nobody → poirot.alex
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Summary: Prevent loading uncessary module in the parent process by lazy loading them → Prevent loading uncessary modules by lazy loading them
(Assignee)

Comment 10

2 years ago
I'm going to proceed with these patches even if DAMP doesn't report win we can see them on the profile.
Here is a profile without my patches:
https://new.cleopatra.io/public/e4b10f9c09d920dab6c0923adfec60a0457fcfbe/calltree/?jsOnly

And another one with all these patches:
https://new.cleopatra.io/public/6da0e8f390be60484f096dd5114d77965417bf07/calltree/?jsOnly
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8819933 - Attachment is obsolete: true
(Assignee)

Comment 16

2 years ago
Comment on attachment 8819932 [details]
Bug 1323466 - Prevent loading webbrowser.js in child process.

We go from loading it in the child in 4ms:
https://new.cleopatra.io/public/e4b10f9c09d920dab6c0923adfec60a0457fcfbe/calltree/?jsOnly&search=webbrowser.js&thread=2
To not loading it at all in the child:
https://new.cleopatra.io/public/6da0e8f390be60484f096dd5114d77965417bf07/calltree/?jsOnly&search=webbrowser.js&thread=2

You can see child.js overall impact being slightly lowered (139 to 132ms):
https://new.cleopatra.io/public/e4b10f9c09d920dab6c0923adfec60a0457fcfbe/calltree/?jsOnly&search=child.js&thread=2
https://new.cleopatra.io/public/6da0e8f390be60484f096dd5114d77965417bf07/calltree/?jsOnly&search=child.js&thread=2

Ok, this one isn't a big win, but I think it is salutary to cleanup these actor module registering codepaths.
(Assignee)

Comment 17

2 years ago
Comment on attachment 8819931 [details]
Bug 1323466 - Split actors/worker.js in two to prevent loading unecessary actor code in parent process.

We go from loading worker.js in parent during 12ms:
https://new.cleopatra.io/public/e4b10f9c09d920dab6c0923adfec60a0457fcfbe/calltree/?jsOnly&search=actors%2Fworker&thread=0
(do not hesitate to expand the tree a lot with Right key!)

To loading just worker-list.js in 1ms:
https://new.cleopatra.io/public/6da0e8f390be60484f096dd5114d77965417bf07/calltree/?jsOnly&search=actors%2Fworker&thread=0
(Assignee)

Comment 19

2 years ago
jryans, I'm going to flag you for review, but do not hesitate to forward and/or ask me to move some patch in their own bug.
(Assignee)

Updated

2 years ago
Attachment #8819930 - Flags: review?(jryans)
Attachment #8819931 - Flags: review?(jryans)
Attachment #8819932 - Flags: review?(jryans)
Attachment #8819934 - Flags: review?(jryans)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8819930 [details]
Bug 1323466 - Lazy load optional deps from toolbox.js.

https://reviewboard.mozilla.org/r/99534/#review100282

Thanks, seems straightforward!
Attachment #8819930 - Flags: review?(jryans) → review+

Comment 23

2 years ago
mozreview-review
Comment on attachment 8819931 [details]
Bug 1323466 - Split actors/worker.js in two to prevent loading unecessary actor code in parent process.

https://reviewboard.mozilla.org/r/99536/#review100284

Thanks, makes sense!
Attachment #8819931 - Flags: review?(jryans) → review+

Comment 24

2 years ago
mozreview-review
Comment on attachment 8819934 [details]
Bug 1323466 - Lazy load dependencies from actors/script.js.

https://reviewboard.mozilla.org/r/99542/#review100292

::: devtools/server/actors/script.js
(Diff revision 3)
>  
>  Object.assign(PauseScopedObjectActor.prototype.requestTypes, {
>    "threadGrip": PauseScopedObjectActor.prototype.onThreadGrip,
>  });
>  
> -function hackDebugger(Debugger) {

Is this not needed anymore?  Even if it's not, you may want to remove it in a separate commit or bug, it seems a bit separate from just making things lazy.
Attachment #8819934 - Flags: review?(jryans)

Comment 25

2 years ago
mozreview-review
Comment on attachment 8819932 [details]
Bug 1323466 - Prevent loading webbrowser.js in child process.

https://reviewboard.mozilla.org/r/99538/#review100288

Seems close, but a few things to revise first.

::: devtools/server/content-server.jsm:37
(Diff revision 3)
>  
>    if (!DebuggerServer.initialized) {
>      DebuggerServer.init();
>    }
> +  // For browser content toolbox, we do need a regular root actor and all tab
> +  // actors, but don't need all the "brower actors" that are only useful when

Nit: brower -> browser

::: devtools/server/main.js:249
(Diff revision 3)
>  
>    /**
> +   * Register all type of actors. Only register the one that are not already
> +   * registered.
> +   *
> +   * @param root boolean Registers the root actor from webbrowser module, which

Move the description text to the next line, so there's a common start point for the indentation, making it easier to read.  Some existing examples start the description at the same column as the param name, but choose your favorite as long as it's the same for all params.

::: devtools/server/main.js:277
(Diff revision 3)
> +      this.addTabActors();
> +    }
> +
> +    // Check for webbrowser and not just 'root' value as addBrowserActors or a
> +    // previous call to registerActors may have registered the root actor.
> +    this.rootLessServer = !this.isModuleRegistered("devtools/server/actors/webbrowser");

Seems like `rootlessServer` should be a getter instead.  I don't see a reason to cache it as a separate piece of data.

(Also, I suggest the spelling `rootlessServer` with lowercase l, which reads a bit more naturally to me.)

::: devtools/server/main.js:425
(Diff revision 3)
> +    if (!this.isModuleRegistered("devtools/server/actors/webbrowser")) {
> -    this.registerModule("devtools/server/actors/webbrowser");
> +      this.registerModule("devtools/server/actors/webbrowser");
> +    }
> +
> +    // Prevent loading the actors twice
> +    if (this.isModuleRegistered("devtools/server/actors/addons")) {

Instead of having to throw these guards everywhere, should we just remove the error from `registerModule` when it's called multiple times?  Hoping for your opinion here, I could be okay with either approach.
Attachment #8819932 - Flags: review?(jryans)
(Assignee)

Comment 26

2 years ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #24)
> Comment on attachment 8819934 [details]
> Bug 1323466 - Lazy load dependencies from actors/script.js.
>
> > -function hackDebugger(Debugger) {
> 
> Is this not needed anymore?  Even if it's not, you may want to remove it in
> a separate commit or bug, it seems a bit separate from just making things
> lazy.

I just assumed that at least one test would start failing, but none reported it from being missing.
The thing is that "Debugger" symbol isn't used in script.js. So I think it is safe removing that.
But yes, you are right, I'll do that in a dedicated bug.
(Assignee)

Comment 27

2 years ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #25)
> Comment on attachment 8819932 [details]
> Bug 1323466 - Prevent loading webbrowser.js in child process.
>
> ::: devtools/server/main.js:425
> (Diff revision 3)
> > +    if (!this.isModuleRegistered("devtools/server/actors/webbrowser")) {
> > -    this.registerModule("devtools/server/actors/webbrowser");
> > +      this.registerModule("devtools/server/actors/webbrowser");
> > +    }
> > +
> > +    // Prevent loading the actors twice
> > +    if (this.isModuleRegistered("devtools/server/actors/addons")) {
> 
> Instead of having to throw these guards everywhere, should we just remove
> the error from `registerModule` when it's called multiple times?  Hoping for
> your opinion here, I could be okay with either approach.

I hesitated a lot to do that, so I think this is a sign that we should just do that instead!
(Assignee)

Updated

2 years ago
Depends on: 1324902
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

2 years ago
mozreview-review
Comment on attachment 8819934 [details]
Bug 1323466 - Lazy load dependencies from actors/script.js.

https://reviewboard.mozilla.org/r/99542/#review100886

Thanks, looks reasonable!
Attachment #8819934 - Flags: review?(jryans) → review+

Comment 31

2 years ago
mozreview-review
Comment on attachment 8819932 [details]
Bug 1323466 - Prevent loading webbrowser.js in child process.

https://reviewboard.mozilla.org/r/99538/#review100890

Okay, looks good to me!  Thanks for making this change.  This comments about what we wanted to register in each path are especially helpful to have.
Attachment #8819932 - Flags: review?(jryans) → review+
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 40

2 years ago
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28fef8ed444b
Lazy load optional deps from toolbox.js. r=jryans
https://hg.mozilla.org/integration/autoland/rev/4d2cf5124fc3
Split actors/worker.js in two to prevent loading unecessary actor code in parent process. r=jryans
https://hg.mozilla.org/integration/autoland/rev/62dae2f53261
Prevent loading webbrowser.js in child process. r=jryans
https://hg.mozilla.org/integration/autoland/rev/0529846e3084
Lazy load dependencies from actors/script.js. r=jryans

Comment 41

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/28fef8ed444b
https://hg.mozilla.org/mozilla-central/rev/4d2cf5124fc3
https://hg.mozilla.org/mozilla-central/rev/62dae2f53261
https://hg.mozilla.org/mozilla-central/rev/0529846e3084
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.