1127552 - Add Telemetry to fetch

Status: RESOLVED FIXED

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

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

add to FormDataParser usage to check how often big copies are made.

Comment 2

[Andrew Overholt [:overholt]]

What sort of data would you like to collect (other than what you mention in comment 1)?

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 5

[Andrew Overholt [:overholt]]

I'm going to work on this.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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!

Comment 7

[Andrew Overholt [:overholt]]

Attachment: simple first patch

Here's a super-simple first patch just to get it off my laptop.

Comment 8

[Andrew Overholt [:overholt]]

Attachment: simple first patch #2

bkelly mentioned a nit about the probe name and the description so I fixed it.

Comment 9

[Andrew Overholt [:overholt]]

Attachment: fetch-telemetry.patch

I'm working on tests but wanted to get your thoughts before doing too much more.  Thanks!

Comment 10

[Andrew Overholt [:overholt]]

Attachment: Patch 1 - Fetch comes from mainthread or not.

This is the same as it was a month ago.

Comment 11

[Andrew Overholt [:overholt]]

Attachment: Patch 2 - Note Fetches that just respond with same Request they intercepted

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.

Comment 12

[Andrew Overholt [:overholt]]

(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 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 15

[Andrew Overholt [:overholt]]

Attachment: Patch 2 v2 - Note Fetches that just respond with same Request they intercepted

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.

Comment 16

[Andrew Overholt [:overholt]]

Attachment: Patch 1 v2 - Fetch comes from mainthread or not.

(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.

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.'

Comment 18

[Andrew Overholt [:overholt]]

Attachment: Rollup for committing

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8126fbd73f26

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8126fbd73f26