Closed
Bug 1418928
Opened 7 years ago
Closed 7 years ago
requestCookies and responseCookies should be loaded lazily
Categories
(DevTools :: Netmonitor, enhancement, P2)
DevTools
Netmonitor
Tracking
(firefox59 fixed)
RESOLVED
FIXED
Firefox 59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: rickychien, Assigned: rickychien)
References
Details
Attachments
(1 file)
According to https://bugzilla.mozilla.org/show_bug.cgi?id=1404913#c9,
Lazy load requestCookies and responseCookies looks promising for improving netmonitor perf.
Assignee | ||
Updated•7 years ago
|
Summary: Request Post DATA should be loaded lazily → requestCookies and responseCookies should be loaded lazily
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8931194 [details]
Bug 1418928 - requestCookies and responseCookies should be loaded lazily
https://reviewboard.mozilla.org/r/202284/#review208052
::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:387
(Diff revision 2)
> break;
> + case "requestCookies":
> + case "responseCookies":
> case "requestPostData":
> - this.updateRequest(actor, {
> - // This field helps knowing when/if requestPostData property is available
> + // This field helps knowing when/if requestPostData property is available
nit: now this is not only for the requestPostData
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
This patch needs to apply on top of bug 1404928.
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8931194 [details]
Bug 1418928 - requestCookies and responseCookies should be loaded lazily
https://reviewboard.mozilla.org/r/202284/#review208140
Looks nice and works for me.
R+ assuming my inline comments are resolved and try is green.
Thanks Ricky!
Honza
::: devtools/client/netmonitor/src/components/CookiesPanel.js:42
(Diff revision 5)
> + };
> + }
> +
> + componentDidMount() {
> + this.maybeFetchRequestCookies(this.props);
> + this.maybeFetchResponseCookies(this.props);
merge these two methods into one: `maybeFetchCookies`
::: devtools/client/netmonitor/src/components/RequestListColumnCookies.js:40
(Diff revision 5)
> }
>
> + /**
> + * When switching to another request, lazily fetch request cookies
> + * from the backend. The panel will first be empty and then display the content.
> + */
The comment should be updated
::: devtools/client/netmonitor/src/components/RequestListColumnSetCookies.js:40
(Diff revision 5)
> }
>
> + /**
> + * When switching to another request, lazily fetch response cookies
> + * from the backend. The panel will first be empty and then display the content.
> + */
The comment should be updated
Attachment #8931194 -
Flags: review?(odvarko) → review+
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8931194 [details]
Bug 1418928 - requestCookies and responseCookies should be loaded lazily
https://reviewboard.mozilla.org/r/202284/#review208428
Sorry to interfere once again, but this patch is breaking filtering by response cookies:
https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/devtools/client/netmonitor/src/utils/filter-autocomplete-provider.js#44-53
We briefly discussed about that in netmonitor meeting, but didn't come to a conclusion.
We should either strip this filtering feature or force fetching all response cookies when user uses these cookies filters.
::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:232
(Diff revision 6)
> payload.requestCookies = reqCookies;
> }
> }
> +
> + // Lock down requestCookiesAvailable once we fetch data from back-end.
> + payload.requestCookiesAvailable = false;
Why do you do that? Is it really used?
Why only for cookies and not for responseContent, securityInfo nor requestPostData?
Attachment #8931194 -
Flags: review-
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
A quick comment about what I said regarding waiting for async events.
Today, we are using `waitForNetworkEvents` almost everywhere:
https://searchfox.org/mozilla-central/search?q=waitForNetworkEvents&case=false®exp=false&path=
If we are introducing async requests related to cookies, we should do the same,
and wait for these cookies requests in each action that dispatch some.
We should not only wait for them in teardown.
Waiting for them only in teardown will most likely make netmonitor test prone to intermittents.
Because these cookies async requests may collide. Two distinct actions may happen to run at the same time by overlapping.
This is as bad as two tests overlapping.
So I would suggest to tune waitForNetworkEvents to accept an argument specific to cookies,
or introduce another helper specific to cookies requests.
This helper would wait for EVENTS.RECEIVED_REQUEST_COOKIES.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
Alex,
I'm going to introduce a helper waitForLazyLoadEvents() in head.js for addressing lazy fetching payload in mochitest [1]. All failed tests will need to insert this helper between the place which will cause async events. I'd like to ask your opinions first before going to fix the rest of failed tests.
[1] https://hg.mozilla.org/try/diff/420ccb4bcf99/devtools/client/netmonitor/test/head.js#l1.112
[2] https://hg.mozilla.org/try/diff/420ccb4bcf99/devtools/client/netmonitor/test/browser_net_column_headers_tooltips.js#l1.12
Comment 16•7 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #15)
> Alex,
>
> I'm going to introduce a helper waitForLazyLoadEvents() in head.js for
> addressing lazy fetching payload in mochitest [1]. All failed tests will
> need to insert this helper between the place which will cause async events.
> I'd like to ask your opinions first before going to fix the rest of failed
> tests.
>
>
> [1]
> https://hg.mozilla.org/try/diff/420ccb4bcf99/devtools/client/netmonitor/test/
> head.js#l1.112
> [2]
> https://hg.mozilla.org/try/diff/420ccb4bcf99/devtools/client/netmonitor/test/
> browser_net_column_headers_tooltips.js#l1.12
It looks good overall, with that, tests should be solid!
May be the helper can be slightly more complex to help listening at multiple events at once.
With something like this:
let waitForEvents = waitForLazyLoadEvents(monitor, [ EVENTS.RECEIVED_REQUEST_COOKIES, EVENTS.RECEIVED_RESPONSE_COOKIES ], 1);
...
yield waitForEvents;
Also, so do not forget to call panel.off when you are done listening.
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8931194 [details]
Bug 1418928 - requestCookies and responseCookies should be loaded lazily
https://reviewboard.mozilla.org/r/202284/#review209250
::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:183
(Diff revision 7)
> }, 0);
>
> requestPostData.postData.text = postData;
> payload.requestPostData = Object.assign({}, requestPostData);
> payload.requestHeadersFromUploadStream = { headers, headersSize };
> }
Should we add `payload.fetchPostDataAvailable = false;` here for consistency?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8931194 [details]
Bug 1418928 - requestCookies and responseCookies should be loaded lazily
https://reviewboard.mozilla.org/r/202284/#review209250
> Should we add `payload.fetchPostDataAvailable = false;` here for consistency?
this flag mechanism has removed, see above comment.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8931194 [details]
Bug 1418928 - requestCookies and responseCookies should be loaded lazily
https://reviewboard.mozilla.org/r/202284/#review208428
> Why do you do that? Is it really used?
> Why only for cookies and not for responseContent, securityInfo nor requestPostData?
Every time a props change will trigger maybeFetchRequestCookies() [1] and try fetching data via requestData(). In order to prevent fetching existed data again, I set `requestCookiesAvailable = false` as a flag to skip redundant data fetching.
After investigating, I'm going to remove the code [2] to avoid unnecessary data fetching. Hopefully that makes sense to you.
[1] https://hg.mozilla.org/try/rev/9509a99bafa15eeee65bd6e85dfeda5da222ab52#l2.22
[2] https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/devtools/client/netmonitor/src/connector/firefox-data-provider.js#511-513
Assignee | ||
Comment 23•7 years ago
|
||
Some test cases might start fetching network update payloads while test page is loading. It's hard for catching all request events by using onResponseConent similar approach to wait for specific events. I'm going to introduce a lazy events collector when a test start [1] to capture all lazily fetched events, and then wait for all captured events at restartNetMonitor [2] & teardown [3],
Surprisingly, it works as expected! It means the robust harness can detect and catch all lazy events to avoid connection exceptions.
[1] https://hg.mozilla.org/try/diff/4d8b495c822f/devtools/client/netmonitor/test/head.js#l1.31
[2] https://hg.mozilla.org/try/diff/4d8b495c822f/devtools/client/netmonitor/test/head.js#l1.57
[3] https://hg.mozilla.org/try/diff/4d8b495c822f/devtools/client/netmonitor/test/head.js#l1.78
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #22)
> Comment on attachment 8931194 [details]
> Bug 1418928 - requestCookies and responseCookies should be loaded lazily
>
> https://reviewboard.mozilla.org/r/202284/#review208428
>
> > Why do you do that? Is it really used?
> > Why only for cookies and not for responseContent, securityInfo nor requestPostData?
>
> Every time a props change will trigger maybeFetchRequestCookies() [1] and
> try fetching data via requestData(). In order to prevent fetching existed
> data again, I set `requestCookiesAvailable = false` as a flag to skip
> redundant data fetching.
>
> After investigating, I'm going to remove the code [2] to avoid unnecessary
> data fetching. Hopefully that makes sense to you.
>
> [1]
> https://hg.mozilla.org/try/rev/9509a99bafa15eeee65bd6e85dfeda5da222ab52#l2.22
>
> [2]
> https://searchfox.org/mozilla-central/rev/
> be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/devtools/client/netmonitor/src/
> connector/firefox-data-provider.js#511-513
You can't remove `this.lazyRequestData.delete(key, promise);` from [2],
otherwise you are going to leak a lot of data as this Map will never be freed!
May be you need to move the delete *after* the `updateRequest` call.
What should happen is that, either there is a pending promise in `lazyRequestData`
and so we should not dispatch duplicated RDP messages -or- request.requestCookies will be defined,
so that maybeFetchRequestCookies won't call connector.requestData anymore.
Comment 26•7 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #23)
> Some test cases might start fetching network update payloads while test page
> is loading. It's hard for catching all request events by using
> onResponseConent similar approach to wait for specific events.
This is not new. Many test use a "performRequests" function to wait before dispatching requests and use waitForRequests around the performRequests call.
> I'm going to introduce a lazy events collector when a test start [1] to capture all
> lazily fetched events, and then wait for all captured events at
> restartNetMonitor [2] & teardown [3],
Your helper is fine. Hooking into initNetmonitor and using promises like that is clever.
The only issue is about calling it on restartNetmonitor/teardown.
Because the issue I'm highlighting in comment 12 remains:
> We should not only wait for them in teardown.
> Waiting for them only in teardown will most likely make netmonitor test prone to intermittents.
> Because these cookies async requests may collide. Two distinct actions may happen to run at the same time by overlapping.
> This is as bad as two tests overlapping.
So you should put `yield Promise.all(lazyLoadEvents);` in tests, after each action that introduces async requests.
(You probably want to call an helper instead of `yield Promise.all(lazyLoadEvents);`)
Exactly like our current usage of waitForRequests.
This is why I suggested to do something around waitForRequests, but may be it is easier to introduce another distinct helper.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #25)
> You can't remove `this.lazyRequestData.delete(key, promise);` from [2],
> otherwise you are going to leak a lot of data as this Map will never be
> freed!
Right. This might happen. we should prevent eating up memory in this case.
> May be you need to move the delete *after* the `updateRequest` call.
> What should happen is that, either there is a pending promise in
> `lazyRequestData`
> and so we should not dispatch duplicated RDP messages -or-
> request.requestCookies will be defined,
> so that maybeFetchRequestCookies won't call connector.requestData anymore.
Moving it after updateRequest call doesn't work because requestData could be called in every component changes in high frequency even before updateRequest is done. As a result, I'm going back to use flag approach to ensure we won't get unnecessary data fetching.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
Fixed autocomplete issues from comment 10 and lazyRequestData from comment 25.
For issue of comment 26, we had a conversion in IRC and understand that for the issue of async requests may collide (see comment 12), we can use existing wait code approach like [1] or waitUntil() to wait for specific react render state. There's no conflicts in using both approaches simultaneously. With using this event collector, we can guarantee that all lazy network request events will be finished properly before teardown. (currently I just watch request cookies and response cookies).
Let's wait for try green...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=610e6b1af5761a8d9d14ac0f705ae5429c4cba94
[1] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_brotli.js#54
Comment hidden (mozreview-request) |
Assignee | ||
Comment 32•7 years ago
|
||
Update patch for fixing browser_net_copy_params.js failure.
Assignee | ||
Comment 33•7 years ago
|
||
Wait for try green
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee1c5b5d9204
Assignee | ||
Comment 34•7 years ago
|
||
Try is green now and damp result looks nice as well!
complicated.netmonitor.requestsFinished.DAMP -5.65% 19.23 (high)
simple.netmonitor.requestsFinished.DAMP -20.98% 5.20 (high)
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=61159b0c4c44675ae3749593b5acf8b4e3f2c436&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=86400
Comment 35•7 years ago
|
||
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee15ca302afd
requestCookies and responseCookies should be loaded lazily r=Honza
Comment 36•7 years ago
|
||
Backed out for devtools failures on browser_net_cause_redirect.js
https://hg.mozilla.org/integration/autoland/rev/ace59c114b1df0a2b2876d03129abdff29c57ddd
https://treeherder.mozilla.org/logviewer.html#?job_id=148963052&repo=autoland&lineNumber=8666
Flags: needinfo?(rchien)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 40•7 years ago
|
||
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8dca851ba43
requestCookies and responseCookies should be loaded lazily r=Honza
Comment 41•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•