All users were logged out of Bugzilla on October 13th, 2018

[Performance] Queue NetworkEventUpdate in netmonitor controller

RESOLVED FIXED in Firefox 55

Status

P1
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: rickychien, Assigned: rickychien)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 55
Dependency tree / graph
Bug Flags:
qe-verify ?

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [netmonitor])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
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 4

2 years ago
mozreview-review
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
(Assignee)

Comment 5

2 years ago
I cannot reproduce your failures on my machine and try said: it looks great!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

2 years ago
mozreview-review
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
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8860815 - Attachment is obsolete: true
Attachment #8860815 - Flags: review?(odvarko)

Comment 13

2 years ago
mozreview-review
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+
(Assignee)

Updated

2 years ago
Summary: [Performance] Queue networkUpdateEvent in netmonitor controller → [Performance] Queue NetworkEventUpdate in netmonitor controller
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

2 years ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd7f584c9dcb
Queue NetworkEventUpdate in netmonitor controller r=Honza

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd7f584c9dcb
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

2 years ago
Flags: qe-verify?
(Assignee)

Updated

2 years ago
Blocks: 1362054
(Assignee)

Updated

2 years ago
No longer blocks: 1362054

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.