Closed Bug 1358715 Opened 8 years ago Closed 8 years ago

[Performance] Queue NetworkEventUpdate in netmonitor controller

Categories

(DevTools :: Netmonitor, enhancement, P1)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.4 - May 1
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: rickychien, Assigned: rickychien)

References

Details

(Whiteboard: [netmonitor])

Attachments

(1 file, 1 obsolete file)

Bug 1358715 - Queue NetworkEventUpdate in netmonitor controller
59 bytes, text/x-review-board-request
Honza
: review+
Details
Once network request arrives will follow a series of event packets: 1. First, a "NetworkEvent" event message and invoke addRequest() in netmonitor-controller. 2. Afterward, the network request will follow up to 8 times "NetworkEventUpdate" event messages and then invoke at most 8 times updateRequest() in netmonitor-controller. These following possible packet updateTypes are requestHeaders, requestCookies, requestPostData, securityInfo, responseHeaders, responseCookies, responseStart, responseContent and eventTimings. The order of above NetworkEventUpdate is predictable, and the eventTimings is always the last one packet from "NetworkEventUpdate". As a result, we can queue these "NetworkEventUpdate" into an array and then just dispatch one updateRequest() to tell redux to refresh react component if the entire "NetworkEventUpdate" packets are ready. Bottom line is that after adopting this approach we are able to reduce updateRequest() calls from 8 times to 1 time and to gain performance win.
This patch depends on bug 1356957, please apply all patches in bug 1356957 first and then apply https://reviewboard.mozilla.org/r/132792/diff/1#index_header.
Comment on attachment 8860814 [details] Bug 1358715 - Queue NetworkEventUpdate in netmonitor controller https://reviewboard.mozilla.org/r/132792/#review135850 The patch looks good even if I am a bit worried about user experience. We should test that the UI feels responsive and is dynamically updated. I am also seeing two test failures on my machine. The following tests failed: 22580 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_simple-request-data.js | The attached totalTime is incorrect. - Got undefined, expected number Stack trace: chrome://mochikit/content/browser-test.js:test_is:928 chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_simple-request-data.js:test/</<:276 resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:handler:138 resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:emit:194 resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/netmonitor/src/netmonitor-controller.js:_onNetworkEventUpdate/<:663 22581 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_simple-request-data.js | The attached totalTime should be positive. - Stack trace: chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_simple-request-data.js:test/</<:278 resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:handler:138 resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:emit:194 resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/netmonitor/src/netmonitor-controller.js:_onNetworkEventUpdate/<:663 Buffered messages finished SUITE-END | took 240s Let's see what try says: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d7e5ce3afdb14172277645d89ec66e2e9425261 Honza
I cannot reproduce your failures on my machine and try said: it looks great!
Comment on attachment 8860814 [details] Bug 1358715 - Queue NetworkEventUpdate in netmonitor controller https://reviewboard.mozilla.org/r/132792/#review136016 ::: devtools/client/netmonitor/src/netmonitor-controller.js:591 (Diff revision 3) > postDataObj, requestCookiesObj, responseCookiesObj); > - await this.actions.updateRequest(id, payload, true); > + > + this.pushPayloadToQueue(id, payload); > + > + if (this.isQueuePayloadReady(id)) { > + await this.actions.updateRequest(id, this.getPayloadFromQueue(id), true); we should clear the cached payload by id here to free the memory
Attachment #8860815 - Attachment is obsolete: true
Attachment #8860815 - Flags: review?(odvarko)
Comment on attachment 8860814 [details] Bug 1358715 - Queue NetworkEventUpdate in netmonitor controller https://reviewboard.mozilla.org/r/132792/#review136038 Thanks for working on this Ricky! Honza
Attachment #8860814 - Flags: review?(odvarko) → review+
Summary: [Performance] Queue networkUpdateEvent in netmonitor controller → [Performance] Queue NetworkEventUpdate in netmonitor controller
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dd7f584c9dcb Queue NetworkEventUpdate in netmonitor controller r=Honza
https://hg.mozilla.org/mozilla-central/rev/dd7f584c9dcb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: qe-verify?
Blocks: 1362054
No longer blocks: 1362054
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: