Closed
Bug 1356957
Opened 8 years ago
Closed 7 years ago
[Performance] Call updateRequest once when updateRequest in netmonitor-controller
Categories
(DevTools :: Netmonitor, defect, P1)
DevTools
Netmonitor
Tracking
(firefox55 fixed)
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: gasolin, Assigned: gasolin)
References
(Blocks 1 open bug)
Details
(Whiteboard: [netmonitor])
Attachments
(1 file, 1 obsolete file)
As mentioned in bug 1350227 comment 3 by Ricky, updateRequest now call action.updateRequest 6 times, we can reduce the quest to 1 by merging data payload into one object and call updateRequest() once at the end of the function.
Comment hidden (mozreview-request) |
Comment 2•8 years ago
|
||
mozreview-review |
Comment on attachment 8858715 [details]
Bug 1356957 - call updateRequest once when update request in netmonitor-controller;
https://reviewboard.mozilla.org/r/130744/#review133290
Looks good! I see some issues please see mozreview comments.
::: devtools/client/netmonitor/src/netmonitor-controller.js:432
(Diff revision 1)
> )
> .then(() => window.emit(EVENTS.REQUEST_ADDED, id));
> },
>
> async updateRequest(id, data) {
> await this.actions.updateRequest(id, data, true);
This updateRequest could also be merged.
::: devtools/client/netmonitor/src/netmonitor-controller.js:441
(Diff revision 1)
> responseHeaders,
> requestCookies,
> requestHeaders,
> requestPostData,
> } = data;
> let request = getRequestById(window.gStore.getState(), id);
we can skip additional updateRequest if request is null.
::: devtools/client/netmonitor/src/netmonitor-controller.js:442
(Diff revision 1)
> requestCookies,
> requestHeaders,
> requestPostData,
> } = data;
> let request = getRequestById(window.gStore.getState(), id);
>
nit: remove empty line
::: devtools/client/netmonitor/src/netmonitor-controller.js:459
(Diff revision 1)
> - );
> }
> }
>
> - if (request && responseContent && responseContent.content) {
> - let { mimeType } = request;
> + let { mimeType } = request;
nit: move `let { mimeType } = request;` inside of below if statement.
Attachment #8858715 -
Flags: review?(rchien) → review-
Updated•8 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 55.3 - Apr 17
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [netmonitor]
Updated•8 years ago
|
Summary: call updateRequest once when update request in netmonitor-controller → [Performance] Call updateRequest once when updateRequest in netmonitor-controller
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8858715 [details]
Bug 1356957 - call updateRequest once when update request in netmonitor-controller;
https://reviewboard.mozilla.org/r/130744/#review133290
> nit: move `let { mimeType } = request;` inside of below if statement.
it's used to detect mimetype at the end and send the IMAGE_DISPLAYED event, but I think the event should be in request-list-column-file to reflect its meaning
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8858715 [details]
Bug 1356957 - call updateRequest once when update request in netmonitor-controller;
https://reviewboard.mozilla.org/r/130744/#review133584
I still see performance impact as we await every asynchronous calls in sequence but not parallel. Simply our current call path in updateRequest() will look like:
```
await fetchHeaders(requestHeaders, getLongString);
await fetchHeaders(responseHeaders, getLongString);
await getLongString(text);
await getLongString(text);
await getLongString(cookie.value),
await getLongString(cookie.value),
```
There are 6 await excludes `await this.actions.updateRequest` and they are running in sequence and wait until previous task is complete and then run the next task. I can leverage `await Promise.all([...])` to wait necessary async task at once and then invoke updateRequest() once as well.
::: devtools/client/netmonitor/src/netmonitor-controller.js:446
(Diff revision 2)
> - true,
> - );
> - }
> }
>
> if (request && responseContent && responseContent.content) {
`request` can be removed since `mineType` is able to be accessed from responseContent.content.mimeType, and therefore, above `if (!request) { ... } check` can be removed safely as well.
::: devtools/client/netmonitor/src/netmonitor-controller.js:452
(Diff revision 2)
> let { text, encoding } = responseContent.content;
> let response = await getLongString(text);
> - let payload = {};
>
> if (mimeType.includes("image/")) {
> - payload.responseContentDataUri = formDataURI(mimeType, encoding, response);
> + data.responseContentDataUri = formDataURI(mimeType, encoding, response);
nit:
Is there any eslint rule in m-c to avoid modifying method parameter variable? I remember someone disagrees this pattern, and maybe I'm wrong. But there is one, we can keep using `let payload` to store new data.
Attachment #8858715 -
Flags: review?(rchien) → review-
Comment hidden (mozreview-request) |
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8858715 [details]
Bug 1356957 - call updateRequest once when update request in netmonitor-controller;
https://reviewboard.mozilla.org/r/130744/#review133606
::: devtools/client/netmonitor/src/components/request-list-column-file.js:36
(Diff revision 3)
> + if (responseContentDataUri) {
> + window.emit(EVENTS.RESPONSE_IMAGE_THUMBNAIL_DISPLAYED);
> + }
nit: Please remove this.
EVENTS.RESPONSE_IMAGE_THUMBNAIL_DISPLAYED is unnecessary since we can use waitUntil to wait our UI change without polluting extra event into src codebase.
::: devtools/client/netmonitor/src/netmonitor-controller.js:540
(Diff revision 3)
> + }
> }
> }
> }
> +
> + await Promise.all([
Nice abstraction! Can we move these inner functions out of updateRequest()? There are so many inner methods is merely used inside updateRequest seems to be superfluous.
We can move them out and return a payload and then merge them into one single payload object for this.actions.updateRequest().
::: devtools/client/netmonitor/src/netmonitor-controller.js:541
(Diff revision 3)
> + questImage(),
> + questRequestHeaders(),
> + questResponseHeaders(),
> + questPostData(),
> + questRequestCookies(),
> + questResponseCookies(),
nit: quest sounds weird to me. How bout fetchImage, fetchRequestHeaders...etc?
Attachment #8858715 -
Flags: review?(rchien) → review-
Updated•8 years ago
|
Flags: qe-verify? → qe-verify-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8858715 [details]
Bug 1356957 - call updateRequest once when update request in netmonitor-controller;
https://reviewboard.mozilla.org/r/130744/#review133606
I also fixed the simple_request-data.js which caused by `the events emit from netmonitor-controller does not wait after updateRequest is complete`.
> nit: Please remove this.
>
> EVENTS.RESPONSE_IMAGE_THUMBNAIL_DISPLAYED is unnecessary since we can use waitUntil to wait our UI change without polluting extra event into src codebase.
as we tested now I can use waitUntil to get element correctly. Removed unused events & fix related tests.
> Nice abstraction! Can we move these inner functions out of updateRequest()? There are so many inner methods is merely used inside updateRequest seems to be superfluous.
>
> We can move them out and return a payload and then merge them into one single payload object for this.actions.updateRequest().
Sure, to move inner methods out cause some extra function params passthrough & object assign overhead, but its doable.
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8858715 [details]
Bug 1356957 - call updateRequest once when update request in netmonitor-controller;
https://reviewboard.mozilla.org/r/130744/#review133668
Thanks! It's very close. I'm seeing there are some issues which opened in previous comments. Please remember to fix them.
::: devtools/client/netmonitor/src/netmonitor-controller.js:434
(Diff revision 5)
> - );
> - }
> - }
> -
> - if (request && responseContent && responseContent.content) {
> let { mimeType } = request;
We can access mimeType from responseContent.content. Please remove request so that we don't need to depend on it anymore.
::: devtools/client/netmonitor/src/netmonitor-controller.js:545
(Diff revision 5)
> - }));
> - }
> - if (resCookies.length) {
> + if (!request) {
> + await this.actions.updateRequest(id, data, true);
> + return;
> - await this.actions.updateRequest(id, { responseCookies: resCookies }, true);
> - }
> - }
> }
Remove this. request.mimeType is unsed.
::: devtools/client/netmonitor/test/browser_net_icon-preview.js:12
(Diff revision 5)
> * Tests if image responses show a thumbnail in the requests menu.
> */
>
> add_task(function* () {
> let { tab, monitor } = yield initNetMonitor(CONTENT_TYPE_WITHOUT_CACHE_URL);
> + const SELECTOR = ".requests-list-icon[data-type=thumbnail]";
nit: ".requests-list-icon[src]"; to detect thumbnail exists and then we can remove the additional data-type.
Attachment #8858715 -
Flags: review?(rchien) → review-
Updated•8 years ago
|
Iteration: 55.3 - Apr 17 → 55.4 - May 1
Assignee | ||
Comment 12•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8858715 [details]
Bug 1356957 - call updateRequest once when update request in netmonitor-controller;
https://reviewboard.mozilla.org/r/130744/#review133668
> We can access mimeType from responseContent.content. Please remove request so that we don't need to depend on it anymore.
nice catch!
Comment hidden (mozreview-request) |
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8858715 [details]
Bug 1356957 - call updateRequest once when update request in netmonitor-controller;
https://reviewboard.mozilla.org/r/130744/#review134152
Let's ship it. Thanks!
::: devtools/client/netmonitor/src/constants.js
(Diff revision 6)
> - // When the request post params are displayed in the UI.
> - REQUEST_POST_PARAMS_DISPLAYED: "NetMonitor:RequestPostParamsAvailable",
> -
> - // When the image response thumbnail is displayed in the UI.
> - RESPONSE_IMAGE_THUMBNAIL_DISPLAYED:
> - "NetMonitor:ResponseImageThumbnailAvailable",
> -
> - // Fired when charts have been displayed in the PerformanceStatisticsView.
> - PLACEHOLDER_CHARTS_DISPLAYED: "NetMonitor:PlaceholderChartsDisplayed",
> - PRIMED_CACHE_CHART_DISPLAYED: "NetMonitor:PrimedChartsDisplayed",
> - EMPTY_CACHE_CHART_DISPLAYED: "NetMonitor:EmptyChartsDisplayed",
> -
Nice cleanup. I like that!
Attachment #8858715 -
Flags: review?(rchien) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 15•8 years ago
|
||
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/948294d1e5f0
call updateRequest once when update request in netmonitor-controller;r=rickychien
Keywords: checkin-needed
Comment 16•8 years ago
|
||
Backed out for frequently failing devtools/client/netmonitor/test/browser_net_truncate.js with e10s:
https://hg.mozilla.org/integration/autoland/rev/8f54a749dcc288bf3413bcccd96a9b501f267b41
Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=948294d1e5f03a3bd09e958940b2624d7aa28541&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=92564288&repo=autoland
[task 2017-04-19T04:23:02.515804Z] 04:23:02 INFO - TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_truncate.js | The tooltip type is correct. - Got text/plain, expected text/plain; charset=utf-8
[task 2017-04-19T04:23:02.516856Z] 04:23:02 INFO - Stack trace:
[task 2017-04-19T04:23:02.519372Z] 04:23:02 INFO - chrome://mochikit/content/browser-test.js:test_is:928
[task 2017-04-19T04:23:02.520467Z] 04:23:02 INFO - chrome://mochitests/content/browser/devtools/client/netmonitor/test/head.js:verifyRequestItemTarget:437
[task 2017-04-19T04:23:02.521645Z] 04:23:02 INFO - chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_truncate.js:test/</<:37
[task 2017-04-19T04:23:02.522762Z] 04:23:02 INFO - resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:handler:138
[task 2017-04-19T04:23:02.523542Z] 04:23:02 INFO - resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:emit:194
[task 2017-04-19T04:23:02.524777Z] 04:23:02 INFO - resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/netmonitor/src/netmonitor-controller.js:_onResponseContent/<:736
[task 2017-04-19T04:23:02.526033Z] 04:23:02 INFO - Displayed transferred size:
[task 2017-04-19T04:23:02.527190Z] 04:23:02 INFO - Tooltip transferred size: null
Flags: needinfo?(gasolin)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•8 years ago
|
||
The regression is cause by we changed the retrieval of the mimeType in request item. I found we can save more updateRequest call by improving _onNetworkEventUpdate.
There was 84 update calls per request, new its 11 * 1
was: 12(_onNetworkEventUpdate) x 7 (updateRequest)
now: 11*1
we can reduce the number to 9 in the following patch
Flags: needinfo?(gasolin)
Assignee | ||
Updated•8 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•8 years ago
|
||
Check the detail of 2nd PR in Bug 1358054
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8859928 [details]
Bug 1356957 - combine double updateRequest call while receive event in _onNetworkEventUpdate;
https://reviewboard.mozilla.org/r/131990/#review134756
r+. Just with few nits. Thanks!
::: devtools/client/netmonitor/src/netmonitor-controller.js:686
(Diff revision 1)
> * Handles additional information received for a "securityInfo" packet.
> *
> * @param object response
> * The message received from the server.
> */
> - _onSecurityInfo: function (response) {
> + _onSecurityInfo: function (data, response) {
nit:
how about changing response to securityInfo, so that we can just write shortly `Object.assign({ securityInfo }, data);`
::: devtools/client/netmonitor/src/netmonitor-controller.js:740
(Diff revision 1)
> * Handles additional information received for a "eventTimings" packet.
> *
> * @param object response
> * The message received from the server.
> */
> - _onEventTimings: function (response) {
> + _onEventTimings: function (data, response) {
nit:
how about changing response to eventTimings, so that we can just write shortly `Object.assign({ eventTimings }, data);`
Attachment #8859928 -
Flags: review?(rchien) → review+
Assignee | ||
Comment 25•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8859928 [details]
Bug 1356957 - combine double updateRequest call while receive event in _onNetworkEventUpdate;
https://reviewboard.mozilla.org/r/131990/#review134756
> nit:
>
> how about changing response to securityInfo, so that we can just write shortly `Object.assign({ securityInfo }, data);`
I think the consistency for all those similar callback params is more valuable then the {} syntax sugar. It's more easy to accknowledge the `response.from` for all those calls.
I tend to keep the change compact so it might be easier for uplift.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 33•8 years ago
|
||
In comment 18 I made the wrong estimation. The actual state update before this patch is 18 per request (12 (_onNetworkEventUpdate) + 6 extra(updateRequest)). Now its 10 state update per request. (I backed out the onSecurityInfo enhancement which cause security_error test failure)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 35•8 years ago
|
||
the final version also fixed onSecurityInfo, so its 18 -> 9 state updates per request.
Keywords: checkin-needed
Comment 36•8 years ago
|
||
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/125045e56532
call updateRequest once when update request in netmonitor-controller;r=rickychien
https://hg.mozilla.org/integration/autoland/rev/f6a1f5acfd58
combine double updateRequest call while receive event in _onNetworkEventUpdate;r=rickychien
Keywords: checkin-needed
Comment 37•8 years ago
|
||
sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=93276575&repo=autoland
Flags: needinfo?(gasolin)
Comment 38•8 years ago
|
||
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/38732fc0f437
Backed out changeset f6a1f5acfd58
https://hg.mozilla.org/integration/autoland/rev/5e4195f34934
Backed out changeset 125045e56532 for frequent failures in browser_net_security-error.js
Assignee | ||
Comment 39•8 years ago
|
||
Hmm, the try test is green before land https://treeherder.mozilla.org/#/jobs?repo=try&revision=737cb4827d9e&selectedJob=93236679
I'll trigger another try again
Assignee | ||
Comment 40•7 years ago
|
||
Comment on attachment 8859928 [details]
Bug 1356957 - combine double updateRequest call while receive event in _onNetworkEventUpdate;
remove the PART II patch to further identify if it cause some unintented security test failure
Attachment #8859928 -
Attachment is obsolete: true
Flags: needinfo?(gasolin)
Assignee | ||
Comment 41•7 years ago
|
||
The test result looks good after remove the PART II patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3612363baf7
Will trigger test again to make sure it really passed.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 44•7 years ago
|
||
test green again https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3612363baf7
https://treeherder.mozilla.org/#/jobs?repo=try&revision=16061c027398
Keywords: checkin-needed
Comment 45•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.
hg error in cmd: hg rebase -s 90f916ccbc89 -d 62b649c6b314: rebasing 391342:90f916ccbc89 "Bug 1356957 - call updateRequest once when update request in netmonitor-controller;r=rickychien" (tip)
merging devtools/client/netmonitor/src/components/request-list-column-file.js
merging devtools/client/netmonitor/src/constants.js
merging devtools/client/netmonitor/test/browser_net_image-tooltip.js
warning: conflicts while merging devtools/client/netmonitor/src/components/request-list-column-file.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Updated•7 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment 47•7 years ago
|
||
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1163b1091f35
call updateRequest once when update request in netmonitor-controller;r=rickychien
Comment 48•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 49•7 years ago
|
||
as a note, this caused a slight damp regression on linux64:
== Change summary for alert #6324 (as of April 19 2017 15:00 UTC) ==
Regressions:
2% damp summary linux64 opt e10s 291.81 -> 297.71
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6324
this was so long ago, I didn't file a new regression template, I am happy to open a new bug if you wish.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•