Closed Bug 1127552 Opened 5 years ago Closed 5 years ago

Add Telemetry to fetch

Categories

(Core :: DOM: Core & HTML, defect)

33 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: nsm, Assigned: overholt)

References

Details

Attachments

(1 file, 7 obsolete files)

add to FormDataParser usage to check how often big copies are made.
What sort of data would you like to collect (other than what you mention in comment 1)?
Flags: needinfo?(nsm.nikhil)
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)
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)
I'm going to work on this.
Assignee: nobody → overholt
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!
Attached patch simple first patch (obsolete) — Splinter Review
Here's a super-simple first patch just to get it off my laptop.
Attached patch simple first patch #2 (obsolete) — Splinter Review
bkelly mentioned a nit about the probe name and the description so I fixed it.
Attached patch fetch-telemetry.patch (obsolete) — Splinter Review
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)
Attachment #8570118 - Flags: feedback?(nsm.nikhil) → feedback+
This is the same as it was a month ago.
Attachment #8570118 - Attachment is obsolete: true
Attachment #8580872 - Flags: review?(nsm.nikhil)
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)
(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.
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+
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)
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)
(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)
Attachment #8584063 - Flags: review?(nsm.nikhil) → review+
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+
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8126fbd73f26
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.