predictors: Add prefetching hit/miss histograms.

chrome/browser/predictors/resource_prefetch_predictor.cc

Index: chrome/browser/predictors/resource_prefetch_predictor.cc
diff --git a/chrome/browser/predictors/resource_prefetch_predictor.cc b/chrome/browser/predictors/resource_prefetch_predictor.cc
index 67dd25aec05b3071e51f0285cd1e853f4d21cc69..28bd9ad23a427436bc9016407ad01c791b93a7d1 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor.cc
@@ -120,6 +120,56 @@ void GetUrlVisitCountTask::DoneRunOnMainThread() {
 
 GetUrlVisitCountTask::~GetUrlVisitCountTask() {}
 
+void ReportPrefetchAccuracy(
+    const ResourcePrefetcher::PrefetcherStats& stats,
+    const std::vector<ResourcePrefetchPredictor::URLRequestSummary>&
+        summaries) {
+  if (stats.requests_stats.empty())
+    return;
+
+  std::set<GURL> urls;
+  for (const auto& summary : summaries)
+    urls.insert(summary.resource_url);
+
+  int cached_misses_count = 0;
+  int not_cached_misses_count = 0;
+  int cached_hits_count = 0;
+  int not_cached_hits_count = 0;
+  size_t misses_bytes = 0;
+  size_t hits_bytes = 0;
+
+  for (const auto& request_stats : stats.requests_stats) {
+    bool hit = urls.find(request_stats.resource_url) != urls.end();
+    bool cached = request_stats.was_cached;
+    size_t bytes = request_stats.total_received_bytes;
+
+    cached_hits_count += cached && hit;
+    cached_misses_count += cached && !hit;
+    not_cached_hits_count += !cached && hit;
+    not_cached_misses_count += !cached && !hit;
+    misses_bytes += !hit * bytes;
+    hits_bytes += hit * bytes;
+  }
+
+  UMA_HISTOGRAM_COUNTS_100(
+      internal::kResourcePrefetchPredictorPrefetchMissesCountCached,
+      cached_misses_count);
+  UMA_HISTOGRAM_COUNTS_100(
+      internal::kResourcePrefetchPredictorPrefetchMissesCountNotCached,
+      not_cached_misses_count);
+  UMA_HISTOGRAM_COUNTS_100(
+      internal::kResourcePrefetchPredictorPrefetchHitsCountCached,
+      cached_hits_count);
+  UMA_HISTOGRAM_COUNTS_100(
+      internal::kResourcePrefetchPredictorPrefetchHitsCountNotCached,
+      not_cached_hits_count);
+  UMA_HISTOGRAM_COUNTS_10000(
+      internal::kResourcePrefetchPredictorPrefetchHitsSize, hits_bytes / 1024);
+  UMA_HISTOGRAM_COUNTS_10000(
+      internal::kResourcePrefetchPredictorPrefetchMissesSize,
+      misses_bytes / 1024);
+}
+
 void ReportPredictionAccuracy(
     const std::vector<GURL>& predicted_urls,
     const ResourcePrefetchPredictor::PageRequestSummary& summary) {
@@ -534,9 +584,12 @@ void ResourcePrefetchPredictor::StopPrefetching(const GURL& url) {
 }
 
 void ResourcePrefetchPredictor::OnPrefetchingFinished(
-    const GURL& main_frame_url) {
+    const GURL& main_frame_url,
+    const ResourcePrefetcher::PrefetcherStats& stats) {
   if (observer_)
-    observer_->OnPrefetchingFinished(main_frame_url);
+    observer_->OnPrefetchingFinished(main_frame_url, stats);

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

You don't use stats inside an observer. Why?

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

Done.

+
+  prefetcher_stats_.insert({main_frame_url, stats});
 }
 
 bool ResourcePrefetchPredictor::IsUrlPrefetchable(const GURL& main_frame_url) {
@@ -642,12 +695,33 @@ void ResourcePrefetchPredictor::OnNavigationComplete(
   std::unique_ptr<PageRequestSummary> summary = std::move(nav_it->second);
   inflight_navigations_.erase(nav_it);
 
+  const GURL& main_frame_url = nav_id_without_timing_info.main_frame_url;
   std::vector<GURL> predicted_urls;
-  bool has_data = GetPrefetchData(nav_id_without_timing_info.main_frame_url,
-                                  &predicted_urls);
+  bool has_data = GetPrefetchData(main_frame_url, &predicted_urls);
   if (has_data)
     ReportPredictionAccuracy(predicted_urls, *summary);
 
+  auto it = prefetcher_stats_.find(main_frame_url);

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

I suspect that prefetching can end after navigation complete in rare cases. Do
you think it is ok?

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

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?

[alexilin, 2017/02/13 17:07:05]

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

+  if (it != prefetcher_stats_.end()) {
+    const std::vector<URLRequestSummary>& summaries =
+        summary->subresource_requests;
+    ReportPrefetchAccuracy(it->second, summaries);
+    prefetcher_stats_.erase(it);
+  }
+
+  // Remove old prefetches that haven't been claimed.

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

Could you move this into CleanupAbandonedNavigations()? I also added cleanup
(timeout-based) code to this function in my CL.

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

Done.

+  base::TimeTicks now = base::TimeTicks::Now();
+  it = prefetcher_stats_.begin();
+  while (it != prefetcher_stats_.end()) {
+    if (now - it->second.start_time > base::TimeDelta::FromMinutes(5)) {
+      // No requests -> everything is a miss.
+      ReportPrefetchAccuracy(it->second, std::vector<URLRequestSummary>());
+      it = prefetcher_stats_.erase(it);
+    } else {
+      ++it;
+    }
+  }
+
   // Kick off history lookup to determine if we should record the URL.
   history::HistoryService* history_service =
       HistoryServiceFactory::GetForProfile(profile_,