Open Bug 1308441 Opened 8 years ago Updated 4 months ago

[Performance] Use react-virtualized for RequestList in NetMonitor panel

Categories

(DevTools :: Netmonitor, defect, P2)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(Not tracked)

People

(Reporter: steveck, Unassigned)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: [netmonitor])

Attachments

(5 files, 3 obsolete files)

Bug 1308441 - Use react-virtualized for RequestList
59 bytes, text/x-review-board-request
Details
Bug 1308441 - Fix test cases
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1308441 - Remove commonLibRequire trick
59 bytes, text/x-review-board-request
Honza
: review+
Details
net-panel-timings.png
127.79 KB, image/png
Details
react-virtualized-net-panel2.swf
2.11 MB, application/x-shockwave-flash
Details
Performance of the net panel is critical since slow rendering can alter timing measurements. Note that one of the main features of the tool is providing precise page load performance info.
Whiteboard: [devtools-html]
Whiteboard: [netmonitor]
Flags: qe-verify+
QA Contact: ciprian.georgiu
Priority: -- → P2
Depends on: 1312481
Some quick testing (always rendering only the last 20 rows) tells me that react-virtualized could bring a 30% performance improvement for page load. This makes this bug the most promising candidate for a performance improvement. Setting this as a blocker of bug 1321749 (the talos regression report). It's likely that we'll need to fully complete the XUL->HTML migration first, otherwise we might have difficulties with getting the layout right. Setting bug 1309826 as a dependency.
Assignee: nobody → jsnajdr
Blocks: 1321749
Depends on: 1309826
Priority: P2 → P1
Status: NEW → ASSIGNED
Iteration: --- → 53.5 - Jan 23
Iteration: 53.5 - Jan 23 → 54.1 - Feb 6
Iteration: 54.1 - Feb 6 → 54.2 - Feb 20
Assignee: jsnajdr → nobody
Status: ASSIGNED → NEW
Iteration: 54.2 - Feb 20 → ---
Priority: P1 → P2
Priority: P2 → P3
Whiteboard: [netmonitor] → [netmonitor-reserve]
Bug 1309826 XUL->HTML migration has done, so it's time to resume this work and get rid of some workarounds which we introduced at the beginning.
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Iteration: --- → 54.3 - Mar 6
Priority: P3 → P1
Ask for first round review but please note that arrow key navigation is missing because I suspect there is a framework issue from react virtualized itself. Issue has been filed at https://github.com/bvaughn/react-virtualized/issues/590. On the other hand, react virtualized doesn't support onContextMenu. So, a trick to do that is to implement a request-list-row.js and map unused onRowDoubleClick to onContextMenu. Jarda, do you have time to take a look for this patch? I'm not sure it's the right way to go, I hope there would be some performance improvement in your test case.
Flags: needinfo?(jsnajdr)
(In reply to Ricky Chien [:rickychien] from comment #5) > Jarda, do you have time to take a look for this patch? I'm not sure it's the > right way to go, I hope there would be some performance improvement in your > test case. Hi Ricky, I had a quick look at the patch and here are my comments: You're using the react-virtualized Table component and virtualizing both rows and columns in the request table. That looks like a complicated overkill to me. I think virtualizing just rows (i.e., using the List component) is all you need to get 100% of the performance benefit. The request list doesn't really scroll horizontally, does it? When using a List, the RequestListItem component can be kept simple and compact, instead of breaking it up into many Column wrappers. I see that the patch removes CSS styles for setting the width of the columns (in vw units) and replaces them with px sizes as Column props. Doesn't that keep the column widths constant even when you change the width of the window? In the current version, the column sizes are flexible and change when the window resizes. The patch (already big enough) contains some unrelated refactorings - the timing-markers reducer, WaterfallBackground, etc. Having them as separate microcommits would help the reviewer. And a last nit: when importing multiple components from react-virtualized, you might make use of a "createFactories" helper function that's used in Reps and Debugger code. I'm not sure how useful it is in this particular case, but it's worth having a look at. It's nice to see the great progress on netmonitor.html!
Flags: needinfo?(jsnajdr)
(In reply to Jarda Snajdr [:jsnajdr] from comment #7) > (In reply to Ricky Chien [:rickychien] from comment #5) > > Jarda, do you have time to take a look for this patch? I'm not sure it's the > > right way to go, I hope there would be some performance improvement in your > > test case. > > Hi Ricky, I had a quick look at the patch and here are my comments: > > You're using the react-virtualized Table component and virtualizing both > rows and columns in the request table. That looks like a complicated > overkill to me. I think virtualizing just rows (i.e., using the List > component) is all you need to get 100% of the performance benefit. The > request list doesn't really scroll horizontally, does it? https://github.com/bvaughn/react-virtualized/blob/master/docs/Table.md#table According to Table doc, it mentioned "Table content can scroll vertically but it is not meant to scroll horizontally." As a result, it will act like a List. Implementation of both List and Table are based on Grid, they are just a wrapper to return a Grid with a fixed columnCount (List [1] & Table [2]). [1] https://github.com/bvaughn/react-virtualized/blob/master/source/List/List.js#L149 [2] https://github.com/bvaughn/react-virtualized/blob/master/source/Table/Table.js#L318 > > When using a List, the RequestListItem component can be kept simple and > compact, instead of breaking it up into many Column wrappers. > > I see that the patch removes CSS styles for setting the width of the columns > (in vw units) and replaces them with px sizes as Column props. Doesn't that > keep the column widths constant even when you change the width of the > window? In the current version, the column sizes are flexible and change > when the window resizes. > > The patch (already big enough) contains some unrelated refactorings - the > timing-markers reducer, WaterfallBackground, etc. Having them as separate > microcommits would help the reviewer. > > And a last nit: when importing multiple components from react-virtualized, > you might make use of a "createFactories" helper function that's used in > Reps and Debugger code. I'm not sure how useful it is in this particular > case, but it's worth having a look at. > > It's nice to see the great progress on netmonitor.html! I'll separate this patch into smaller patches after this draft review :)
Comment on attachment 8839795 [details] Bug 1308441 - Small refactorings https://reviewboard.mozilla.org/r/114352/#review116312 ::: devtools/client/netmonitor/components/request-list-column-file.js:47 (Diff revision 4) > + cellData, > + dataKey, > + rowData, > + rowIndex, > +}) { > + let { urlDetails, responseContentDataUri } = rowData; nit: define destructuring assignments in separate line for consistency ::: devtools/client/netmonitor/components/request-list-empty.js:30 (Diff revision 4) > - className: "requests-list-empty-notice", > + className: "requests-list-empty-notice", > - }, > + }, > div({ className: "notice-reload-message" }, > span(null, L10N.getStr("netmonitor.reloadNotice1")), > - button( > - { > + button({ > + className: "devtools-button requests-menu-reload-notice-button", nit: `requests-list-reload-notice-button` ::: devtools/client/netmonitor/components/request-list.js:237 (Diff revision 4) > + * variables as part of the "style" property of a DOM element, we would use that. > + * > + * However, React doesn't support this, so we need to use a hack and update the > + * DOM element directly: https://github.com/facebook/react/issues/6411 > - */ > + */ > -function RequestList({ isEmpty }) { > + setScalingStyles(prevProps) { Is it make sense to handle `setScalingStyles` in `RequestListColumnWaterfall`? ::: devtools/client/netmonitor/selectors/requests.js:106 (Diff revision 4) > const getSelectedRequest = createSelector( > state => state.requests, > ({ selectedId, requests }) => selectedId ? requests.get(selectedId) : undefined > ); > > +function getSelectedDisplayedRequestIndex(state) { `getSelectedRequestIndex` might sufficient, and you can use reselector (`createSelector`) to cache the select result ::: devtools/client/themes/netmonitor.css:93 (Diff revision 4) > flex-direction: column; > width: 100%; > height: 100%; > } > > -.request-list-empty-notice { > +.requests-list-empty-notice { the style naming is followed the component name, so please rename the component as well if you plan to change the class
Comment on attachment 8839795 [details] Bug 1308441 - Small refactorings https://reviewboard.mozilla.org/r/114352/#review116312 > Is it make sense to handle `setScalingStyles` in `RequestListColumnWaterfall`? Yes, it should move to RequestListColumnWaterfall but right now it's a workaround since react doesn't support setting CSS variables as part of the "style" property. It wouldn't be efficient if we move workaround to RequestListColumnWaterfall since it doesn't sync with react DOM update and you have to invoke it in componentDidMount/componentDidUpdate...etc. Ideally, child component will be updated more frequently than parent component, so it would be more efficient if we leave it in parent (RequestList). > `getSelectedRequestIndex` might sufficient, and you can use reselector (`createSelector`) to cache the select result Ｗe need to add this because we assume to get a sorted result by calling getSortedRequests(), otherwise the request list will be in initial order which is incorrect.
(In reply to Jarda Snajdr [:jsnajdr] from comment #7) > I see that the patch removes CSS styles for setting the width of the columns > (in vw units) and replaces them with px sizes as Column props. Doesn't that > keep the column widths constant even when you change the width of the > window? In the current version, the column sizes are flexible and change > when the window resizes. I hope the px is more enough to fit our need. The reason why I converted original css to px is because React Virtualized prefers to use px as props in their components, like Table[1] Column[2]. Moreover, for example, the width of Column is also required, it's hard to remove px number if we decide to adopt React Virtualized. There is a slightly difference in new UI but it still looks good to me. [1] https://github.com/bvaughn/react-virtualized/blob/master/docs/Table.md [2] https://github.com/bvaughn/react-virtualized/blob/master/docs/Column.md
Patch is ready for review. Please note that react-virtualized doesn't work properly and UI will be broken if we apply this patch after bug 1341159 (which has landed recently). There are several of places using "instanceof Function" to detect props in react virtualized, and which will always return false because react and react virtualized are loaded in deferent context [1]. I've submitted a patch for react virtualized [2] but the latest version still doesn't work unfortunately. It threw a document.body not found error since we're in XUL document doesn't have a body attribute like HTML. The root cause is due to ReactDom monkey-patching, so it could be fixed in bug 1342297 and get rid of the trick in bug 1341159. If you'd like to test this patch, please revert the commit before bug 1341159. [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof#instanceof_and_multiple_context_(e.g._frames_or_windows) [2] https://github.com/bvaughn/react-virtualized/pull/596
Attachment #8840403 - Flags: feedback?(jsnajdr)
I'm trying to run g2 talo test on try with repeating 5 times ./mach try -b o -p linux64,macosx64,win32,win64 -u none -t g2,g2-e10s --rebuild-talos 5 Here is a comparison result but talo test is weird for me, it didn't generate 5 times result in linux 64 build. But overall result seems not bad. https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2edd08176c86&newProject=try&newRevision=4fe2a7f28a37&framework=1&filter=damp&showOnlyImportant=0
Ricky, I am seeing collisions when applying the first patch on m-c. Can you please rebase? patching file devtools/client/netmonitor/netmonitor-controller.js Hunk #1 FAILED at 3 Hunk #3 FAILED at 896 2 out of 3 hunks FAILED -- saving rejects to file devtools/client/netmonitor/netmonitor-controller.js.rej patching file devtools/client/netmonitor/netmonitor.js Hunk #1 FAILED at 0 1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/netmonitor.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory Honza
Flags: needinfo?(rchien)
Patch has updated!
Flags: needinfo?(rchien)
Comment on attachment 8839795 [details] Bug 1308441 - Small refactorings https://reviewboard.mozilla.org/r/114352/#review118276 ::: devtools/client/netmonitor/netmonitor.js:18 (Diff revision 11) > const { render } = require("devtools/client/shared/vendor/react-dom"); > const Provider = createFactory(require("devtools/client/shared/vendor/react-redux").Provider); > + const { configureStore } = require("./store"); > + const store = window.gStore = configureStore(); > + const { NetMonitorController } = require("./netmonitor-controller"); > + NetMonitorController.toolbox = toolbox; In order to make this pach (+ the 'rebase' patch) independent from others it would be good if we also rename the `_toolbox` in RequestListContent component. See RequestListContent.RequestListContent, `window.NetMonitorController._toolbox.doc` is passed into `HTMLTooltip`. These two (refactroing) patches can get R+ and we can focus on the `react-virtualized` one.
Attachment #8839795 - Flags: review?(odvarko)
Comment on attachment 8842818 [details] Bug 1308441 - Remove commonLibRequire trick https://reviewboard.mozilla.org/r/116594/#review118280 Looks like your change [1] could be in React-Virtualized 9.1.0+! Honza [1] https://github.com/bvaughn/react-virtualized/pull/596
(In reply to Jan Honza Odvarko [:Honza] from comment #56) > Comment on attachment 8842818 [details] > Bug 1308441 - Remove commonLibRequire trick > > https://reviewboard.mozilla.org/r/116594/#review118280 > > Looks like your change [1] could be in React-Virtualized 9.1.0+! > > Honza > > [1] https://github.com/bvaughn/react-virtualized/pull/596 Yeah! note that we still need to revert commonLibRequire trick so that React-Virtualized 9.1.0+ can work as expected. Because there are other issues in 9.1.0 which is using document.body but XUL document doesn't support that.
Comment on attachment 8842818 [details] Bug 1308441 - Remove commonLibRequire trick https://reviewboard.mozilla.org/r/116594/#review118328
Attachment #8842818 - Flags: review?(odvarko) → review+
Comment on attachment 8840403 [details] Bug 1308441 - Use react-virtualized for RequestList https://reviewboard.mozilla.org/r/114926/#review118288 I have been playing with this today and here are some comments: - I used Jarda's devtools-performer add-on to test timing https://github.com/jsnajdr/devtools-performer I am not seeing much improvements though. - We should carefuly analyse the panel's performance using new perf tools https://perf-html.io/ It's great tool and it can help us to identify places with performance issues - I suspect the following things (perf issues): * CSS re-layout. We should simplify request-list markup so, CSS recalc layout is fast (e.g. avoid flexbox, use fixed row height, reduce amount of divs and spans) * I think that the waterfall could also be simplified (it also might be source of CSS perf issues). I tried to quickly change size of the browser window and observe how quickly is the Net panel content re-rendered. It's definitelly slower than e.g. Firebug. * Are we properly implementing "shouldComponentUpdate" when necessary? * I still think that we should remove the image preview (doesn't have to be big perf improvement though) --- Performance is definitelly our top priority and we should focus on it before anything else. Honza ::: devtools/client/netmonitor/components/moz.build:17 (Diff revision 12) > + 'request-list-column-method.js', > + 'request-list-column-size.js', > + 'request-list-column-status.js', > + 'request-list-column-transferred.js', > + 'request-list-column-type.js', > + 'request-list-column-waterfall.js', Nice! I always wanted to follow the rule: one file == one component.
(In reply to Jan Honza Odvarko [:Honza] from comment #64) > Comment on attachment 8840403 [details] > Bug 1308441 - Use react-virtualized for RequestList > > https://reviewboard.mozilla.org/r/114926/#review118288 > > I have been playing with this today and here are some comments: > > - I used Jarda's devtools-performer add-on to test timing > https://github.com/jsnajdr/devtools-performer > I am not seeing much improvements though. Yeah, it's not good but also not bad. According to the outcome of comment 47, it shows some improvements on Windows 7 but it didn't impact on others so much. At least we can say react-virtualized could improve when scrolling in enormous number of requests, it will not insert all requests on DOM at once. > - We should carefuly analyse the panel's performance using new perf tools > https://perf-html.io/ > It's great tool and it can help us to identify places with performance issues > > - I suspect the following things (perf issues): > > * CSS re-layout. We should simplify request-list markup so, CSS recalc > layout is fast > (e.g. avoid flexbox, use fixed row height, reduce amount of divs and spans) > > * I think that the waterfall could also be simplified (it also might be > source of CSS perf issues). > I tried to quickly change size of the browser window and observe how quickly > is the Net panel > content re-rendered. It's definitelly slower than e.g. Firebug. Agree. browser window resize always trigger waterfall resize event, resizing becomes laggy in this time. Waterfall can be addressed in bug 1308695 which takes over this part of refactoring, bug title can be changed in order to find out a better solution for improving waterfall. > * Are we properly implementing "shouldComponentUpdate" when necessary? Yes, I'd like to implement shouldComponentUpdate() for all type of columns in a separate bug. This bug will focus on introducing react-virtualized first and make sure we don't bring any new performance regressions. Filed bug 1344107 for shouldComponentUpdate. > * I still think that we should remove the image preview (doesn't have to be > big perf improvement though) Agree! I'm still like to see image preview in ResponsePanel which offers more details for images. Maybe we can discuss this in UI analysis to propose to drop all tooltips such as images, stack trace, waterfall (not implemented yet).
No longer blocks: 1344107
It's a good change to get a little pref improvement through removing overscanRowCount: 20 props in request-list.js. Honza, I can see scrolling is smoother after removing overscanRowCount, could you try it on your machine? The issue of transitory blank while scrolling too fast is not that obvious on my machine, could you try to play with this example https://bvaughn.github.io/react-virtualized/#/components/Table to see this issue still happen?
Flags: needinfo?(odvarko)
The majority of performance impact in request list is image preview (using data uri) in file column and the second candidate is waterfall column. An easy way to get performance improvement is to remove image preview, so it would be obviously smoother when loading large website like http://edition.cnn.com.
(In reply to Ricky Chien [:rickychien] from comment #72) > It's a good change to get a little pref improvement through removing > overscanRowCount: 20 props in request-list.js. > > Honza, I can see scrolling is smoother after removing overscanRowCount, > could you try it on your machine? I tried and I think there is an improvement but, the problem is still visible. (In reply to Ricky Chien [:rickychien] from comment #73) > The majority of performance impact in request list is image preview (using > data uri) in file column and the second candidate is waterfall column. An > easy way to get performance improvement is to remove image preview, so it > would be obviously smoother when loading large website like > http://edition.cnn.com. Agreed I also think that changing the CSS Layout (for request list) and optimizing layout calculation can have good impact. Honza
Flags: needinfo?(odvarko)
(In reply to Ricky Chien [:rickychien] from comment #72) > The issue of transitory blank while scrolling too fast is not that obvious > on my machine, could you try to play with this example > https://bvaughn.github.io/react-virtualized/#/components/Table to see this > issue still happen? Do you still see transitory blank in above example on your machine?
Flags: needinfo?(odvarko)
Here is an issue about scrolling "scroll performance issues in Firefox" https://github.com/bvaughn/react-virtualized/issues/518#issuecomment-281220155
Attachment #8843578 - Attachment is obsolete: true
Patch has been updated and merge with shouldComponentUpdate (bug 1344107). Here is summary of changes: * Remove image preview (showing data URI as an image) in file column which brings a huge performance improvement. * Remove window resize for waterfall column, which reduces a lots of latency when resizing browser window. I'm seeing the usability of waterfall column still acceptable in most cases but it's really smoother than before. * Reduce some dynamic DOM element if possible for columns. * Reduce the times of react component reconciliation by adding shouldComponentUpdate(). * Rewrite and clean up CSS for requests-list and related columns. Lots of unused CSS has removed and simplified. * Remove flex layout and use fixed width for request list item in order to reduce reflow times. * Reflow happens when new data update from NetworkEventUpdate which triggers many request column update, so flex layout will re-calcuate layout and then slow down network panel. * Set react virtualized table overscanRowCount to 0 to speed up performance when loading large website like CNN. Known issues: * I'm still seeing transitory blank while scrolling in request list that could be the bug in Firefox [1]. I can see in http://edition.cnn.com, the update of request list item and scrolling is smoother in the meantime new requests is loading. Old request list is pretty laggy with obvious frame drop when scrolling in such large website. I believe the [1] is our platform issue and it should be addressed by platform team. Moreover, this shouldn't be a blocker here but I'll ask our platform engineer for help about this. [1] https://github.com/bvaughn/react-virtualized/issues/518#issuecomment-281220155
Update patch to drop keyCodes library but use standard KeyboardEvent.key instead since KeyboardEvent.keyCode is deprecated. https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode Off topic UI proposal: In order to optimize render speed for request list, preview image has been removed and outcome looks great. So, the image tooltip has removed as well since there is no image in file column anymore. The only tooltip in netmonitor is stack trace tooltip but I'm still thinking that isn't useful and hard to discoverable. I'd suggest to remove it JS cause icon and tooltip as well. As a result, we can get rid of the onHover trick in request-list.js and request-list-tooltip.js and toolbox.viewSourceInDebugger APIs from netmonitor.
I've discussed with Ricky offline. The new proposal to avoid reflow is to add a placeholder before every file, instead of remove images and tooltip. If this approach works, we can keep thumbnails and image preview tooltip, so no related regression will be filed after this bug land. The related request menu improvement proposal is in UI analysis doc.
Attachment #8839795 - Attachment is obsolete: true
Attachment #8839795 - Flags: review?(odvarko)
Add a new patch - Support image preview. I can see after avoiding reflow in file column, request list performance looks good and it's still smoother than before. Honza, I uploaded Support image preview as a standalone patch so that you can opt in to test it on your machine. The performance issue doesn't noticeable on my machine, so maybe you can do me a favor to compare the result for these two versions (with image preview and without image preview). Thanks!
Iteration: 54.3 - Mar 6 → 55.1 - Mar 20
After doing a survey about the transitory blank of scrolling with our platform engineer, the conclusion indicates that the root cause is due to Async Pan/Zoom (a.k.a APZ) [1]. A simple experiment is to disable APZ in Nightly by pref off `layers.async-pan-zoom.enabled` in about:config (which needs to restart browser). And then try to scroll as fast as you can in this example https://bvaughn.github.io/react-virtualized/#/components/Table (You can reduce `Overscan` prop to 0 to see scrolling effect obviously). After disabling APZ, there is no obvious transitory blank when scrolling, but I can notice there drop frame and laggy in our netmonitor. [1] https://wiki.mozilla.org/Platform/GFX/APZ
Flags: needinfo?(odvarko)
Note that there is a talos regression (bug 1321749) after landing react redux request list, but react-virtualized doesn't help with making the damp result better because the react-virtualized focuses on improving front-end rendering, it's main improvement which reflects to users. The damp.js does merely verify the packet arrival of network requests [1] rather than render performance. [1] http://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#120
Jarda, do you have any idea the root cause of damp talos regression in bug 1321749 and is there any change to improve it? React-virtualized isn't going to help with mitigating the talos regression because damp.js doesn't focus on UI rendering (see also comment 114). Thanks!
Flags: needinfo?(jsnajdr)
(In reply to Ricky Chien [:rickychien] from comment #115) > Jarda, do you have any idea the root cause of damp talos regression in bug > 1321749 and is there any change to improve it? The root cause is that using React, Redux and Immutable.js is slower than hand-coding all the DOM updates yourself and using native JS data structures. You get some very nice improvements in app architecture, but if the original code was well written, you need to fight hard to get back to the previous performance level. > React-virtualized isn't going to help with mitigating the talos regression > because damp.js doesn't focus on UI rendering (see also comment 114). There are two timing measurements in the netmonitor damp test: - reload: measures the time to load the page in the content process. Measures only overhead in the netmonitor backend, not the RDP traffic and frontend rendering - requestsFinished: measures the time to display everything in the frontend, including the rendering time. If you look carefully, the events that waitForAllRequestsFinished is waiting for are fired only after the Redux actions are dispatched and the React rendering is done (synchronously). In current React version, rendering is done synchronously after calling setState(). This might change in the future (Fiber), but last time I checked, everything was synchronous. The 'requestsFinished' timing was added in bug 1328553. Before that, the DAMP timings were not very well-defined and were giving random results. Comparing current DAMP results with bug 1321749 is longer meaningful today, before the 'reload' and 'close' timings are done slightly differently. I think that bug 1321749 can be closed and solved. But performance should be still a major goal, of course - Chrome's netmonitor is several times faster, so there is a lot of room for improvement. Some performance improvement ideas that are worth considering: - try to minimize the number of elements that are rendered. The React render() call should be super-fast. Currently, almost every request table cell is a <div> with an embedded <span>. After a little CSS tweaking, you could get rid of the <span> elements and cut the number of rendered elements almost in half. - I'm not sure about the impact of Greg's React monkeypatching - maybe removing it and using the platform iframe patch that ntim was working on would help. - improve the RDP protocol that is too chatty now and transfers a lot of information that is not needed. For example, the event timings could be all sent in one notification, and there is no need for a getEventTimings call. The cookies don't need to be parsed by the backend every time. Make the parsing lazy and wait until the frontend actually wants to see them. All the frontend needs is the raw headers - everything else can be trivially parsed from them, and the backend doesn't need to waste time there. The chatty protocol traffic is not visible in Cleopatra profiles, but I suspect that it posts a lot of little runnables to the event loop and keeps it unnecessarily busy. Cleopatra displays the time that a runnable spends in the loop queue before being executed, and it's quite long. - optimize the netmonitor backend - it's executing quite a lot of JS code now - can you already run the netmonitor.html in an unprivileged content tab? Try to run it in Chrome. Maybe Chrome's performance tools will show you problems that Firefox tools don't see.
Flags: needinfo?(jsnajdr)
Thanks for sharing this point of view Jarda! Here is my comment but I agree most of you :) (In reply to Jarda Snajdr [:jsnajdr] from comment #116) > - requestsFinished: measures the time to display everything in the frontend, > including the rendering time. If you look carefully, the events that > waitForAllRequestsFinished is waiting for are fired only after the Redux > actions are dispatched and the React rendering is done (synchronously). In > current React version, rendering is done synchronously after calling > setState(). This might change in the future (Fiber), but last time I > checked, everything was synchronous. setState() call is asynchronous by design [1] and we can verify this easily by adding a console.log(this.state.xxx) after a setState() call. Any redux state change will trigger subscribe listener [2] which is a low level API and used in react-redux connect() to pass props to react component. As a result, I'd assume both of them are asynchronous and our requestsFinished() mechanism will not wait for React rendering is done. > The 'requestsFinished' timing was added in bug 1328553. Before that, the > DAMP timings were not very well-defined and were giving random results. > Comparing current DAMP results with bug 1321749 is longer meaningful today, > before the 'reload' and 'close' timings are done slightly differently. I > think that bug 1321749 can be closed and solved. Cool! That's good news for us. We can close the bug 1321749 based on your comment if there is no objections. > But performance should be still a major goal, of course - Chrome's > netmonitor is several times faster, so there is a lot of room for > improvement. > > Some performance improvement ideas that are worth considering: > > - try to minimize the number of elements that are rendered. The React > render() call should be super-fast. Currently, almost every request table > cell is a <div> with an embedded <span>. After a little CSS tweaking, you > could get rid of the <span> elements and cut the number of rendered elements > almost in half. Yes, completely agree with that. This patch has included this CSS improvements and reduce the chance of reflow as many as we could. Please see comment 90 for patch summary. > - I'm not sure about the impact of Greg's React monkeypatching - maybe > removing it and using the platform iframe patch that ntim was working on > would help. Yeah, it's a possible chance to get performance improvement, let's see. > - improve the RDP protocol that is too chatty now and transfers a lot of > information that is not needed. For example, the event timings could be all > sent in one notification, and there is no need for a getEventTimings call. > The cookies don't need to be parsed by the backend every time. Make the > parsing lazy and wait until the frontend actually wants to see them. All the > frontend needs is the raw headers - everything else can be trivially parsed > from them, and the backend doesn't need to waste time there. > > The chatty protocol traffic is not visible in Cleopatra profiles, but I > suspect that it posts a lot of little runnables to the event loop and keeps > it unnecessarily busy. Cleopatra displays the time that a runnable spends in > the loop queue before being executed, and it's quite long. > > - optimize the netmonitor backend - it's executing quite a lot of JS code now Agree. I've seeing the updateRequest in controller would be a chance to slow down our speed. We should rethink about our backend architecture like debuggerClient and webConsoleClient which are responsible for emitting networkEvent and networkEventUpdate packets. Rearrange this packet to fit our architecture can get more performant. This could be the next big step for improving performance for netmonitor. > - can you already run the netmonitor.html in an unprivileged content tab? > Try to run it in Chrome. Maybe Chrome's performance tools will show you > problems that Firefox tools don't see. We haven't ready yet, but this is the first priority for us to make netmonitor runs on content tab. This suggestion looks great for me. I'm looking forward to seeing netmonitor can be profiled on Chrome's tools. [1] https://medium.com/@wereHamster/beware-react-setstate-is-asynchronous-ce87ef1a9cf3#.k33ls0g1p [2] http://redux.js.org/docs/api/Store.html#subscribelistener
I have been testing the new patch and here are some comment: 1) Image preview Great to hear that removing the image thumbnails helped. I think that we should *not* put it back since calculating URI data is expensive and also fetching the response body from the backend causes high RDP traffic. It now happens automatically which is bad. See also the next point. 2) Don't send response body, post data, headers, cookies, etc. HTTP details data are currently requested automatically (in NetworkEventsHandler._onNetworkEventUpdate). The request list doesn't need the details and fetching them causes huge RDP traffic (especially response bodies). I think we only need the basic data (sent with the update event) and timings. I tried to disable this and I observed significant performance improvement. See also the attached screenshot (timings) 3) Yes, disabling APZ fixes the transitory blank screen during scrolling. The scrolling UX is too bad when the transitory blank screen appears so often I don't think we can land it. I am afraid that react-virtualized won't help us much. It's good for *initial* rendering of huge lists but, in our case the list is built incrementally as HTTP requests are executed by the page. My feeling is that we can postpone react-virtualized and land all the other performance improvements. 4) Btw. I am not seeing 'load' and 'DOMContentLoaded' events in the waterfall graph (vertical lines) 5) I am unsure how removing tooltips can help (comment #94) I think that we should keep tooltips in place (at least not remove them as part of this bug). Let's keep the changes focused to performance, land step by step and avoid huge patch. From perf perspective, image preview tooltip is fine as long as data (response body) is fetched on demand - when the tooltip needs to be rendered. Not automatically for every request. Honza
Attached image net-panel-timings.png —
Here are timings I have collected through the devtools-performer extension. I am not entirely sure which number is most useful for us but, I have been usually watching the the 'reload' (when not fetching the details data) and 'requests'. Honza
Btw. I've been using RDP Inspector to monitor the RDP traffic. https://addons.mozilla.org/en-US/firefox/addon/rdp-inspector/ Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #118) > I have been testing the new patch and here are some comment: > > 1) Image preview > Great to hear that removing the image thumbnails helped. I think that we > should *not* put it back since calculating URI data is expensive and also > fetching the response body from the backend causes high RDP traffic. It now > happens automatically which is bad. See also the next point. Agree. I'll remove last patch (Bug 1308441 - Support image preview r?honza) to drop thumbnails in file column. > 2) Don't send response body, post data, headers, cookies, etc. > HTTP details data are currently requested automatically (in > NetworkEventsHandler._onNetworkEventUpdate). The request list doesn't need > the details and fetching them causes huge RDP traffic (especially response > bodies). I think we only need the basic data (sent with the update event) > and timings. I tried to disable this and I observed significant performance > improvement. See also the attached screenshot (timings) Yes, I'm also thinking about this could bring large help. I'd like to improve this part as a follow up bug and there is a reason I prefer to keep react-virtualized, please see below. > 3) Yes, disabling APZ fixes the transitory blank screen during scrolling. > The scrolling UX is too bad when the transitory blank screen appears so > often I don't think we can land it. I am afraid that react-virtualized won't > help us much. It's good for *initial* rendering of huge lists but, in our > case the list is built incrementally as HTTP requests are executed by the > page. My feeling is that we can postpone react-virtualized and land all the > other performance improvements. Here is an alternative solution: Set table's overscanRowCount props to 100 or higher (default sets to 0) which reduces the transitory blank effect during scrolling although it's still visible. But what I see is that scrolling is smoother than without react-virtualized and there is no laggy and drop frame when scrolling and page is loading in the meantime. > 4) Btw. I am not seeing 'load' and 'DOMContentLoaded' events in the > waterfall graph (vertical lines) I'll check it later. > 5) I am unsure how removing tooltips can help (comment #94) > I think that we should keep tooltips in place (at least not remove them as > part of this bug). Let's keep the changes focused to performance, land step > by step and avoid huge patch. From perf perspective, image preview tooltip > is fine as long as data (response body) is fetched on demand - when the > tooltip needs to be rendered. Not automatically for every request. OK, if we decide to keep image preview tooltip, we need to find out a place for hovering and showing the tooltip because we agree #1 to drop image thumbnail in file column. That means right now we don't have a good anchor for hovering and showing image preview tooltip. Here is a good option to show image tooltip when hovering on a text of cause column if type is an image. What do you think?
Flags: needinfo?(odvarko)
> OK, if we decide to keep image preview tooltip, we need to find out a place > for hovering and showing the tooltip because we agree #1 to drop image > thumbnail in file column. That means right now we don't have a good anchor > for hovering and showing image preview tooltip. > > Here is a good option to show image tooltip when hovering on a text of cause > column if type is an image. What do you think? As suggest in comment 99, a placeholder icon (based on file type) before the file name in `file` column could serve image preview tooltip well and increase the performance (because of avoid column reflow and thumbnail retrieving) We can 1. do so in a separate bug (Put placeholder icons by type before the file name in `file` column) 2. close the bug 1321749 (based on comment 116 and above bug's performance improvement) 3. put columns in separate component and reduce elements, we can re-evaluate if we still want react-vertualized at this time
(In reply to Fred Lin [:gasolin] from comment #122) > We can > > 1. do so in a separate bug (Put placeholder icons by type before the file > name in `file` column) Yes, no matter what's our final decision about this. We still can remove image icon in this bug. And so far I don't see a good empty file icon which fits our file column in devtools folder, so new placeholder icon might need visual designer's help. > 2. close the bug 1321749 (based on comment 116 and above bug's performance > improvement) Closed! > 3. put columns in separate component and reduce elements, we can re-evaluate > if we still want react-vertualized at this time Fred, could you explain what's your expectation about this? Thanks!
Attachment #8843905 - Attachment is obsolete: true
Attachment #8843905 - Flags: review?(odvarko)
After discussion in meeting, we decided to drop image preview in this bug but figure out an alternative solution for showing image tooltip. Placeholder icon before file name is a good suggestion as Fred mentioned in comment 122. Patch has updated and increased table's overscanRows to 100, the rendering and performance outcome looks good to me on my machine. Honza, we'd like to take a look of this newer result? Maybe it's a good tradeoff between performance and scrolling problem. It looks much smoother when scrolling and network requests are loading but the transitory blank is not too noticeable as previous version (overscanRows sets to 0). If the outcome is acceptable, we can land this performance improvement with smaller modifications. But if it's not, we need to rewrite entire request-list and its sub-components to support column layout issue, re-apply CSS change from this patch and rewrite div, span element reduction again in a separate bug. Because here is a large component refactoring for adapting react-virtualized. (See also attachment 8840403 [details] for more details)
(In reply to Ricky Chien [:rickychien] from comment #132) > Honza, we'd like to take a look of this newer result? Yes, thanks! > Maybe it's a good > tradeoff between performance and scrolling problem. It looks much smoother > when scrolling and network requests are loading but the transitory blank is > not too noticeable as previous version (overscanRows sets to 0). I am attaching a screen-cast of how it works on my machine (overscanRowCount == 100). The scrolling experience is still flaky on my machine. There are two things I am observing: 1) Setting overscanRowCount to 100 means that a lot more rows needs to be rendered. Since re-rendering happens during scrolling (e.g. as the user is dragging the scroll-thumb) the time needed for re-rendering affects the experience. 200+visible rows need more time to render and there is visible lag when the content is rendering. 2) I can still see the transitory blank (testing with cnn.com). If there is more than 200 requests and I scroll from top to bottom to see the last one the blank screen is still noticeable. See the screencast. To sum up. I think that having `overscanRowCount == 100` makes the virtualized environment unnecessary for cases where we have less then ~200 requests since all of them are actually rendered. And for cases where there is ~200+ requests I can see the transitory blank screen. I am voting to *not* use react-virtualized at this point and focus on optimizing other suggested things. We would be in different spot if we had a flag that can switch off APZ for the Net panel so, we might talk to the platform team first. > If the outcome is acceptable, we can land this performance improvement with > smaller modifications. But if it's not, we need to rewrite entire > request-list and its sub-components to support column layout issue, re-apply > CSS change from this patch and rewrite div, span element reduction again in > a separate bug. Because here is a large component refactoring for adapting > react-virtualized. (See also attachment 8840403 [details] for more details) Yes, I think this is needed and we can discuss details. Honza
Flags: needinfo?(odvarko)
I would advise against setting `overscanRowCount` anywhere near as high as 100 rows. I agree with Honza. That's going to undo a lot of the performance benefits of using react-virtualized. I'm pretty bummed that the APZ issue might keep RV from being used here. If there's anything I can do to help (unlikely, but just in case...) please let me know.
Network Monitor team might already be aware of this, but I just wanted to make note that Inspector team is also encountering trouble with APZ, and they having been looking into possible solutions in bug 1345434 and others linked from there. Let's try to work together across DevTools on APZ issues, so that we can be sure we present them all to Platform as needed.
See Also: → 1345434
Iteration: 55.1 - Mar 20 → ---
Blocks: netmonitor-phaseII
No longer blocks: netmonitor-html
Whiteboard: [netmonitor-reserve] → [netmonitor]
Iteration: --- → 55.2 - Apr 3
Comment on attachment 8840403 [details] Bug 1308441 - Use react-virtualized for RequestList As we discussed last week, using react-virtualized depends on solving the APZ problem. This can take time so, I am removing myself from the review for now. Honza
Attachment #8840403 - Flags: review?(odvarko)
Iteration: 55.2 - Apr 3 → 55.3 - Apr 17
Assignee: rchien → nobody
Status: ASSIGNED → NEW
Iteration: 55.3 - Apr 17 → ---
Priority: P1 → P2
Summary: Use react-virtualized for RequestList in NetMonitor panel → [Performance] Use react-virtualized for RequestList in NetMonitor panel
Product: Firefox → DevTools

I'll take a look.

Assignee: nobody → bwerth
Assignee: bwerth → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: