Add UMA to track time between preload discovery and reference

Add UMA to track time between preload discovery and reference.

Reference in this case is whenever the preloaded resource has
a ResourceClient added to it. This yields an upper bound on
the wins of the preload scanner, by tracking the time between
scanner discovery and DOM discovery.

BUG=603274
Committed: https://crrev.com/bfd430ec1aa83a4a667945453913bc51ab4fc580
Cr-Commit-Position: refs/heads/master@{#396999}

[Charlie Harrison, 2016-05-17 23:48:29]

csharrison@chromium.org changed reviewers:
+ yoav@yoav.ws

[Charlie Harrison, 2016-05-17 23:48:30]

Yoav, can you take a look at this? I'm trying to establish an upper bound on the
time saved preloading a resource. Are there cases this will miss?

[Yoav Weiss, 2016-05-25 12:43:20]

On 2016/05/17 23:48:30, csharrison wrote:
> Yoav, can you take a look at this? I'm trying to establish an upper bound on
the
> time saved preloading a resource. Are there cases this will miss?

Sorry for the late reply... :/ Please feel free to ping me in the future.

It's not clear to me what this patch does in the case of <link rel=preload>.
LinkLoader adds a LinkPreloadResourceClient to each preloaded resource (in order
to fire their onload/onerror events), so I believe
Resource::willAddClientOrObserver is called at that point.

So I believe we'd be measuring the time between preloadScanner discovery and DOM
discovery for these resources. I'm not sure if that's the desired outcome.

[Charlie Harrison, 2016-05-25 13:18:56]

On 2016/05/25 12:43:20, Yoav Weiss wrote:
> On 2016/05/17 23:48:30, csharrison wrote:
> > Yoav, can you take a look at this? I'm trying to establish an upper bound on
> the
> > time saved preloading a resource. Are there cases this will miss?
> 
> Sorry for the late reply... :/ Please feel free to ping me in the future.
Sounds good. No worries this was a pretty low priority CL.
> 
> It's not clear to me what this patch does in the case of <link rel=preload>.
> LinkLoader adds a LinkPreloadResourceClient to each preloaded resource (in
order
> to fire their onload/onerror events), so I believe
> Resource::willAddClientOrObserver is called at that point.
> 
> So I believe we'd be measuring the time between preloadScanner discovery and
DOM
> discovery for these resources. I'm not sure if that's the desired outcome.
That is the desired outcome. By tracking the difference (for all resources)
between
DOM discovery and preload discovery, we can assess the performance impact of the
preload
scanner, because that's the upper bound of the latency saved by the feature. I
can update
the CL description with this explanation.

I am also planning on sending out a follow up CL that tracks the time from
preload discovery
to first byte received on the network. This would assess a plan where we
preconnect discovered resources on the bg parser thread.

[Charlie Harrison, 2016-05-25 13:18:58]

On 2016/05/25 12:43:20, Yoav Weiss wrote:
> On 2016/05/17 23:48:30, csharrison wrote:
> > Yoav, can you take a look at this? I'm trying to establish an upper bound on
> the
> > time saved preloading a resource. Are there cases this will miss?
> 
> Sorry for the late reply... :/ Please feel free to ping me in the future.
Sounds good. No worries this was a pretty low priority CL.
> 
> It's not clear to me what this patch does in the case of <link rel=preload>.
> LinkLoader adds a LinkPreloadResourceClient to each preloaded resource (in
order
> to fire their onload/onerror events), so I believe
> Resource::willAddClientOrObserver is called at that point.
> 
> So I believe we'd be measuring the time between preloadScanner discovery and
DOM
> discovery for these resources. I'm not sure if that's the desired outcome.
That is the desired outcome. By tracking the difference (for all resources)
between
DOM discovery and preload discovery, we can assess the performance impact of the
preload
scanner, because that's the upper bound of the latency saved by the feature. I
can update
the CL description with this explanation.

I am also planning on sending out a follow up CL that tracks the time from
preload discovery
to first byte received on the network. This would assess a plan where we
preconnect discovered resources on the bg parser thread.

[Charlie Harrison, 2016-05-31 17:35:46]

yoav@ ping. Let me know if my reasoning sounds okay.

[Yoav Weiss, 2016-05-31 18:36:18]

Your reasoning makes sense: this is measuring the PreloadScanner's benefits, not
user added <link rel=preload>. As long as that's clear LGTM :D

https://codereview.chromium.org/1987013002/diff/2/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/FetchRequest.h (right):

https://codereview.chromium.org/1987013002/diff/2/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/FetchRequest.h:88: void setForPreload(bool
forPreload, double discoveryTime = 0)
maybe move the implementation to the cpp file, as it's now more than a single
line? I don't think we have a strict rule for that, and if you really like to
keep it here, I can live with it, but I think it's better to keep the h files
lean and mean :)

[Charlie Harrison, 2016-05-31 18:59:34]

Description was changed from

==========
Add UMA to track time between preload discovery and reference

BUG=603274
==========

to

==========
Add UMA to track time between preload discovery and reference.

Reference in this case is whenever the preloaded resource has
a ResourceClient added to it. This yields an upper bound on
the wins of the preload scanner, by tracking the time between
scanner discovery and DOM discovery.

BUG=603274
==========

[Charlie Harrison, 2016-05-31 19:06:05]

csharrison@chromium.org changed reviewers:
+ holte@chromium.org

[Charlie Harrison, 2016-05-31 19:06:06]

Thanks!
+holte for histograms.xml

https://codereview.chromium.org/1987013002/diff/2/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/FetchRequest.h (right):

https://codereview.chromium.org/1987013002/diff/2/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/FetchRequest.h:88: void setForPreload(bool
forPreload, double discoveryTime = 0)
On 2016/05/31 18:36:18, Yoav Weiss wrote:
> maybe move the implementation to the cpp file, as it's now more than a single
> line? I don't think we have a strict rule for that, and if you really like to
> keep it here, I can live with it, but I think it's better to keep the h files
> lean and mean :)

Done.

[Steven Holte, 2016-05-31 22:08:54]

lgtm

[Charlie Harrison, 2016-05-31 22:09:31]

Thank you!

[Charlie Harrison, 2016-05-31 22:09:35]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-05-31 22:09:36]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/1987013002/#ps50001
(title: "update histograms.xml to be a bit clearer")

[commit-bot: I haz the power, 2016-05-31 22:10:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987013002/50001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987013002/50001

[commit-bot: I haz the power, 2016-06-01 00:51:01]

Description was changed from

==========
Add UMA to track time between preload discovery and reference.

Reference in this case is whenever the preloaded resource has
a ResourceClient added to it. This yields an upper bound on
the wins of the preload scanner, by tracking the time between
scanner discovery and DOM discovery.

BUG=603274
==========

to

==========
Add UMA to track time between preload discovery and reference.

Reference in this case is whenever the preloaded resource has
a ResourceClient added to it. This yields an upper bound on
the wins of the preload scanner, by tracking the time between
scanner discovery and DOM discovery.

BUG=603274
==========

[commit-bot: I haz the power, 2016-06-01 00:51:03]

Committed patchset #4 (id:50001)

[commit-bot: I haz the power, 2016-06-01 00:53:13]

Description was changed from

==========
Add UMA to track time between preload discovery and reference.

Reference in this case is whenever the preloaded resource has
a ResourceClient added to it. This yields an upper bound on
the wins of the preload scanner, by tracking the time between
scanner discovery and DOM discovery.

BUG=603274
==========

to

==========
Add UMA to track time between preload discovery and reference.

Reference in this case is whenever the preloaded resource has
a ResourceClient added to it. This yields an upper bound on
the wins of the preload scanner, by tracking the time between
scanner discovery and DOM discovery.

BUG=603274

Committed: https://crrev.com/bfd430ec1aa83a4a667945453913bc51ab4fc580
Cr-Commit-Position: refs/heads/master@{#396999}
==========

[commit-bot: I haz the power, 2016-06-01 00:53:15]

Patchset 4 (id:??) landed as
https://crrev.com/bfd430ec1aa83a4a667945453913bc51ab4fc580
Cr-Commit-Position: refs/heads/master@{#396999}