predictors: Add prefetcher histograms for speculative prefetch.

predictors: Add prefetcher histograms for speculative prefetch.

Prefetching is most useful when a prefetched resource isn't in cache or needs 
to be revalidated. This CL adds a histogram that allows us to estimate how 
often the predictor prefetches resources from the network and how often from 
the cache. This CL also adds histograms for a single prefetcher instance: 
count of prefeched resources and amount of consumed KB.

BUG=680049
Review-Url: https://codereview.chromium.org/2654913004
Cr-Commit-Position: refs/heads/master@{#446309}
Committed: https://chromium.googlesource.com/chromium/src/+/075c5d682d0b00fb8e311a23990082567c8b98e3

[alexilin, 2017-01-25 12:38:56]

alexilin@chromium.org changed reviewers:
+ lizeb@chromium.org

[alexilin, 2017-01-25 12:38:56]

Please review this change, thanks!

[alexilin, 2017-01-25 17:20:41]

Description was changed from

==========
predictors: Add prefetcher histograms for speculative prefetch.

Prefetching is the most useful when prefetched resource isn't in cache or need
to be revalidated. This CL adds histogram that allows us to estimate how often
predictor prefetches resources from the network and how often from the cache.
This CL also adds histograms for a single prefetcher instance: count of
prefeched resources and amount of consumed KB.

BUG=680049
==========

to

==========
predictors: Add prefetcher histograms for speculative prefetch.

Prefetching is most useful when a prefetched resource isn't in cache or needs 
to be revalidated. This CL adds a histogram that allows us to estimate how 
often the predictor prefetches resources from the network and how often from 
the cache. This CL also adds histograms for a single prefetcher instance: 
count of prefeched resources and amount of consumed KB.

BUG=680049
==========

[Benoit L, 2017-01-25 17:22:35]

lgtm, thanks!

https://codereview.chromium.org/2654913004/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher.h (right):

https://codereview.chromium.org/2654913004/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.h:125: int64_t prefetched_count_;
why not int?

https://codereview.chromium.org/2654913004/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2654913004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:54085: +    prefetch. Logged per
request.
nit: Logged once per successful request.

https://codereview.chromium.org/2654913004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:54521: +    The count of successfully
prefetched subresources for a single page.
nit: Single prefetch request?

[alexilin, 2017-01-25 17:47:32]

alexilin@chromium.org changed reviewers:
+ isherman@chromium.org

[alexilin, 2017-01-25 17:47:33]

isherman: Please review histograms.xml. Thanks!

https://codereview.chromium.org/2654913004/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher.h (right):

https://codereview.chromium.org/2654913004/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.h:125: int64_t prefetched_count_;
On 2017/01/25 17:22:34, Benoit L wrote:
> why not int?

Because of return type of URLRequest::GetTotalReceivedBytes(). Do you think it
is ok to cast it to int?

https://codereview.chromium.org/2654913004/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2654913004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:54085: +    prefetch. Logged per
request.
On 2017/01/25 17:22:34, Benoit L wrote:
> nit: Logged once per successful request.

Right. Done.

https://codereview.chromium.org/2654913004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:54521: +    The count of successfully
prefetched subresources for a single page.
On 2017/01/25 17:22:35, Benoit L wrote:
> nit: Single prefetch request?

Won't it be confused with URL request that prefetcher initiates? 
I think it would be better to stick to a prefetcher instance either a page.

[Ilya Sherman, 2017-01-25 18:20:09]

Metrics LGTM % comments:

https://codereview.chromium.org/2654913004/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2654913004/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:54518: +<histogram
name="ResourcePrefetchPredictor.PrefetchedCount">
nit: Please add a units attribute.

https://codereview.chromium.org/2654913004/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:54521: +    The count of successfully
prefetched subresources for a single page.
Please document when this is recorded -- only after all subresources have
reached a terminal success/failure state?

[alexilin, 2017-01-26 08:41:38]

https://codereview.chromium.org/2654913004/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2654913004/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:54518: +<histogram
name="ResourcePrefetchPredictor.PrefetchedCount">
On 2017/01/25 18:20:09, Ilya Sherman wrote:
> nit: Please add a units attribute.

Done.

https://codereview.chromium.org/2654913004/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:54521: +    The count of successfully
prefetched subresources for a single page.
On 2017/01/25 18:20:09, Ilya Sherman wrote:
> Please document when this is recorded -- only after all subresources have
> reached a terminal success/failure state?

Done.

[alexilin, 2017-01-26 08:41:42]

The CQ bit was checked by alexilin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 08:42:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 09:36:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 09:36:37]

Dry run: This issue passed the CQ dry run.

[alexilin, 2017-01-26 09:38:59]

https://codereview.chromium.org/2654913004/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher.h (right):

https://codereview.chromium.org/2654913004/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.h:125: int64_t prefetched_count_;
On 2017/01/25 17:47:33, alexilin wrote:
> On 2017/01/25 17:22:34, Benoit L wrote:
> > why not int?
> 
> Because of return type of URLRequest::GetTotalReceivedBytes(). Do you think it
> is ok to cast it to int?

My bad, I mixed up types for these two fields.
My intent was to use size_t for count and int64_t for bytes.

[alexilin, 2017-01-26 09:39:05]

The CQ bit was checked by alexilin@chromium.org

[alexilin, 2017-01-26 09:39:06]

The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2654913004/#ps80001
(title: "Fix types.")

[commit-bot: I haz the power, 2017-01-26 09:39:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 11:53:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 11:53:14]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alexilin, 2017-01-26 11:55:23]

The CQ bit was checked by alexilin@chromium.org

[commit-bot: I haz the power, 2017-01-26 11:55:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 12:44:15]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485431723852480,
"parent_rev": "5c98db656aa52625137af840e098bbce26db7010", "commit_rev":
"075c5d682d0b00fb8e311a23990082567c8b98e3"}

[commit-bot: I haz the power, 2017-01-26 12:44:51]

Description was changed from

==========
predictors: Add prefetcher histograms for speculative prefetch.

Prefetching is most useful when a prefetched resource isn't in cache or needs 
to be revalidated. This CL adds a histogram that allows us to estimate how 
often the predictor prefetches resources from the network and how often from 
the cache. This CL also adds histograms for a single prefetcher instance: 
count of prefeched resources and amount of consumed KB.

BUG=680049
==========

to

==========
predictors: Add prefetcher histograms for speculative prefetch.

Prefetching is most useful when a prefetched resource isn't in cache or needs 
to be revalidated. This CL adds a histogram that allows us to estimate how 
often the predictor prefetches resources from the network and how often from 
the cache. This CL also adds histograms for a single prefetcher instance: 
count of prefeched resources and amount of consumed KB.

BUG=680049

Review-Url: https://codereview.chromium.org/2654913004
Cr-Commit-Position: refs/heads/master@{#446309}
Committed:
https://chromium.googlesource.com/chromium/src/+/075c5d682d0b00fb8e311a239900...
==========

[commit-bot: I haz the power, 2017-01-26 12:44:52]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/075c5d682d0b00fb8e311a239900...