Log histograms for preloads and preload misses

Log histograms for preloads and preload misses

This patch logs preloads and preload misses when the ResourceFetcher
clears its preloads. It also removes the debug preload macros which seem
unused, given that their implementation doesn't even compile.

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

[Charlie Harrison, 2016-05-13 19:57:13]

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

[Charlie Harrison, 2016-05-13 19:57:14]

kouhei@, can you please review this? I think these metrics are critical to
evaluating the accuracy of the preload scanner.

I'm planning on following up with data on the time delta between preload request
and preload referenced.

https://codereview.chromium.org/1973073005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (left):

https://codereview.chromium.org/1973073005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1069: if (!m_preloads)
Remove this in place of an ASSERT.

[kouhei (in TOK), 2016-05-16 14:42:30]

lgtm

https://codereview.chromium.org/1973073005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1973073005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1145:
cssPreloadMisses.count(stylesheetMisses);
Please use consistent variable names here. Either css or stylesheet.

[Yoav Weiss, 2016-05-16 15:07:42]

On 2016/05/16 14:42:30, kouhei wrote:
> lgtm
> 
>
https://codereview.chromium.org/1973073005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/1973073005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1145:
> cssPreloadMisses.count(stylesheetMisses);
> Please use consistent variable names here. Either css or stylesheet.

Just a note (which doesn't necessarily impact this change):
clearPreloads is called at a fairly early stage, before some <link rel=preload>
based resources were properly reused.
For example, if the developer wants to preload a resource that will be used
after DOMContentLoaded (so after clearPreloads), it will be reported as a miss,
even if it isn't one.

[Charlie Harrison, 2016-05-16 15:18:21]

On 2016/05/16 15:07:42, Yoav Weiss wrote:
> On 2016/05/16 14:42:30, kouhei wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/1973073005/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1973073005/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1145:
> > cssPreloadMisses.count(stylesheetMisses);
> > Please use consistent variable names here. Either css or stylesheet.
> 
> Just a note (which doesn't necessarily impact this change):
> clearPreloads is called at a fairly early stage, before some <link
rel=preload>
> based resources were properly reused.
> For example, if the developer wants to preload a resource that will be used
> after DOMContentLoaded (so after clearPreloads), it will be reported as a
miss,
> even if it isn't one.

Thanks for the note. How often do you think this occurs? I'll make a note in
histograms.xml that RawResource preloads may have skewed miss rate (I believe
that's what all link rel="preload" requests turn into).

[Charlie Harrison, 2016-05-16 19:15:07]

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

[Charlie Harrison, 2016-05-16 19:15:07]

Thanks all.
asvitkine@ PTAL. Note that I'm using someone elses suffix list.

[Alexei Svitkine (slow), 2016-05-17 17:28:58]

lgtm

[Charlie Harrison, 2016-05-17 17:33:49]

Thanks!
yoav@, I'll land this now but let's circle back if we end up getting bad data
for link rel preloads. Maybe with weak memory cache we can move this logic
there?

[Charlie Harrison, 2016-05-17 17:34:18]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-05-17 17:34:18]

The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/1973073005/#ps60001
(title: "kouhei@ review + updated histograms per yoav@")

[commit-bot: I haz the power, 2016-05-17 17:34:39]

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

[commit-bot: I haz the power, 2016-05-17 21:25:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-17 21:27:16]

Description was changed from

==========
Log histograms for preloads and preload misses

This patch logs preloads and preload misses when the ResourceFetcher
clears its preloads. It also removes the debug preload macros which seem
unused, given that their implementation doesn't even compile.

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

to

==========
Log histograms for preloads and preload misses

This patch logs preloads and preload misses when the ResourceFetcher
clears its preloads. It also removes the debug preload macros which seem
unused, given that their implementation doesn't even compile.

BUG=603274

Committed: https://crrev.com/37853844afa06038c94e5ce6d8b75d1db9faf2a6
Cr-Commit-Position: refs/heads/master@{#394234}
==========

[commit-bot: I haz the power, 2016-05-17 21:27:17]

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

[Yoav Weiss, 2016-05-18 07:55:42]

On 2016/05/17 17:33:49, csharrison wrote:
> Thanks!
> yoav@, I'll land this now but let's circle back if we end up getting bad data
> for link rel preloads. Maybe with weak memory cache we can move this logic
> there?

A weak memcache absed counting would make much more sense (and the skew for link
preload resources is not limited to RawResource: `as` initiates these requests
with their destination ResourceType

[Charlie Harrison, 2016-05-18 14:00:20]

On 2016/05/18 07:55:42, Yoav Weiss wrote:
> On 2016/05/17 17:33:49, csharrison wrote:
> > Thanks!
> > yoav@, I'll land this now but let's circle back if we end up getting bad
data
> > for link rel preloads. Maybe with weak memory cache we can move this logic
> > there?
> 
> A weak memcache absed counting would make much more sense (and the skew for
link
> preload resources is not limited to RawResource: `as` initiates these requests
> with their destination ResourceType

What if we just put this logging code in Resource's pre-finalizer? That could
work today I think. I'll test it out.

[Charlie Harrison, 2016-05-18 15:28:32]

On 2016/05/18 14:00:20, csharrison wrote:
> On 2016/05/18 07:55:42, Yoav Weiss wrote:
> > On 2016/05/17 17:33:49, csharrison wrote:
> > > Thanks!
> > > yoav@, I'll land this now but let's circle back if we end up getting bad
> data
> > > for link rel preloads. Maybe with weak memory cache we can move this logic
> > > there?
> > 
> > A weak memcache absed counting would make much more sense (and the skew for
> link
> > preload resources is not limited to RawResource: `as` initiates these
requests
> > with their destination ResourceType
> 
> What if we just put this logging code in Resource's pre-finalizer? That could
> work today I think. I'll test it out.

Of course, we'd lack the aggregation to do a CustomCountHistogram, but maybe
that resolution isn't needed. WDYT?