Closed
Bug 1434855
Opened 7 years ago
Closed 7 years ago
HAR export is very slow
Categories
(DevTools :: Netmonitor, defect, P2)
DevTools
Netmonitor
Tracking
(firefox60 fixed)
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: ochameau, Assigned: Honza)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
STR: * open netmonitor * visit duckduckgo.com * Right click on any request * Click on "Save all as HAR" Here it takes about 10 seconds before the file save dialog appears. Here is a profile of such STR: https://perfht.ml/2npGY8N Both parent and content processes aren't very busy. They are mostly waiting. There is a lot of very small react updates that accumulates, but that doesn't explain such long delay. Everytime we request a lazy attribute like response content, headers, cookies, ... redux store is updated and react then also checks for updates. There is no reflows being done. It is only redux and especially react work to check for updates. For example, the StatusBar, as a pure function gets its render method called n-times for each request. 'n' being equal to the number of lazy attribute we fetch. bug 1434848 is opened to optimize this. But there is other React components trying to update, like RequestListContent. This one will be harder to optimize. So we can optimize all this, but that is not the main reason why this feature is so slow. The reason is that when HAR calls connector's `requestData`, it waits for redux actions to complete: https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-data-provider.js#417-424 https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-data-provider.js#124 If I comment them both, the same STR takes only 2 to 3 seconds to complete. I imagine redux action only complete after the action is really dispatched, after the throttling. So if we have "n" requests, and we fetch for "x" lazy attributes it will wait for n*50ms*x. duckduckgo.com spawn 18 requests and at first sight, HAR fetches 7 attributes lazily, so 18*7*50 = 6.3 seconds.
Assignee | ||
Updated•7 years ago
|
Has STR: --- → yes
Priority: -- → P2
Assignee | ||
Updated•7 years ago
|
Blocks: netmonitor-har
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
Alex, here are some comments: - The patch focuses on avoiding the timeouts. - I'd like to keep the patch simple and land it yet in 60 (or uplift) - So, all other improvements should be filled as follow ups (if not already) - I am also working on extending DAMP test (bug 1434849), but this bug should land first. The DAMP test is too slow for complicated.netmonitor Honza
Assignee | ||
Comment 3•7 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a93226925ce403e98b6504760075a8d465c9eade Honza
Reporter | ||
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8955186 [details] Bug 1434855 - Refactor HAR builder options; https://reviewboard.mozilla.org/r/224344/#review230922 Works great! Please just make this patch less hacky as suggested in next comment. > - So, all other improvements should be filled as follow ups (if not already) Do you have in mind some followups? ::: devtools/client/netmonitor/src/har/har-exporter.js:200 (Diff revision 1) > + form: { title, url } > + } = getTabTarget(); > + > + // Disconnect from redux actions/store. > + let actions = connector.connector.dataProvider.actions; > + connector.connector.dataProvider.actions = {}; A couple of comments here: - Please consider splitting your patches within bugs. (You don't have to attach only one patch per bug) The important modification you do is right here and in line 214, where you reset action property. This is hidden by the rest of your patch which is the refactoring of `buildHarData`'s `options` arguments. - This is too hacky. You are playing from `HarExporter` with `FirefoxDataProvider` internals. You should address that in this patch, not in a followup. You should expose helpers on `FirefoxDataProvider` to make that hack explicit from firefox-data-provider.js. Like adding `FirefoxDataProvider.{start|stop}ReduxUpdates()` or similar. - Don't you think we should try to keep doing the redux updates? By disabling it, it means that the UI will redo the RDP request that HAR already did. This is good fit for followup. - Note that in the current form, FirefoxDataProvider can be instanciated outside of netmonitor UI if that helps simplifying HAR codebase when run without a toolbox. It just expect a `webConsoleClient` and `actions` can be set to such empty object. We can consider moving this module outside of netmonitor ui, it looks more like a client helper. May be https://searchfox.org/mozilla-central/source/devtools/shared/webconsole, next to console client. (it might also help the webextension usage)
Attachment #8955186 -
Flags: review?(poirot.alex) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #4) > Do you have in mind some followups? I have been thinking mostly about better way how to avoid the timeouts e.g. instead of avoiding redux updates (by not firing redux actions), we could rather customize the batching middle-ware or do something better. > > ::: devtools/client/netmonitor/src/har/har-exporter.js:200 > (Diff revision 1) > > + form: { title, url } > > + } = getTabTarget(); > > + > > + // Disconnect from redux actions/store. > > + let actions = connector.connector.dataProvider.actions; > > + connector.connector.dataProvider.actions = {}; > > A couple of comments here: > - Please consider splitting your patches within bugs. (You don't have to > attach only one patch per bug) > The important modification you do is right here and in line 214, where you > reset action property. > This is hidden by the rest of your patch which is the refactoring of > `buildHarData`'s `options` arguments. Done > - This is too hacky. You are playing from `HarExporter` with > `FirefoxDataProvider` internals. You should address that in this patch, not > in a followup. You should expose helpers on `FirefoxDataProvider` to make > that hack explicit from firefox-data-provider.js. Like adding > `FirefoxDataProvider.{start|stop}ReduxUpdates()` or similar. Yes, done > - Don't you think we should try to keep doing the redux updates? By > disabling it, it means that the UI will redo the RDP request that HAR > already did. This is good fit for followup. Created bug 1443567 > - Note that in the current form, FirefoxDataProvider can be instanciated > outside of netmonitor UI if that helps simplifying HAR codebase when run > without a toolbox. It just expect a `webConsoleClient` and `actions` can be > set to such empty object. We can consider moving this module outside of > netmonitor ui, it looks more like a client helper. May be > https://searchfox.org/mozilla-central/source/devtools/shared/webconsole, > next to console client. (it might also help the webextension usage) Yep, sounds good to me, I need to thing a bit more about this. Thanks for the review Alex! Honza
Reporter | ||
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8956519 [details] Bug 1434855 - Improve performance of HAR export; https://reviewboard.mozilla.org/r/225430/#review231622 Thanks a lot for the changeset split. It really eases the review! I wasn't expecting you to do it after the fact. It was more a suggestion for next patches ;) ::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:42 (Diff revision 1) > // Event handlers > this.onNetworkEvent = this.onNetworkEvent.bind(this); > this.onNetworkEventUpdate = this.onNetworkEventUpdate.bind(this); > } > > + enableActions(enable) { A brief comment about the purpose of this method here would be helpful.
Attachment #8956519 -
Flags: review?(poirot.alex) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #8) > Thanks a lot for the changeset split. It really eases the review! > I wasn't expecting you to do it after the fact. > It was more a suggestion for next patches ;) I am always happy to make the review easier :-) > > + enableActions(enable) { > > A brief comment about the purpose of this method here would be helpful. Done Honza
Comment 12•7 years ago
|
||
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a87683b49741 Refactor HAR builder options; r=ochameau https://hg.mozilla.org/integration/autoland/rev/ceea3f9776b1 Improve performance of HAR export; r=ochameau
![]() |
||
Comment 13•7 years ago
|
||
Backed out for failing browser-chrome's browser/components/extensions/test/browser/browser_ext_devtools_network.js: https://hg.mozilla.org/integration/autoland/rev/4d03290053fc523b65eb3193f53333b71be906ca Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ceea3f9776b1887dcfe3135778b8234b38f9f0d8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Log: https://treeherder.mozilla.org/logviewer.html#?job_id=166543760&repo=autoland 18:12:59 INFO - 317 INFO TEST-PASS | browser/components/extensions/test/browser/browser_ext_devtools_network.js | The resolved value is equal to the one received in the callback API mode - 18:12:59 INFO - 318 INFO Leaving test bound test_devtools_network_on_request_finished 18:12:59 INFO - Buffered messages finished 18:12:59 ERROR - 319 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_devtools_network.js | message queue is empty - Got ["onRequestFinished-callbackExecuted","onRequestFinished-promiseResolved"], expected [] 18:12:59 INFO - Stack trace: 18:12:59 INFO - chrome://mochikit/content/browser-test.js:test_is:1271 18:12:59 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:27 18:12:59 INFO - chrome://mochikit/content/browser-test.js:Tester.prototype.nextTest<:666 18:12:59 INFO - testScope/test_finish/<@chrome://mochikit/content/browser-test.js:1363:11 18:12:59 INFO - run@chrome://mochikit/content/browser-test.js:1300:9
Flags: needinfo?(odvarko)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
The test should be fixed now, relanding... Honza
Flags: needinfo?(odvarko)
Comment 17•7 years ago
|
||
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/73628e4869b3 Refactor HAR builder options; r=ochameau https://hg.mozilla.org/integration/autoland/rev/470c96bdde51 Improve performance of HAR export; r=ochameau
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/73628e4869b3 https://hg.mozilla.org/mozilla-central/rev/470c96bdde51
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•