Closed
Bug 1323466
Opened 8 years ago
Closed 8 years ago
Prevent loading uncessary modules by lazy loading them
Categories
(DevTools :: General, defect, P1)
DevTools
General
Tracking
(firefox53 fixed)
RESOLVED
FIXED
Firefox 53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
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 1•8 years ago
|
||
(in progress)
Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3acc9e49d34202800da84ce234df967995fa659b
Talos:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ffd0411ee2e4b356ea041543ec295b9dd7fb8dcb&newProject=try&newRevision=3acc9e49d34202800da84ce234df967995fa659b&framework=1&showOnlyImportant=0
Assignee | ||
Comment 2•8 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•8 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 | ||
Comment 9•8 years ago
|
||
Talos link for following patch: Bug 1323466 - Lazy load dependencies from actors/script.js. r=jryans
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=152cd91f4ee6cd4f374474153e2c4d09d33ebb63&newProject=try&newRevision=737cd764c9e9c6409ca72d22221e7915baa1a5eb&originalSignature=b97a8a540045b6139d2ce67b9c8b9a2f5c2918de&newSignature=b97a8a540045b6139d2ce67b9c8b9a2f5c2918de&framework=1
This is surprising talos isn't reporting a win as the profiler says
we spend 130ms to load script.js with the following numbers for each of its top dependency:
53ms - breakpoint
23ms - source
17ms - environment
4ms - frame
Link to profiler results:
https://clptr.io/2hRGu74
Talos link for my current patch queue:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=152cd91f4ee6cd4f374474153e2c4d09d33ebb63&newProject=try&newRevision=47804e01db84a6c6c4c84c85d604b46e4fdc83b8&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1
Which says 10% overall win, but subtests results are just crazy...
Assignee | ||
Updated•8 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•8 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•8 years ago
|
Attachment #8819933 -
Attachment is obsolete: true
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8819934 [details]
Bug 1323466 - Lazy load dependencies from actors/script.js.
For this patch we go from 74ms to load script.js in the child to 27ms:
https://new.cleopatra.io/public/e4b10f9c09d920dab6c0923adfec60a0457fcfbe/calltree/?jsOnly&search=script.js&thread=2
to
https://new.cleopatra.io/public/6da0e8f390be60484f096dd5114d77965417bf07/calltree/?jsOnly&search=script.js&thread=2
Assignee | ||
Comment 16•8 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•8 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 18•8 years ago
|
||
Comment on attachment 8819930 [details]
Bug 1323466 - Lazy load optional deps from toolbox.js.
We go from loading HUDService in 8ms from the parent:
https://new.cleopatra.io/public/e4b10f9c09d920dab6c0923adfec60a0457fcfbe/calltree/?jsOnly&search=hudservice&thread=0
To not loading it:
https://new.cleopatra.io/public/6da0e8f390be60484f096dd5114d77965417bf07/calltree/?jsOnly&search=hudservice&thread=0
And from 4ms for loading view-source in the parent:
https://new.cleopatra.io/public/e4b10f9c09d920dab6c0923adfec60a0457fcfbe/calltree/?jsOnly&search=view-source&thread=0
To nothing:
https://new.cleopatra.io/public/6da0e8f390be60484f096dd5114d77965417bf07/calltree/?jsOnly&search=view-source&thread=0
Assignee | ||
Comment 19•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 30•8 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•8 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•8 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•8 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
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•