1411852 - All react components updated when new request is received

Status: NEW

(DevTools :: Netmonitor, defect, P3)

Description

[[e-mail]]

Running react devtool and spot all components updated when new request is received.

http://recordit.co/xbpnoZQVbN

Some of them are unnecessary update, ex:

* Toolbar should not get updated while add new request
* Existing requests should not get updated (except the timeline part) as well while add new request

Comment 1

[[e-mail]]

Note react-devtools described the highlight is not accurate before v16
https://github.com/facebook/react-devtools#does-highlight-updates-trace-renders

So this might not a valid issue, since todomvc demo looks like have similar issue http://todomvc.com/examples/react/#/

Comment 2

[Tim Nguyen :ntim]

Attachment: reduce-toolbar-updates.patch

This patch seems to reduce the number of updates on the Toolbar component for me.

Comment 3

[[e-mail]]

With above patch, the result looks good on DAMP

simple.netmonitor.reload.DAMP -4.98% (med)
simple.netmonitor.requestsFinished - 2.5%

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=e9a892d92d11e303d47d0060f7b6209a126ab090&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800

Comment 4

[[e-mail]]

Seems selector/getRequestFilterTypes is not cached by reselect, it could be moved to utils/ or called directly without extra function.

Comment 5

[[e-mail]]

If only add shouldComponentUpdate for the SearchBox, webconsole reload time is increased. 

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=48f7d0aace0d6776f95b2a6255205e30586406db&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=172800

I will remove the shouldComponentUpdate for the SearchBox part and test again

Comment 6

[[e-mail]]

also saw the `Warning: Toolbar.shouldComponentUpdate(): Returned undefined instead of a boolean value. Make sure to return true or false` in console

removed the line `this.refs.searchbox.focused;` fix the warning

Comment 7

[[e-mail]]

can use `this.refs.searchbox && this.refs.searchbox.focused`

Comment 8

[[e-mail]]

After patch toolbar without Searchbox shouldComponentUpdate, the result still looks good for netmonitor

simple.netmonitor.open 535.19 > 519.69 -2.90% (low) 
damp simple.netmonitor.reload 79.50 > 75.49 -5.05% (med)

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=d4b7a5b6fce8d3a8cfefe7da39a9f6be7b029173&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=172800

Comment 9

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

@Fred: what about this patch, should we review/land it?

Honza

Comment 10

[[e-mail]]

I suggest to modify this patch a bit then try to land it

1. remove Searchbox shouldComponentUpdate so this patch won't affect other modules perf
2. remove selector/getRequestFilterTypes & modify netmonitor/src/middleware/prefs.js as this patch

ntim, could you help update the patch?