predictors: Add prefetching hit/miss histograms.

predictors: Add prefetching hit/miss histograms.

When the resource_prefetch_predictor triggers a prefetch, record how
many resources were used / not used (hits and misses), and the pattern
of these (in cache, or not in cache). For the resources that are not in
the cache, record the amount of data prefetched and wasted.

This also assumes that a prefetch older than 5 minutes is wasted.

BUG=680049
Review-Url: https://codereview.chromium.org/2688633002
Cr-Commit-Position: refs/heads/master@{#450935}
Committed: https://chromium.googlesource.com/chromium/src/+/2e4491eac73425489384c1428b17415e7dcec8e2

[Benoit L, 2017-02-09 02:20:33]

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

[commit-bot: I haz the power, 2017-02-09 02:20:57]

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-02-09 05:21:59]

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

[commit-bot: I haz the power, 2017-02-09 05:22:00]

Dry run: This issue passed the CQ dry run.

[alexilin, 2017-02-09 15:24:17]

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

[alexilin, 2017-02-09 15:24:18]

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:590:
observer_->OnPrefetchingFinished(main_frame_url, stats);
You don't use stats inside an observer. Why?

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:704: auto it =
prefetcher_stats_.find(main_frame_url);
I suspect that prefetching can end after navigation complete in rare cases. Do
you think it is ok?

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:712: // Remove old
prefetches that haven't been claimed.
Could you move this into CleanupAbandonedNavigations()? I also added cleanup
(timeout-based) code to this function in my CL.

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:594:
http_response->set_content(summary.content);
There already is http_response->set_content() call.
This one is unused, feel free to delete together with content field in
ResourceSummary.

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:608:
http_response->set_content(std::string(1024, static_cast<char>(42)));
It will cause additional parsing errors in tests. But whatever...

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:636:
std::unique_ptr<base::HistogramTester> histogram_tester_;
Could you put it at the end of (first) protected section?
https://google.github.io/styleguide/cppguide.html#Declaration_Order

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

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.cc:35:
total_received_bytes(total_received_bytes) {}  //
What is "//" for?

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.cc:196: size_t
total_received_bytes = request->GetTotalReceivedBytes();
request->GetTotalReceivedBytes() returns int64_t
I doubt that we will prefetch something more than 4GB but let's make this code
more consistent.
Please, change either prefetched_bytes type or type of this local variable.

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_manager.cc:90:
base::Unretained(predictor_), main_frame_url, stats));
Can we avoid copying of stats object?
I think, we could pass it as unique_ptr from prefetcher to this delegate first
and then to UI thread.

[Benoit L, 2017-02-13 16:13:15]

Thanks!

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:590:
observer_->OnPrefetchingFinished(main_frame_url, stats);
On 2017/02/09 15:24:17, alexilin wrote:
> You don't use stats inside an observer. Why?

Done.

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:704: auto it =
prefetcher_stats_.find(main_frame_url);
On 2017/02/09 15:24:17, alexilin wrote:
> I suspect that prefetching can end after navigation complete in rare cases. Do
> you think it is ok?

Yes, that's possible, but I don't think it's worth the complexity, as this would
happen for the case where a request which is a miss hangs, while the page loads.
wdyt?

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:712: // Remove old
prefetches that haven't been claimed.
On 2017/02/09 15:24:17, alexilin wrote:
> Could you move this into CleanupAbandonedNavigations()? I also added cleanup
> (timeout-based) code to this function in my CL.

Done.

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:594:
http_response->set_content(summary.content);
On 2017/02/09 15:24:17, alexilin wrote:
> There already is http_response->set_content() call.
> This one is unused, feel free to delete together with content field in
> ResourceSummary.

Oops... was myopic.
Done.

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:608:
http_response->set_content(std::string(1024, static_cast<char>(42)));
On 2017/02/09 15:24:17, alexilin wrote:
> It will cause additional parsing errors in tests. But whatever...

Changed to a space, which should be fine for the various text resources.
Done.

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:636:
std::unique_ptr<base::HistogramTester> histogram_tester_;
On 2017/02/09 15:24:18, alexilin wrote:
> Could you put it at the end of (first) protected section?
> https://google.github.io/styleguide/cppguide.html#Declaration_Order

Done.

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

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.cc:35:
total_received_bytes(total_received_bytes) {}  //
On 2017/02/09 15:24:18, alexilin wrote:
> What is "//" for?

Oops.
Done.

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.cc:196: size_t
total_received_bytes = request->GetTotalReceivedBytes();
On 2017/02/09 15:24:18, alexilin wrote:
> request->GetTotalReceivedBytes() returns int64_t
> I doubt that we will prefetch something more than 4GB but let's make this code
> more consistent.
> Please, change either prefetched_bytes type or type of this local variable.

Done.

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_manager.cc:90:
base::Unretained(predictor_), main_frame_url, stats));
On 2017/02/09 15:24:18, alexilin wrote:
> Can we avoid copying of stats object?
> I think, we could pass it as unique_ptr from prefetcher to this delegate first
> and then to UI thread.

It was initially because gmock doesn't like unique_ptr, but that's just
laziness. Thank you for not letting that pass.
Done.

[Benoit L, 2017-02-13 16:59:51]

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

[commit-bot: I haz the power, 2017-02-13 17:00:30]

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

[alexilin, 2017-02-13 17:07:06]

P.S. don't forget about histograms.xml

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:704: auto it =
prefetcher_stats_.find(main_frame_url);
On 2017/02/13 16:13:15, Benoit L wrote:
> On 2017/02/09 15:24:17, alexilin wrote:
> > I suspect that prefetching can end after navigation complete in rare cases.
Do
> > you think it is ok?
> 
> Yes, that's possible, but I don't think it's worth the complexity, as this
would
> happen for the case where a request which is a miss hangs, while the page
loads.
> wdyt?

Yes, I also think that this case isn't worth the additional complexity. But it
reminded me that we should be aware of size of subresources that we prefetch. 
I think we need to abort too big resources while prefetching. Resource size
written to the database could be wrong (and we don't even store it yet).

https://codereview.chromium.org/2688633002/diff/60001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2688633002/diff/60001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.h:305: //
inflight_navigations_ and inflight_prefetches_.
nit:
Need to clarify this comment. Add prefetcher_stats_ to the end of it or rewrite
it completely e.g.:
Removes data for navigations and prefetches that were abandoned. Will cleanup
internal data based on timeouts.

https://codereview.chromium.org/2688633002/diff/60001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher_unittest.cc (right):

https://codereview.chromium.org/2688633002/diff/60001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_unittest.cc:234: EXPECT_TRUE(
This test fails.

EXPECT_CALL introduces assert that will be checked after test execution, but
your assert must be true at the call time.

You could move assert after the last OnResponse() that actually calls Delegate
or you could check assertion in a destructor of TestResourcePrefetcherDelegate.

[commit-bot: I haz the power, 2017-02-13 17:27:11]

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

[commit-bot: I haz the power, 2017-02-13 17:27:13]

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

[Benoit L, 2017-02-13 17:32:51]

Thanks!

https://codereview.chromium.org/2688633002/diff/60001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2688633002/diff/60001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.h:305: //
inflight_navigations_ and inflight_prefetches_.
On 2017/02/13 17:07:05, alexilin wrote:
> nit:
> Need to clarify this comment. Add prefetcher_stats_ to the end of it or
rewrite
> it completely e.g.:
> Removes data for navigations and prefetches that were abandoned. Will cleanup
> internal data based on timeouts.

Done.

https://codereview.chromium.org/2688633002/diff/60001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher_unittest.cc (right):

https://codereview.chromium.org/2688633002/diff/60001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_unittest.cc:234: EXPECT_TRUE(
On 2017/02/13 17:07:05, alexilin wrote:
> This test fails.
> 
> EXPECT_CALL introduces assert that will be checked after test execution, but
> your assert must be true at the call time.
> 
> You could move assert after the last OnResponse() that actually calls Delegate
> or you could check assertion in a destructor of
TestResourcePrefetcherDelegate.

Done.

[alexilin, 2017-02-13 17:41:49]

lgtm, thank you!

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

https://codereview.chromium.org/2688633002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:55125: +<histogram
name="ResourcePrefetchPredictor.PrefetchHitsCount.Cached">
tiny nit:
specify units for count histograms also e.g
units="count" or units="urls"

[Benoit L, 2017-02-14 08:39:14]

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

[commit-bot: I haz the power, 2017-02-14 08:39:30]

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

[Benoit L, 2017-02-14 08:40:15]

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

[Benoit L, 2017-02-14 08:40:16]

isherman: Please review the histogram changes here, thanks!

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

https://codereview.chromium.org/2688633002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:55125: +<histogram
name="ResourcePrefetchPredictor.PrefetchHitsCount.Cached">
On 2017/02/13 17:41:49, alexilin wrote:
> tiny nit:
> specify units for count histograms also e.g
> units="count" or units="urls"

Done.

[commit-bot: I haz the power, 2017-02-14 09:38:57]

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

[commit-bot: I haz the power, 2017-02-14 09:38:58]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-02-15 00:07:41]

https://codereview.chromium.org/2688633002/diff/100001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2688633002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:167:
internal::kResourcePrefetchPredictorPrefetchHitsSize, hits_bytes / 1024);
This goes up to about 10MB, right?  Is that enough range for this histogram?

[Benoit L, 2017-02-15 09:55:34]

https://codereview.chromium.org/2688633002/diff/100001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2688633002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:167:
internal::kResourcePrefetchPredictorPrefetchHitsSize, hits_bytes / 1024);
On 2017/02/15 00:07:41, Ilya Sherman wrote:
> This goes up to about 10MB, right?  Is that enough range for this histogram?

I think so, yes. The amount of actual data fetched should be in the 100s of KB
range.

[Ilya Sherman, 2017-02-15 23:55:27]

Metrics LGTM, thanks.

[Benoit L, 2017-02-16 09:16:19]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2017-02-16 09:16:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexilin@chromium.org
Link to the patchset: https://codereview.chromium.org/2688633002/#ps100001
(title: "Add units.")

[commit-bot: I haz the power, 2017-02-16 09:17:06]

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-02-16 11:46:56]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487236579013740,
"parent_rev": "f1af3fdf3da1e6c1a9b60f747625c288de6f9451", "commit_rev":
"2e4491eac73425489384c1428b17415e7dcec8e2"}

[commit-bot: I haz the power, 2017-02-16 11:47:35]

Description was changed from

==========
predictors: Add prefetching hit/miss histograms.

When the resource_prefetch_predictor triggers a prefetch, record how
many resources were used / not used (hits and misses), and the pattern
of these (in cache, or not in cache). For the resources that are not in
the cache, record the amount of data prefetched and wasted.

This also assumes that a prefetch older than 5 minutes is wasted.

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

to

==========
predictors: Add prefetching hit/miss histograms.

When the resource_prefetch_predictor triggers a prefetch, record how
many resources were used / not used (hits and misses), and the pattern
of these (in cache, or not in cache). For the resources that are not in
the cache, record the amount of data prefetched and wasted.

This also assumes that a prefetch older than 5 minutes is wasted.

BUG=680049

Review-Url: https://codereview.chromium.org/2688633002
Cr-Commit-Position: refs/heads/master@{#450935}
Committed:
https://chromium.googlesource.com/chromium/src/+/2e4491eac73425489384c1428b17...
==========

[commit-bot: I haz the power, 2017-02-16 11:47:36]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/2e4491eac73425489384c1428b17...