Closed Bug 1396619 Opened 7 years ago Closed 7 years ago

Lazy load inspector related actors

Categories

(DevTools :: General, enhancement)

enhancement
Not set
normal

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
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).
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.
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
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...
Attachment #8904463 - Flags: review?(pbrosset) → review?(zer0)
Looks good to me, but Matteo was the one working on highlighters registration the most, so I'd rather have him review this change.
(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 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 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+
(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.)
(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.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3248148e4d09
Lazy load actor codebase related to the inspector. r=zer0
(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.
https://hg.mozilla.org/mozilla-central/rev/3248148e4d09
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.