1419350 - Stop doing React updates while netmonitor is in background

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P2)

Description

[Alexandre Poirot [:ochameau]]

Similarely to bug 1399242, we could use VisibilityHandler React Element to prevent doing unecessary React updates while the panel is in background:
  https://hg.mozilla.org/integration/autoland/rev/3742c29706eb

Also, the new DAMP test specific to panels being in background should be tweaked to also cover the netmonitor:
  https://hg.mozilla.org/integration/autoland/rev/7eae6691b2f9

Comment 1

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

Attachment: Bug 1419350 - Stop doing React updates while netmonitor is in background;

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

Comment 2

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

@Alex: this bug is a first step towards Bug 1436665 (onRequestFinished event should be sent even if the Netmonitor UI isn't initialized)

I recall this comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=1311171#c55

As soon as the Net panel UI isn't updated when it's running in the background, what would be blocking us from loading the panel automatically when an extension appends `onRequestFinished` listener?

Honza

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8952392 [details]
Bug 1419350 - Stop doing React updates while netmonitor is in background;

https://reviewboard.mozilla.org/r/221642/#review227526

Great, thanks a lot for looking into that!

Looks good, but you would also need to tweak the dedicated DAMP test.

See these two docs about DAMP:
http://docs.firefox-dev.tools/tests/performance-tests.html
http://docs.firefox-dev.tools/tests/writing-perf-tests.html
But to keep it short, the command to run this test is:
  ./mach talos-test --activeTests damp --cycles 1 --tppagecycles 1 --subtest panelsInBackground

The test is here:
https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#716-735
You should call:
  await toolbox.selectTool("netmonitor");
right after:
  let toolbox = await this.openToolbox("webconsole");
So that we also open the netmonitor in this test.

Then, add something in the page script here:
  https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#718-723
It should stress up the netmonitor, like doing 2000 xhr for example.
Tweak 2000, in order to increase test duration around 2x. So that both tools are on parity.
Netmonitor is still going to compute things, redux work should still occur while react won't.
At least that was happens for the console.

::: devtools/client/netmonitor/src/utils/redux-connect.js:22
(Diff revision 1)
> +  }
> +}
> +
> +module.exports = {
> +  connect: connectWrapper
> +}

Wait. Does that mean we have to hack into all connect() calls? Isn't it enough to have it on the root component?

If that's the case, it looks like the webconsole misses such interception in these two places:
https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/components/FilterBar.js#298
https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/components/SideBar.js#86

( We only add VisibilityHandler here:
  https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/components/ConsoleOutput.js#198 )

If that's confirmed for the webconsole, then may be we should move this next to/into VisibilityHandler module as we would most likely share that between netmon and console!

::: devtools/client/netmonitor/test/browser_net_background_update.js:33
(Diff revision 1)
> +  info("Wait for Net panel to be hidden");
> +  await waitUntil(() => (document.visibilityState == "hidden"));
> +
> +  // Execute another two requests
> +  await performRequests(monitor, tab);
> +

It would be great to also verify in this test that we do not render while hidden.
May be asserting that:
document.querySelectorAll(".request-list-item").length == 2
here, would be enough?

::: devtools/client/netmonitor/test/browser_net_background_update.js:49
(Diff revision 1)
> +  let wait = waitForNetworkEvents(monitor, 2);
> +  await ContentTask.spawn(tab.linkedBrowser, {}, () => {
> +    content.wrappedJSObject.performRequests(2);
> +  });
> +  await wait;
> +}

It would be great to have this helper moved to head.js and use it everywhere we do this.

Note that spawn allows you to pass arguments to the content scripts, via its second argument, like this:
  let wait = waitForNetworkEvents(monitor, 2);
  await ContentTask.spawn(tab.linkedBrowser, 2, requestCount => {
    content.wrappedJSObject.performRequests(requestCount);
  });

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #2)
> @Alex: this bug is a first step towards Bug 1436665 (onRequestFinished event
> should be sent even if the Netmonitor UI isn't initialized)
> 
> I recall this comment:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1311171#c55
> 
> As soon as the Net panel UI isn't updated when it's running in the
> background, what would be blocking us from loading the panel automatically
> when an extension appends `onRequestFinished` listener?

We would still have redux/reducer work, which isn't always free of complex computation.
Nicolas did some work to optimize it and it had a significant impact on panelsInBackground test,
which proves that panels, can still have a significant impact on performance, even with the VisibilityHandler trick.

Ideally, regarding bug 1311171 comment 55, we would come up with an independant helper, managing all RDP client calls that can be used by HAR, WebExt and netmonitor independantly. FirefoxDataProvider could be this independant helper. Couldn't it?

Comment 5

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

Attachment: Bug 1419350 - Update tests;

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

Comment 6

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

Comment on attachment 8952392 [details]
Bug 1419350 - Stop doing React updates while netmonitor is in background;

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

Comment 7

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

(In reply to Alexandre Poirot [:ochameau] from comment #3)
> It should stress up the netmonitor, like doing 2000 xhr for example.
> Tweak 2000, in order to increase test duration around 2x. So that both tools
> are on parity.
> Netmonitor is still going to compute things, redux work should still occur
> while react won't.
> At least that was happens for the console.
I attached a new path that is trying to do it.

> Wait. Does that mean we have to hack into all connect() calls? Isn't it
> enough to have it on the root component?
It isn't enough to apply it only on the root, there are more connect() calls
in the component hierarchy.
 
> If that's the case, it looks like the webconsole misses such interception in
> these two places:
> https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-
> console-output/components/FilterBar.js#298
> https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-
> console-output/components/SideBar.js#86
> 
> ( We only add VisibilityHandler here:
>  
> https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-
> console-output/components/ConsoleOutput.js#198 )
Yes, it's enough for the Console


> If that's confirmed for the webconsole, then may be we should move this next
> to/into VisibilityHandler module as we would most likely share that between
> netmon and console!
@nchevobbe I am looking at how the Console is using VisibilityHelper. I am using it for the Net panel too (see the attached patch), but applying the component only on the root isn't enough since there is more places in the component hierarchy where connect() - to redux store - is used.

So, I implemented new connect method that wraps the current component with VisibilityHelper automatically. Perhaps we could share this with the Console panel?

One question, as Alex pointed out, there are some connect() calls in the Console that are not using the VisibilityHelper wrapper component. I understand that it isn't necessary since those comps are not updated when a new log appears. But, in net panel I used it for *every* connect call to make sure that not update happens if the panel is in background - even if the mapsStateToProps changes in the future. How does that sound to you?

> It would be great to also verify in this test that we do not render while
> hidden.
> May be asserting that:
> document.querySelectorAll(".request-list-item").length == 2
> here, would be enough?
Done

> ::: devtools/client/netmonitor/test/browser_net_background_update.js:49
> (Diff revision 1)
> > +  let wait = waitForNetworkEvents(monitor, 2);
> > +  await ContentTask.spawn(tab.linkedBrowser, {}, () => {
> > +    content.wrappedJSObject.performRequests(2);
> > +  });
> > +  await wait;
> > +}
> 
> It would be great to have this helper moved to head.js and use it everywhere
> we do this.
Good point, I created a bug 1439888 to cover this.

> Note that spawn allows you to pass arguments to the content scripts, via its
> second argument, like this:
>   let wait = waitForNetworkEvents(monitor, 2);
>   await ContentTask.spawn(tab.linkedBrowser, 2, requestCount => {
>     content.wrappedJSObject.performRequests(requestCount);
>   });
Nice, done


Honza

Comment 8

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

@Jason: See the attached patch that implements custom `connect` method (wrapping Redux's connect) and using VisibilityHelper for every store-connected component - effectively blocking any changes in the store that could cause updates (re-rendering). 
How does that sound to you?
Honza

Comment 9

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

Comment on attachment 8952392 [details]
Bug 1419350 - Stop doing React updates while netmonitor is in background;

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

Comment 10

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

Comment on attachment 8952694 [details]
Bug 1419350 - Update tests;

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

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

> @nchevobbe I am looking at how the Console is using VisibilityHelper. I am using it for the Net panel too (see the attached patch), but applying the component only on the root isn't enough since there is more places in the component hierarchy where connect() - to redux store - is used.
>So, I implemented new connect method that wraps the current component with VisibilityHelper automatically. Perhaps we could share this with the Console panel?
>One question, as Alex pointed out, there are some connect() calls in the Console that are not using the VisibilityHelper wrapper component. I understand that it isn't necessary since those comps are not updated when a new log appears. But, in net panel I used it for *every* connect call to make sure that not update happens if the panel is in background - even if the mapsStateToProps changes in the future. How does that sound to you?

Looks good to me. As discussed yesterday, we should only make sure this does not add too much overhead (a DAMP push with the custom connect function applied to netmonitor should tell us if it is). But if it is harmless, I'd gladly use it in the console.

Comment 12

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

Comment on attachment 8952694 [details]
Bug 1419350 - Update tests;

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

Comment 13

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

Comment on attachment 8952694 [details]
Bug 1419350 - Update tests;

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

Comment 14

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

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> Ideally, regarding bug 1311171 comment 55, we would come up with an
> independant helper, managing all RDP client calls that can be used by HAR,
> WebExt and netmonitor independantly. FirefoxDataProvider could be this
> independant helper. Couldn't it?
The thing is that requests reducer is collecting all the fetched data
from the backend. If we'd want to avoid/bypass the reducer we need
to duplicate the logic.
HAR, WebExt and Net monitor they all need the data with expected structure
and this data are in the reducer.
I'd like to first see how big impact the Net monitor reducer really
has and if we can optimize it. We are not using immutablejs anymore
and there are no difficult calculations.

Honza

Comment 15

[Alexandre Poirot [:ochameau]]

Comment on attachment 8952694 [details]
Bug 1419350 - Update tests;

https://reviewboard.mozilla.org/r/221922/#review228348

I do not reproduce the error locally, but the test fails on try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fa01cfbf96c22c277feb069731c27361c9d0b4f

You can push to try like this:
  ./mach try  -b o -p linux64 -u none -t g2-e10s --artifact
  More doc available here:
  http://docs.firefox-dev.tools/tests/performance-tests.html

::: testing/talos/talos/tests/devtools/addon/content/damp.js:733
(Diff revision 4)
>      await toolbox.selectTool("options");
>  
> +    // Reload the page and wait for all HTTP requests
> +    // to finish (1 doc + 800 XHRs).
>      await this.reloadPageAndLog("panelsInBackground", toolbox);
> +    await this.waitForPayload(801, monitor.panelWin);

I'm wondering if you could have a race here, because we start listening *after* we trigger the action that dispatches payload events.
Can we call `this.waitForPayload` before calling `reloadPageandLog`?

::: testing/talos/talos/tests/devtools/addon/content/damp.js:750
(Diff revision 4)
> +        payloadReady++;
> +        maybeResolve();
> +      }
> +
> +      function maybeResolve() {
> +        if (payloadReady === count) {

Otherwise, I'm wondering if that's the same issue than bug 1439991, may be you need to checkout for payloadReady >= count?

To debug that, you can put dump() statements here and push to try to see what really happens on try.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8952694 [details]
Bug 1419350 - Update tests;

https://reviewboard.mozilla.org/r/221922/#review228350

Comment 17

[Alexandre Poirot [:ochameau]]

Comment on attachment 8952392 [details]
Bug 1419350 - Stop doing React updates while netmonitor is in background;

https://reviewboard.mozilla.org/r/221642/#review228262

Looking at the profiler, I'm not sure this patch changes anything to the netmonitor while it is in background.

I did this str:
* open this url: `data:text/html,<script>window.onclick=()=>{var x=new XMLHttpRequest(); x.open("GET", "http://mozilla.org", false); x.send(null);}</script>`
* open the netmonitor
* switch to options panel
* start recording in perf-html
* click on the page to dispatch XHRs
* stop recording and look at the profile

Here is profiles without and with your patch, and there is still some renders being done:
* without patch:
https://perfht.ml/2om2jQa
* with patch:
https://perfht.ml/2Cdacku

We see redux stacks in both, and both are calling React.setState.
You can see RequestListItem and/or RequestListContent's render method being called.
A good filter, in the call tree view is "src/components", it helps seeing the netmonitor React components.

::: devtools/client/netmonitor/src/moz.build:18
(Diff revision 3)
>      'utils',
>      'widgets',
>  ]
>  
>  DevToolsModules(
> +    'connect.js',

This connect.js file isn't used.
Is that expected?

::: devtools/client/netmonitor/src/utils/redux-connect.js:6
(Diff revision 3)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +

A small comment about the purpose of this module would be helpful here.

Comment 18

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

Comment on attachment 8952392 [details]
Bug 1419350 - Stop doing React updates while netmonitor is in background;

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

Comment 19

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

(In reply to Alexandre Poirot [:ochameau] from comment #17)
> You can see RequestListItem and/or RequestListContent's render method being
> called.
> A good filter, in the call tree view is "src/components", it helps seeing
> the netmonitor React components.
This is weird.

Here is my profile, following your STRs from comment 17,
I don't see any render methods called.
https://perfht.ml/2CE2MSV

Also, I don't see any console.log()s if I put them into the render methods.

> ::: devtools/client/netmonitor/src/moz.build:18
> (Diff revision 3)
> >      'utils',
> >      'widgets',
> >  ]
> >  
> >  DevToolsModules(
> > +    'connect.js',
> 
> This connect.js file isn't used.
> Is that expected?
Ah, good catch, removed


> ::: devtools/client/netmonitor/src/utils/redux-connect.js:6
> (Diff revision 3)
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +"use strict";
> > +
> 
> A small comment about the purpose of this module would be helpful here.
Done

Honza

Comment 20

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

Comment on attachment 8952392 [details]
Bug 1419350 - Stop doing React updates while netmonitor is in background;

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

Comment 21

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

Attachment: Bug 1419350 - Update tests;

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

Comment 22

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

Comment on attachment 8952392 [details]
Bug 1419350 - Stop doing React updates while netmonitor is in background;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221642/diff/5-6/

Comment 23

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

(In reply to Alexandre Poirot [:ochameau] from comment #15)
> ::: testing/talos/talos/tests/devtools/addon/content/damp.js:733
> (Diff revision 4)
> >      await toolbox.selectTool("options");
> >  
> > +    // Reload the page and wait for all HTTP requests
> > +    // to finish (1 doc + 800 XHRs).
> >      await this.reloadPageAndLog("panelsInBackground", toolbox);
> > +    await this.waitForPayload(801, monitor.panelWin);
> 
> I'm wondering if you could have a race here, because we start listening
> *after* we trigger the action that dispatches payload events.
> Can we call `this.waitForPayload` before calling `reloadPageandLog`?
Fixed

> ::: testing/talos/talos/tests/devtools/addon/content/damp.js:750
> (Diff revision 4)
> > +        payloadReady++;
> > +        maybeResolve();
> > +      }
> > +
> > +      function maybeResolve() {
> > +        if (payloadReady === count) {
> 
> Otherwise, I'm wondering if that's the same issue than bug 1439991, may be
> you need to checkout for payloadReady >= count?
Done

> You can push to try like this:
>   ./mach try  -b o -p linux64 -u none -t g2-e10s --artifact
>   More doc available here:
>   http://docs.firefox-dev.tools/tests/performance-tests.html

I fixed the possible race and the condition, but try still fails
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e67a9984c6d298a1fb8473e4cdafe1c862f13dc

This is puzzling me, perhaps the test finished too soon?

Honza

Comment 24

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #19)
> Here is my profile, following your STRs from comment 17,
> I don't see any render methods called.
> https://perfht.ml/2CE2MSV

Oh. That may be because your computer is significantly faster than mine?
Most render method are pretty naive and don't do much, in that case, they are not shown in the profiler.

You can try to set a lower interval in perf-html add-on:
http://docs.firefox-dev.tools/contributing/performance/profiler-buffer-size.png
Here, tweak "interval" to the left, to a faster interval. Between 0.1 and 1ms.

> Also, I don't see any console.log()s if I put them into the render methods.

I do see dumps if I add a `dump("RENDER\n");` into:
  devtools/client/netmonitor/src/components/RequestListContent.js's render method

I imagine it would be easier to debug this first via log rather than the profiler...

Comment 25

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> (In reply to Alexandre Poirot [:ochameau] from comment #15)
> > ::: testing/talos/talos/tests/devtools/addon/content/damp.js:733
> > (Diff revision 4)
> > >      await toolbox.selectTool("options");
> > >  
> > > +    // Reload the page and wait for all HTTP requests
> > > +    // to finish (1 doc + 800 XHRs).
> > >      await this.reloadPageAndLog("panelsInBackground", toolbox);
> > > +    await this.waitForPayload(801, monitor.panelWin);
> > 
> > I'm wondering if you could have a race here, because we start listening
> > *after* we trigger the action that dispatches payload events.
> > Can we call `this.waitForPayload` before calling `reloadPageandLog`?
> Fixed
> 
> > ::: testing/talos/talos/tests/devtools/addon/content/damp.js:750
> > (Diff revision 4)
> > > +        payloadReady++;
> > > +        maybeResolve();
> > > +      }
> > > +
> > > +      function maybeResolve() {
> > > +        if (payloadReady === count) {
> > 
> > Otherwise, I'm wondering if that's the same issue than bug 1439991, may be
> > you need to checkout for payloadReady >= count?
> Done
> 
> > You can push to try like this:
> >   ./mach try  -b o -p linux64 -u none -t g2-e10s --artifact
> >   More doc available here:
> >   http://docs.firefox-dev.tools/tests/performance-tests.html
> 
> I fixed the possible race and the condition, but try still fails
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9e67a9984c6d298a1fb8473e4cdafe1c862f13dc


When you see such output:
08:22:46     INFO -  Timeout waiting for test completion; killing browser...
08:22:46     INFO -  Terminating psutil.Process(pid=28864, name='firefox', started='07:22:45')
08:22:46     INFO -  PID 28864 | ExceptionHandler::GenerateDump cloned child 32106
08:22:46     INFO -  PID 28864 | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
08:22:46     INFO -  PID 28864 | ExceptionHandler::WaitForContinueSignal waiting for continue signal...
08:22:46     INFO -  mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/f8JVDLvRRLyEkzZzTW8GMA/artifacts/public/build/target.crashreporter-symbols.zip
08:22:55     INFO -  mozcrash Copy/paste: /home/cltbld/workspace/build/linux64-minidump_stackwalk /tmp/tmp1F1At7/profile/minidumps/11a9eabf-4a16-b3f2-916e-9e2b337e745e.dmp /tmp/tmpJMdb5m
08:22:55     INFO -  mozcrash Saved minidump as /home/cltbld/workspace/build/blobber_upload_dir/11a9eabf-4a16-b3f2-916e-9e2b337e745e.dmp
08:22:55     INFO -  PROCESS-CRASH | damp | application crashed [unknown top frame]

The important line is the first one, not the last.
For some reason, Talos crashes in case of timeout (I have no idea why, it seems to be on purpose).
So here it only timed out.

Also, it becomes clear that this is a timeout when you see these two lines:
07:23:51     INFO -  PID 26712 | Garbage collect
08:22:45     INFO -  Timeout waiting for test completion; killing browser...

Look at the time between the two, about an hour without doing anything.

I'm wondering if DAMP fails just because the patch doesn't really prevent the render/reflows and then makes the test very slow to run, use a lot of memory and slow down all DAMP tests.
So I would suggest looking first at why render methods are still called before looking back at DAMP.

Comment 26

[Alexandre Poirot [:ochameau]]

Comment on attachment 8952392 [details]
Bug 1419350 - Stop doing React updates while netmonitor is in background;

https://reviewboard.mozilla.org/r/221642/#review229586

Sorry about the previous report but I must have had something wrong in my setup.
The patch has a correct behavior now. And profiler confirms this.

A profile with current tip:
  https://perfht.ml/2FBtYEO

And a profile with your patch:
  https://perfht.ml/2F0F2dx

We go from 730ms down to 221ms of computation while doing 5 requests.

But out of these 221ms, there is still 140ms spent within react.js.
(The vast majority of react.js is delegated to react-dom.js:enqueueSetState [138ms])
By looking at the stacks, it looks like it is only to update the VisibilityHandler component.
We can see its shouldComponentUpdate method being called.

Then comes reselect and all the filter function, we spend 34ms in them.

So this patch is a good step forward, but it would be great to at least try to get rid of react overhead.
Can we somehow prevent redux from updating any react component state?
I was wondering if we could avoid using VisibilityHandler react component by hacking something only into redux pipeline?

And regarding comment 14 and bug 1311171 comment 55, these two things (react and reselect work) both look unnecessary for the webextension/har need. What do we miss in the redux work that isn't done by FirefoxDataProvider?
(may be we could open a dedicated bug to discuss about that?)

Comment 27

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

Comment on attachment 8952392 [details]
Bug 1419350 - Stop doing React updates while netmonitor is in background;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221642/diff/6-7/

Comment 28

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

Comment on attachment 8953983 [details]
Bug 1419350 - Update tests;

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

Comment 29

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

Comment on attachment 8953983 [details]
Bug 1419350 - Update tests;

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

Comment 30

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

Test updated, try push looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a684e748f02e2b170a0b40c4859b2928d18901e

Honza

Comment 31

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

(In reply to Alexandre Poirot [:ochameau] from comment #26)
> We go from 730ms down to 221ms of computation while doing 5 requests.
> 
> But out of these 221ms, there is still 140ms spent within react.js.
> (The vast majority of react.js is delegated to react-dom.js:enqueueSetState
> [138ms])
> By looking at the stacks, it looks like it is only to update the
> VisibilityHandler component.
> We can see its shouldComponentUpdate method being called.
> 
> Then comes reselect and all the filter function, we spend 34ms in them.
> 
> So this patch is a good step forward, but it would be great to at least try
> to get rid of react overhead.
> Can we somehow prevent redux from updating any react component state?
> I was wondering if we could avoid using VisibilityHandler react component by
> hacking something only into redux pipeline?
I reported bug 1441785 for this.

> And regarding comment 14 and bug 1311171 comment 55, these two things (react
> and reselect work) both look unnecessary for the webextension/har need. What
> do we miss in the redux work that isn't done by FirefoxDataProvider?
> (may be we could open a dedicated bug to discuss about that?)

1) FirefoxDataProvider is responsible for fetching data from the backend and
properly waiting for responses. As soon as new data are available an action
is fired to update `requests` reducer.

2) Redux `requests` reducer is responsible for collecting all the data coming
from the backend. The reducer's state is updated when 'ADD_REQUEST' or
`UPDATe_REQUEST` are fired (by FirefoxDataProvider).

So, these two pieces are working together, It isn't like one could replace
another. It would be great if we could dynamically disconnect components
from the store for the time when the panel is running in the background
and connect it again when the panel is selected.


I think we can discuss this in bug 1441785

Honza

Comment 32

[Alexandre Poirot [:ochameau]]

Comment on attachment 8953983 [details]
Bug 1419350 - Update tests;

https://reviewboard.mozilla.org/r/223136/#review229874

Thanks Honza, it looks good now.

I pushed to try a patch that lands the DAMP test without the netmonitor one:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=914cd9883224cd18cfdcfbfba270c31f06662575&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1&selectedTimeRange=172800
On it confirms that the DAMP test will report a significant regression if we break the render disabling!

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/panels-in-background.html:1
(Diff revision 3)
> +<!DOCTYPE html>

Could you move this file and the sjs file into a panels-in-background folder?
Julian recently landed a patch where each custom page is in its own folder.
(you would most likely have to rebase on top of this patch as I'm expecting it to conflict in damp.js's header)

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/panels-in-background.html:27
(Diff revision 3)
> +      xhr.send(null);
> +    }
> +  }
> +
> +  peformLogs(2000);
> +  performRequests(800);

I've been trying to find what is the best value here.
It would be great to have the same impact between the console and netmonitor. So that the test duration double.
I tried 400:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=39cf6f666e3563f2dada60326c3eb6f25d606948&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1&selectedTimeRange=172800
But it makes the test go from 337ms to 458ms

And 600:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=3bb4f9ea52a0b22faa3c735c766c56cd73c5af0e&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1&selectedTimeRange=172800
Which makes it double from 336ms to 670ms

Whereas 800:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=3f7ad3a2906cfafc05bf9f0069397526b539234d&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1&selectedTimeRange=172800
Almost triples it from 337ms to 912ms

So, could you use 600 in your final patch?

Comment 33

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

Comment on attachment 8952392 [details]
Bug 1419350 - Stop doing React updates while netmonitor is in background;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221642/diff/7-8/

Comment 34

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

Comment on attachment 8953983 [details]
Bug 1419350 - Update tests;

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

Comment 35

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

(In reply to Alexandre Poirot [:ochameau] from comment #32)
> Could you move this file and the sjs file into a panels-in-background folder?
Done

> > +  peformLogs(2000);
> > +  performRequests(800);
> So, could you use 600 in your final patch?
Done

Thanks Alex for the review!
Honza

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0d6b6657a6e
Stop doing React updates while netmonitor is in background; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/0fc7db6c6c56
Update tests; r=ochameau

Comment 37

[Narcis Beleuzu [:NarcisB]]

Backed out for devtools failures on browser_webconsole_network_messages_expand.js

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=165074596&repo=autoland&lineNumber=11855

Comment 38

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f17721e6da6
Backed out 2 changesets for devtools failures on browser_webconsole_network_messages_expand.js. CLOSED TREE

Comment 39

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

Comment on attachment 8952392 [details]
Bug 1419350 - Stop doing React updates while netmonitor is in background;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221642/diff/8-9/

Comment 40

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

Comment on attachment 8953983 [details]
Bug 1419350 - Update tests;

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

Comment 41

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

Just for the record:

The directory where 'redux-connect.js' file is located [1] must be in the list of browser based directories [2] so, |window| global is available in the file and also in loaded VisibilityHandler file. VisibilityHandler component needs |window| to add "visibilitychange" listener.

Honza

[1] resource://devtools/client/netmonitor/src/utils
[2] https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/devtools/client/shared/browser-loader.js#12

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5574f24919b
Stop doing React updates while netmonitor is in background; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/91c3a7795bb0
Update tests; r=ochameau

Comment 43

[Jason Laster [:jlast]]

Thanks. This is nice. I especially like the mochitest.

Comment 44

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/a5574f24919b
https://hg.mozilla.org/mozilla-central/rev/91c3a7795bb0