Closed Bug 1323466 Opened 8 years ago Closed 7 years ago

Prevent loading uncessary modules by lazy loading them

Categories

(DevTools :: General, defect, P1)

Product:
DevTools
Component:
General
Type:
defect

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

Bug 1323466 - Lazy load optional deps from toolbox.js.
58 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1323466 - Split actors/worker.js in two to prevent loading unecessary actor code in parent process.
58 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1323466 - Prevent loading webbrowser.js in child process.
58 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1323466 - Lazy load dependencies from actors/script.js.
58 bytes, text/x-review-board-request
jryans
: review+
Details 
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.
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?
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
Summary: Prevent loading uncessary module in the parent process by lazy loading them → Prevent loading uncessary modules by lazy loading them
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
Attachment #8819933 - Attachment is obsolete: true
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
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.
Attachment #8819930 - Flags: review?(jryans)
Attachment #8819931 - Flags: review?(jryans)
Attachment #8819932 - Flags: review?(jryans)
Attachment #8819934 - Flags: review?(jryans)
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 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 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 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)
(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.
(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!
Depends on: 1324902
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 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+
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: