netmonitor can lazy load various components modules

RESOLVED FIXED in Firefox 59

Status

P2
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 59
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
Here is a profile of DAMP:
  https://perfht.ml/2zwbLsH

It highlights that MonitorPanel is force-loaded by App.js and it pulls many dependencies

https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/App.js#30-36
But this code loads either MonitorPanel or StatisticsPanel, so I imagine there is cases where it would be interesting to only load one of the two and not always the two.


While we are at lazy loading these dependencies, we should do the same with NetworkDetailsPanel from here:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/MonitorPanel.js#19
As it is used only under some conditions:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/MonitorPanel.js#107-112

Same for RequestListEmptyNotice / RequestListContent:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestList.js#12-13
conditionally used from:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestList.js#12-13

Same for all the columns components:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListItem.js#15-34
They are all conditionally used from:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListItem.js#140

Most of context-menu dependencies are used only when some context menu items are clicked, which is rare, so we should lazy load these dependencies:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/request-list-context-menu.js#8-12
(HarExporter is slow to load)

I didn't went through the whole netmonitor codebase, but that may be worth looking all all React component and see which ones are conditionally loaded and load them lazily.


In order to load a module lazily, you should use this helper:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/har/toolbox-overlay.js#9
loader.lazyRequireGetter(this, "HarAutomation", "devtools/client/netmonitor/src/har/har-automation", true);
(Assignee)

Updated

a year ago
Assignee: nobody → poirot.alex
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8937410 - Flags: review?(odvarko)
(Assignee)

Comment 4

a year ago
Here is a DAMP run:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=9bef6cb13c43bbce21d40ffaea595e082a4c28db&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmo&framework=1&selectedTimeRange=172800
It highlights a 9 to 13% win on netmonitor opening and a 7% regression on page reload.
We regress page reload as some modules are now loaded only when the first request is displayed.
So some modules are no longer loaded during "open" test, which displays an empty netmonitor.
But they are still loaded during "reload" test.
Comment on attachment 8937410 [details]
Bug 1420289 - Lazy load optional React components from netmonitor.

https://reviewboard.mozilla.org/r/208082/#review214418

Looks great, just couple of inline comments.
Thanks Alex!

Honza

::: devtools/client/netmonitor/src/components/PropertiesView.js:14
(Diff revision 3)
>  const { Component, createFactory } = require("devtools/client/shared/vendor/react");
>  const dom = require("devtools/client/shared/vendor/react-dom-factories");
>  const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
>  
>  const { REPS, MODE } = require("devtools/client/shared/components/reps/reps");
>  const { Rep } = REPS;

What about lazy-loading Reps too, there is a case in `renderValueWithRep` where Reps are not rendered (and there is significant amount of code/deps behind Reps module).

Similarly in HeadersPanel.js

::: devtools/client/netmonitor/src/components/RequestListItem.js:38
(Diff revision 3)
> -const RequestListColumnStartTime = createFactory(require("./RequestListColumnStartTime"));
> -const RequestListColumnStatus = createFactory(require("./RequestListColumnStatus"));
> -const RequestListColumnTransferredSize = createFactory(require("./RequestListColumnTransferredSize"));
> -const RequestListColumnType = createFactory(require("./RequestListColumnType"));
> -const RequestListColumnWaterfall = createFactory(require("./RequestListColumnWaterfall"));
> -
> +          RequestListColumnStartTime,
> +          RequestListColumnStatus,
> +          RequestListColumnTransferredSize,
> +          RequestListColumnType,
> +          RequestListColumnWaterfall */
> +const COLUMNS = [

nit: please insert an empty line after the list of globals and the indenatation should be just 2 spaces.

// Components
/* global
  RequestListColumnCause,
  RequestListColumnContentSize,
  RequestListColumnCookies,
  ...
*/

::: devtools/client/netmonitor/src/components/moz.build:27
(Diff revision 3)
>      'RequestListColumnEndTime.js',
>      'RequestListColumnFile.js',
>      'RequestListColumnLatency.js',
>      'RequestListColumnMethod.js',
>      'RequestListColumnProtocol.js',
> -    'RequestListColumnRemoteIp.js',
> +    'RequestListColumnRemoteIP.js',

Nice catch
Attachment #8937410 - Flags: review?(odvarko)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> ::: devtools/client/netmonitor/src/components/PropertiesView.js:14
> (Diff revision 3)
> >  const { Component, createFactory } = require("devtools/client/shared/vendor/react");
> >  const dom = require("devtools/client/shared/vendor/react-dom-factories");
> >  const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
> >  
> >  const { REPS, MODE } = require("devtools/client/shared/components/reps/reps");
> >  const { Rep } = REPS;
> 
> What about lazy-loading Reps too, there is a case in `renderValueWithRep`
> where Reps are not rendered (and there is significant amount of code/deps
> behind Reps module).
> 
> Similarly in HeadersPanel.js

Good catch!

Yes, it looks like reps is only used when we open some panels (headers and cookies at least) and not before.
Summary: devtools/client/netmonitor/src/components/App should load MonitorPanel and StatisticsPanel lazily → netmonitor can lazy load various components modules
Comment on attachment 8937410 [details]
Bug 1420289 - Lazy load optional React components from netmonitor.

https://reviewboard.mozilla.org/r/208082/#review214758

Looks great, thanks Alex!

R+ assuming try is green

Honza
Attachment #8937410 - Flags: review?(odvarko) → review+

Comment 10

a year ago
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3dd0966303ca
Lazy load optional React components from netmonitor. r=Honza

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3dd0966303ca
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
sign.... This patch entirely break launchpad (loader is not defined). We should be aware of any patch which could break launchpad workflow before landing.
Flags: needinfo?(poirot.alex)
Flags: needinfo?(odvarko)
Alex,

I've some concerns about the current lazyGetter approach here. We've been taking a lots of effort to clean up these internal gecko APIs and prevent using global variable like `loader.lazyGetter` in our codebase. Trying hard to adopt modern web coding pratice into the codebase just like using require anywhere.

Inspired by rewriting loader [1], do you think is that possible to do the opposite way to invoke `loader.lazyGetter` inside all require() calls which would try initializing resources if needed? So that we can stay commonJS import style. 

[1] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/webpack.config.js#39,41
(Assignee)

Comment 14

a year ago
(In reply to Ricky Chien [:rickychien] from comment #12)
> sign.... This patch entirely break launchpad (loader is not defined). We
> should be aware of any patch which could break launchpad workflow before
> landing.

For that you would need to create a taskcluster job that builds netmonitor with yarn and then run a test suite with netmonitor running in a tab.

(In reply to Ricky Chien [:rickychien] from comment #13)
> Alex,
> 
> I've some concerns about the current lazyGetter approach here. We've been
> taking a lots of effort to clean up these internal gecko APIs and prevent
> using global variable like `loader.lazyGetter` in our codebase. Trying hard
> to adopt modern web coding pratice into the codebase just like using require
> anywhere.

This isn't gecko specific. This is only specific to devtools codebase. Only DevTools use this API.

> Inspired by rewriting loader [1], do you think is that possible to do the
> opposite way to invoke `loader.lazyGetter` inside all require() calls which
> would try initializing resources if needed? So that we can stay commonJS
> import style. 

`var symbol = require(module);` just can't be made asynchronous.
There is some tricks involving proxies, but this is very error prone.

This is no standard way for lazy loading modules.
ES6 modules don't have any finalized solution, this is still under discussions:
  https://hospodarets.com/native-ecmascript-modules-dynamic-import
The only somewhat common way to do this is via requirejs, but that would still be different from nodejs require:  requirejs(['jquery', 'canvas', 'app/sub'],
  function   ($,        canvas,   sub) {
  });
Flags: needinfo?(poirot.alex)
Flags: needinfo?(odvarko)

Updated

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