Closed Bug 1358715 Opened 3 years ago Closed 3 years ago

[Performance] Queue NetworkEventUpdate in netmonitor controller

Categories

(DevTools :: Netmonitor, enhancement, P1)

enhancement

Tracking

(firefox55 fixed)

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

People

(Reporter: rickychien, Assigned: rickychien)

References

Details

(Whiteboard: [netmonitor])

Attachments

(1 file, 1 obsolete file)

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: 3 years ago
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.