Closed
Bug 1127552
Opened 8 years ago
Closed 8 years ago
Add Telemetry to fetch
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: nsm, Assigned: overholt)
References
Details
Attachments
(1 file, 7 obsolete files)
7.40 KB,
patch
|
overholt
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•8 years ago
|
||
add to FormDataParser usage to check how often big copies are made.
Assignee | ||
Comment 2•8 years ago
|
||
What sort of data would you like to collect (other than what you mention in comment 1)?
Flags: needinfo?(nsm.nikhil)
Reporter | ||
Comment 3•8 years ago
|
||
It would be good to know the context from which fetch is called (window or worker). Simple counter. Just lets us know the usage. Something useful to know would be how often a Request fetched from a SW is just the same Request that was received from a window/worker due to an intercepted fetch, vs how many fetches() are custom request/responses. If a large fraction of consumers are just fwding the intercepted request to fetch() with no modifications, we could look at bypassing the whole worker<->main thread<-> worker messaging when we know that is going to happen. The easiest way to implement this would be to have Request/Responses have a boolean field (bool mCreatedByChrome?), which does not get copied by Request::Constructor. FetchDriver() can then submit a probe about if the flag existed or not. Ben, do you think this will be useful for cache too?
Flags: needinfo?(nsm.nikhil) → needinfo?(bkelly)
Comment 4•8 years ago
|
||
Its very similar to something I plan to add in the future. I want to add a mCacheBodyId to InternalRequest and InternalResponse. If these are set to valid values, then I can detect a body that was retrieved from Cache and now getting put back into Cache. This lets me then avoid any data copying at all. I can just update the DB to point to the existing file. So I think this sounds good. Can we do it on Internal(Request|Response), though?
Flags: needinfo?(bkelly)
Reporter | ||
Comment 6•8 years ago
|
||
Bug 1109751 adds a FormDataParser that copies possibly big strings. There is a comment in there (search for Telemetry), which would be nice to have as a followup. Thanks!
Assignee | ||
Comment 7•8 years ago
|
||
Here's a super-simple first patch just to get it off my laptop.
Assignee | ||
Comment 8•8 years ago
|
||
bkelly mentioned a nit about the probe name and the description so I fixed it.
Assignee | ||
Comment 9•8 years ago
|
||
I'm working on tests but wanted to get your thoughts before doing too much more. Thanks!
Attachment #8568550 -
Attachment is obsolete: true
Attachment #8568565 -
Attachment is obsolete: true
Attachment #8570118 -
Flags: feedback?(nsm.nikhil)
Reporter | ||
Updated•8 years ago
|
Attachment #8570118 -
Flags: feedback?(nsm.nikhil) → feedback+
Assignee | ||
Comment 10•8 years ago
|
||
This is the same as it was a month ago.
Attachment #8570118 -
Attachment is obsolete: true
Attachment #8580872 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 11•8 years ago
|
||
I think it's correct but I'm seeing some odd results so want to get your opinion first. http://people.mozilla.org/~aoverholt/dump/ is a decent test. Bug 1143090 means we can't have a mochitest for this.
Attachment #8580874 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Andrew Overholt [:overholt] from comment #10) > Created attachment 8580872 [details] [diff] [review] > Patch 1 - Fetch comes from mainthread or not. I have been using http://addyosmani.com/demos/fetch-api/ as a simple testcase. Bug 1143090 needs to be fixed before we can have a mochitest for this telemetry.
Reporter | ||
Comment 13•8 years ago
|
||
Comment on attachment 8580872 [details] [diff] [review] Patch 1 - Fetch comes from mainthread or not. Review of attachment 8580872 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following change. ::: dom/fetch/Fetch.cpp @@ +247,4 @@ > WorkerPrivate* worker = GetCurrentThreadWorkerPrivate(); > MOZ_ASSERT(worker); > > + Telemetry::Accumulate(Telemetry::FETCH_IS_MAINTHREAD, 0); I don't think it is ok to trigger telemetry off main thread. This should go in MainThreadFetchRunnable::Run after the |if (!mResolver) { return; }| bail out conditional.
Attachment #8580872 -
Flags: review?(nsm.nikhil) → review+
Reporter | ||
Comment 14•8 years ago
|
||
Comment on attachment 8580874 [details] [diff] [review] Patch 2 - Note Fetches that just respond with same Request they intercepted Review of attachment 8580874 [details] [diff] [review]: ----------------------------------------------------------------- A better way would be to call the flag mCreatedByFetchEvent, and set it on the Request created in FetchEventRunnable DispatchFetchEvent(). Then in FetchDriver: Telemetry::Accumulate(PASSTHROUGH, mRequest->WasCreatedByFetchEvent()); ::: dom/fetch/InternalRequest.h @@ +323,4 @@ > bool mSynchronous; > bool mUnsafeRequest; > bool mUseURLCredentials; > + bool mCreatedByServiceWorker = false; Please add a comment about what this tracks, specifically that this is set only when script creates a Request object.
Attachment #8580874 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 15•8 years ago
|
||
With some hand-holding from nsm I've got this working now. I copy mCreatedByFetchEvent in the copy constructor and clear it if the internalRequest object is being modified based on the Request parameters.
Attachment #8580874 -
Attachment is obsolete: true
Attachment #8584048 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 16•8 years ago
|
||
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #13) > ::: dom/fetch/Fetch.cpp > @@ +247,4 @@ > > WorkerPrivate* worker = GetCurrentThreadWorkerPrivate(); > > MOZ_ASSERT(worker); > > > > + Telemetry::Accumulate(Telemetry::FETCH_IS_MAINTHREAD, 0); > > I don't think it is ok to trigger telemetry off main thread. This should go > in MainThreadFetchRunnable::Run after the |if (!mResolver) { return; }| bail > out conditional. I think I got it but I'd feel more comfortable with another review from you on this change. Thanks.
Attachment #8580872 -
Attachment is obsolete: true
Attachment #8584063 -
Flags: review?(nsm.nikhil)
Reporter | ||
Updated•8 years ago
|
Attachment #8584063 -
Flags: review?(nsm.nikhil) → review+
Reporter | ||
Comment 17•8 years ago
|
||
Comment on attachment 8584048 [details] [diff] [review] Patch 2 v2 - Note Fetches that just respond with same Request they intercepted Review of attachment 8584048 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comment fixes and one IMPORTANT fix. In Request::Constructor, reset the flag inside the |if (aInit.mHeaders.WasPassed())| block too. That block will lead to headers on the Request being modified. ::: dom/fetch/InternalRequest.h @@ +335,4 @@ > bool mSynchronous; > bool mUnsafeRequest; > bool mUseURLCredentials; > + // This is only set when a Request object is created by script. We use it to created by a fetch event. @@ +335,5 @@ > bool mSynchronous; > bool mUnsafeRequest; > bool mUseURLCredentials; > + // This is only set when a Request object is created by script. We use it to > + // check if Service Workers are simply returning intercepted Request objects. Replace 'returning ...' with 'fetching intercepted Request objects without modifying them.'
Attachment #8584048 -
Flags: review?(nsm.nikhil) → review+
Assignee | ||
Comment 18•8 years ago
|
||
Carrying nsm's r+ in this rollup patch. Here's a try build which has some failures that are unrelated to my patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5f7ceeee2d5 Thanks.
Attachment #8584048 -
Attachment is obsolete: true
Attachment #8584063 -
Attachment is obsolete: true
Attachment #8584491 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 19•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8126fbd73f26
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8126fbd73f26
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•