1420289 - netmonitor can lazy load various components modules

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

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);

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1420289 - Lazy load optional React components from netmonitor.

Review commit: https://reviewboard.mozilla.org/r/208082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208082/

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 8937410 [details]
Bug 1420289 - Lazy load optional React components from netmonitor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208082/diff/1-2/

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8937410 [details]
Bug 1420289 - Lazy load optional React components from netmonitor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208082/diff/2-3/

Comment 4

[Alexandre Poirot [:ochameau]]

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 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8937410 [details]
Bug 1420289 - Lazy load optional React components from netmonitor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208082/diff/3-4/

Comment 7

[Alexandre Poirot [:ochameau]]

(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.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 9

[Alexandre Poirot [:ochameau]]

Updated DAMP results, with latest patch:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=a17d7206658b7e4d08c54e50a57e4b533eed6168&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmon&framework=1&selectedTimeRange=172800
  complicated.open -10%
  complicated.reload +3.6%
  simple.open -12%
  simple.requestsFinished +7%

It looks like reps doesn't make a big difference afterall, surprising.

Green try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a17d7206658b7e4d08c54e50a57e4b533eed6168

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3dd0966303ca
Lazy load optional React components from netmonitor. r=Honza

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3dd0966303ca

Comment 12

[Ricky Chien [:rickychien] (inactive)]

sign.... This patch entirely break launchpad (loader is not defined). We should be aware of any patch which could break launchpad workflow before landing.

Comment 13

[Ricky Chien [:rickychien] (inactive)]

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

Comment 14

[Alexandre Poirot [:ochameau]]

(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) {
  });