Closed Bug 1356957 Opened 7 years ago Closed 7 years ago

[Performance] Call updateRequest once when updateRequest in netmonitor-controller

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox55 fixed)

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

People

(Reporter: gasolin, Assigned: gasolin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file, 1 obsolete file)

Bug 1356957 - call updateRequest once when update request in netmonitor-controller;
59 bytes, text/x-review-board-request
rickychien
: review+
Details 
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 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-
Status: NEW → ASSIGNED
Iteration: --- → 55.3 - Apr 17
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [netmonitor]
Summary: call updateRequest once when update request in netmonitor-controller → [Performance] Call updateRequest once when updateRequest in netmonitor-controller
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 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 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-
Flags: qe-verify? → qe-verify-
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 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-
Iteration: 55.3 - Apr 17 → 55.4 - May 1
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 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+
Keywords: checkin-needed
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
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)
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)
See Also: → 1358054
Check the detail of 2nd PR in Bug 1358054
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+
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.
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)
the final version also fixed onSecurityInfo, so its 18 -> 9 state updates per request.
Keywords: checkin-needed
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
sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=93276575&repo=autoland
Flags: needinfo?(gasolin)
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
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
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)
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.
Blocks: 1358054
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)
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1163b1091f35
call updateRequest once when update request in netmonitor-controller;r=rickychien
https://hg.mozilla.org/mozilla-central/rev/1163b1091f35
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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.
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: