Open
Bug 1411852
Opened 7 years ago
Updated 2 years ago
All react components updated when new request is received
Categories
(DevTools :: Netmonitor, defect, P3)
DevTools
Netmonitor
Tracking
(firefox57 fix-optional)
NEW
Tracking | Status | |
---|---|---|
firefox57 | --- | fix-optional |
People
(Reporter: gasolin, Unassigned)
References
Details
Attachments
(1 file)
3.88 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•7 years ago
|
||
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/#/
Updated•7 years ago
|
status-firefox57:
--- → fix-optional
Priority: -- → P3
Comment 2•7 years ago
|
||
This patch seems to reduce the number of updates on the Toolbar component for me.
Reporter | ||
Comment 3•7 years ago
|
||
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
Reporter | ||
Comment 4•7 years ago
|
||
Seems selector/getRequestFilterTypes is not cached by reselect, it could be moved to utils/ or called directly without extra function.
Reporter | ||
Comment 5•7 years ago
|
||
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
Reporter | ||
Comment 6•7 years ago
|
||
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
Reporter | ||
Comment 7•7 years ago
|
||
can use `this.refs.searchbox && this.refs.searchbox.focused`
Reporter | ||
Comment 8•7 years ago
|
||
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•7 years ago
|
||
@Fred: what about this patch, should we review/land it?
Honza
Flags: needinfo?(gasolin)
Reporter | ||
Comment 10•7 years ago
|
||
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?
Flags: needinfo?(gasolin) → needinfo?(ntim.bugs)
Updated•7 years ago
|
Flags: needinfo?(ntim.bugs)
Updated•7 years ago
|
Product: Firefox → DevTools
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•