Closed
Bug 1396619
Opened 7 years ago
Closed 7 years ago
Lazy load inspector related actors
Categories
(DevTools :: General, enhancement)
DevTools
General
Tracking
(firefox57 fixed)
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
When opening the inspector for the first time, we load a lot of actors, which may not be all necessary. Loading devtools/server/actors/inspector.js (and all its dependencies) appears as the biggest chunk of JS computation in the child process: https://perfht.ml/2eyb6Kv
Comment 1•7 years ago
|
||
Looking at the profile, at least this one is lazy-loadable from styles.js: const {getDefinedGeometryProperties} = require("devtools/server/actors/highlighters/geometry-editor"); It's used inside an actor method that is only called when the box-model tab is visible (which is not the default).
Comment 2•7 years ago
|
||
There might be a way to lazy-load the reflow actor too. In fact I don't understand why it's being initialized in this profile. 2 panels use it: the box-model widget that is inside the computed-view and layout-view, and the grid inspector inside the layout-view. The computed and layout views are not visible by default. The rule-view is. And they should have code to avoid doing anything as long as they're not selected.
Assignee | ||
Comment 3•7 years ago
|
||
With this patch we go from: https://perfht.ml/2iYHeMb to: https://perfht.ml/2ezSZ6Z From 62ms to 25ms to load actors/inspector.js (and its deps). From 33% down to 10% of overall JS processing in the child process. With this patch, the profiler now mostly sees shared/specs/inspector.js and server/actors/string.js as costly dependencies of actors/inspector.js. Now, it doesn't mean we saved 33-10=23% of overall JS processing. Various dependencies are still loaded later by some other steps. Like actors/highlighters.js for example. But the main win of this patch is that we really no longer load *all* highlighters until each is really (rarely) used. So with this patch we only load highlighters/box-model and none of the others on my simple inspector load STR. My STR is: * open clean firefox (clean profile with just perf-html) * open data:text/html,foo * toggle the inspector We should continue looking into this and look dependency by dependency. geometry-editor is no longer loaded, but actors/reflows still is, by this: http://searchfox.org/mozilla-central/source/devtools/server/actors/inspector.js#900
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
DAMP results isn't obvious, but at least highlights some possible wins: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=07af3090520d8784564b4cc4ad8c1e2ee051ca55&newProject=try&newRevision=1bb06bf3c75979b8faa8bc26c8a639335715ca4f&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=inspector&framework=1 Otherwise speaking about the patch itself. In most cases I just translated `require` to `loader.lazyRequireGetter`. For actors/highlighters.js it isn't that simple. I introduced some lazy module loading into `register` and `CustomHighlighterActor.initialize`. Also I got rid of automatic export of all highlighters constructors from `highlighters` module as I think it is better to import them directly from each of their own modules...
Updated•7 years ago
|
Attachment #8904463 -
Flags: review?(pbrosset) → review?(zer0)
Comment 6•7 years ago
|
||
Looks good to me, but Matteo was the one working on highlighters registration the most, so I'd rather have him review this change.
Assignee | ||
Comment 7•7 years ago
|
||
Another DAMP run, using fixes to DAMP from bug 1394804: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=527341ed12c473fc7b83f4a37fb277ea321de9d3&newProject=try&newRevision=94783d31562766bec9dda8710aaea2b4c79babee&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1 This one reports a 5% win on inspector open for warn start and 3% on cold start.
Comment 8•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #7) > Another DAMP run, using fixes to DAMP from bug 1394804: > https://treeherder.mozilla.org/perf.html#/ > comparesubtest?originalProject=try&originalRevision=527341ed12c473fc7b83f4a37 > fb277ea321de9d3&newProject=try&newRevision=94783d31562766bec9dda8710aaea2b4c7 > 9babee&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignatur > e=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1 > This one reports a 5% win on inspector open for warn start and 3% on cold > start. That's a big win! The 'inspector open' measurement is really important since it's the entry point for the Inspect Element context menu.
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8904463 [details] Bug 1396619 - Lazy load actor codebase related to the inspector. https://reviewboard.mozilla.org/r/176312/#review181942 ::: devtools/server/actors/highlighters.js:724 (Diff revision 1) > +for (let symbol in Highlighters) { > + let module = Highlighters[symbol]; > + register(symbol, module); > +} > +/* > const { BoxModelHighlighter } = require("./highlighters/box-model"); Sorry was passing by the code review to see what I can learn, but I want to note this is something that we want to remove before landing.
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8904463 [details] Bug 1396619 - Lazy load actor codebase related to the inspector. https://reviewboard.mozilla.org/r/176312/#review182084 Looks good to me, thanks for working on this, Alex! Just address the comment from Gabe and the one about `Highlighters` object and / or the loop. ::: devtools/server/actors/highlighters.js:707 (Diff revision 1) > this._tabActor = null; > this._win = null; > } > }; > > +const Highlighters = { I don't see much value to have this descriptor, since it seems used just for registering. We could just call `register` directly, and remove the `for…in`. ```js register("BoxModelHighlighter", "box-model"); register("CssGridHighlighter", "css-grid"); // etc. ``` I was also thinking that we could just get rid of this mapping, and using only the module's name and change the highlighter's module to exports by default the highlighter itself; but it's definitely out of the scope of this bug. ::: devtools/server/actors/highlighters.js:719 (Diff revision 1) > + MeasuringToolHighlighter: "measuring-tool", > + EyeDropper: "eye-dropper", > + PausedDebuggerOverlay: "paused-debugger", > + ShapesHighlighter: "shapes", > +}; > +for (let symbol in Highlighters) { In case you've strong feeling about keep the `Highlighters` object, I would change the names here (`symbol` could be misleading, since the `Symbol` data type; and `module` is shadowing the global `module` variable), and have a `for…of`. Something like: ```js for (let [highlighterName, moduleName] of Object.entries(Highlighters)) { register(highlighterName, moduleName); } ``` or similar. ::: devtools/server/actors/inspector.js:82 (Diff revision 1) > +loader.lazyRequireGetter(this, "HighlighterActor", "devtools/server/actors/highlighters", true); > +loader.lazyRequireGetter(this, "CustomHighlighterActor", "devtools/server/actors/highlighters", true); > +loader.lazyRequireGetter(this, "isTypeRegistered", "devtools/server/actors/highlighters", true); > +loader.lazyRequireGetter(this, "HighlighterEnvironment", "devtools/server/actors/highlighters", true); > +loader.lazyRequireGetter(this, "EventParsers", "devtools/server/event-parsers", true); > +loader.lazyRequireGetter(this, "isAnonymous", "devtools/shared/layout/utils", true); I personally don't like the syntax of `lazyRequireGetter`, it makes everything harder to read at glance. We should at least have the capability to mimic the destructoring, something like: ```js loader.lazyRequireGetter(this, [ "isAnonymous", "isNativeAnonymous", "isXBLAnonymous", "isShadowAnonymous", "getFrameElement", ], "devtools/shared/layout/utils"); ``` But it's definitely out of the scope of this bug. It was just a thought that we could improve a bit our APIs. (I guess returns a `Proxy` would be much slower, otherwise it would be the optimum).
Attachment #8904463 -
Flags: review?(zer0) → review+
Comment 11•7 years ago
|
||
(Since in bugzilla is not clear, the last one was just a thought, is not an "issue" – in mozreview I didn't mark it as issue –; but I will add my thoughts about it in bug 1181967.)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #10) > I don't see much value to have this descriptor, since it seems used just for > registering. We could just call `register` directly, and remove the `for…in`. > ```js > register("BoxModelHighlighter", "box-model"); > register("CssGridHighlighter", "css-grid"); > // etc. > ``` > > I was also thinking that we could just get rid of this mapping, and using > only the module's name and change the highlighter's module to exports by > default the highlighter itself; but it's definitely out of the scope of this > bug. Yes, I haven't tried to address that as each highlighter is required independently from various places. This would required a dedicated patch modifying each call site. Do you think each highlighter module should export only one highlighter and do something like: module.exports = BoxModelHighlighter; ? Or did you had something else in mind? Because that will be hard to do that for geometry-editor and shapes which exports more than one symbol.
Comment 14•7 years ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3248148e4d09 Lazy load actor codebase related to the inspector. r=zer0
Comment 15•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #13) > > I was also thinking that we could just get rid of this mapping, and using > > only the module's name and change the highlighter's module to exports by > > default the highlighter itself; but it's definitely out of the scope of this > > bug. > > Yes, I haven't tried to address that as each highlighter is required > independently from various places. Yeah, it's definitely something we don't want to do here, but it would simplify our code. > This would required a dedicated patch modifying each call site. > Do you think each highlighter module should export only one highlighter and > do something like: > module.exports = BoxModelHighlighter; Yes, that's exactly my thought: before react / es6 module I was a bit against exports directly as this, but there are indeed advantage to have such "default". > Because that will be hard to do that for geometry-editor and shapes which > exports more than one symbol. It shouldn't be hard. The exports in shapes and geometry-editor could be easily moved either in another module, or be part of the highlighter itself if there is a strict coupling – but I don't think it's the case. But since it's not straight forward and involve a lot of changes, is indeed something we need to address separately, it's just that your refactoring made me thinking about it.
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3248148e4d09
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•