Add PLT measurement to Resource Prefetching for Mobile Web

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 ea767daf69fe59792c6ebd121297aaacaf393aa4..f86ee1c6d9c551c257b072c6f50811db8986ae8f 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor.cc
@@ -34,6 +34,7 @@
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/mime_util.h"
+#include "net/base/network_change_notifier.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -90,6 +91,49 @@ void RecordNavigationEvent(NavigationEvent event) {
                             NAVIGATION_EVENT_COUNT);
 }
 
+enum AdditionalConnectionType {
+  CONNECTION_CELLULAR = net::NetworkChangeNotifier::CONNECTION_LAST + 1,

[Alexei Svitkine (slow), 2014/10/20 18:30:35]

What happens if net::NetworkChangeNotifier::CONNECTION_LAST is updated? Now,
your histogram bucket will overlap with another bucket depending on the version
of Chrome.

I don't think you should do this. Either use a separate histogram for these, or
instead use values that can't appear in net::NetworkChangeNotifier after updates
- for example use a sparse histogram and use negative placeholders -1, -2, etc.
(You need to use a sparse histogram if you want to use negative values.)

[Zhen Wang, 2014/10/27 14:12:47]

Good point. Using UMA_HISTOGRAM_SPARSE_SLOWLY now.

Should I worry about this "_SLOWLY" thing? How slower is it?

[Alexei Svitkine (slow), 2014/10/27 14:59:09]

It involves a lock on the histogram object and a std::map operation. So slower
than updating an entry in an array, as is the case with regular histograms, but
not generally still quite fast.

+  CONNECTION_ALL = net::NetworkChangeNotifier::CONNECTION_LAST + 2,
+  CONNECTION_COUNT = net::NetworkChangeNotifier::CONNECTION_LAST + 3
+};
+
+std::string GetNetTypeStr() {
+  switch (net::NetworkChangeNotifier::GetConnectionType()) {
+    case net::NetworkChangeNotifier::CONNECTION_ETHERNET:
+      return "_Ethernet";
+    case net::NetworkChangeNotifier::CONNECTION_WIFI:
+      return "_WiFi";
+    case net::NetworkChangeNotifier::CONNECTION_2G:
+      return "_2G";
+    case net::NetworkChangeNotifier::CONNECTION_3G:
+      return "_3G";
+    case net::NetworkChangeNotifier::CONNECTION_4G:
+      return "_4G";
+    case net::NetworkChangeNotifier::CONNECTION_NONE:
+      return "_None";
+    case net::NetworkChangeNotifier::CONNECTION_BLUETOOTH:
+      return "_Bluetooth";
+    case net::NetworkChangeNotifier::CONNECTION_UNKNOWN:
+    default:
+      break;
+  }
+  return "_Unknown";
+}
+
+void ReportPagePrefetchedNetworkType(int type) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "ResourcePrefetchPredictor.PagePrefetchedNetworkType",

[Alexei Svitkine (slow), 2014/10/20 18:30:35]

I suggest naming this one and the one below to have the same prefix, so you get
grouping on the dashboard.

How about:

ResourcePrefetchPredictor.NetworkType.PagePrefetched
ResourcePrefetchPredictor.NetworkType.PageNotPrefetched

[Zhen Wang, 2014/10/27 14:12:47]

We need to put pages prefetched in different network types into one group and
put pages not prefetched in different network types into another group. This is
to match with ResourcePrefetchPredictor.PLT.Prefetched.{NetType}.

So I think current naming should be easiest to see on dashboard.

[Alexei Svitkine (slow), 2014/10/27 14:59:09]

My suggestion is not contrary to that. I'm just suggesting using different names
for the two histograms you're logging here.

i.e.

ResourcePrefetchPredictor.PagePrefetchedNetworkType ->
ResourcePrefetchPredictor.NetworkType.PagePrefetched

ResourcePrefetchPredictor.PageNotPrefetchedNetworkType ->
ResourcePrefetchPredictor.NetworkType.PageNotPrefetched

The idea is both are a specialization of the
"ResourcePrefetchPredictor.NetworkType" base histogram - one specialization for
"PagePrefetched" and another for "PageNotPrefetched", but otherwise logging the
same type of thing. Then, you can access both histograms by typing
ResourcePrefetchPredictor.NetworkType.* on the dashboard.

[Zhen Wang, 2014/10/27 18:09:46]

I see. Thanks for the clarification! Using new naming now.

+      type,
+      CONNECTION_COUNT);
+}
+
+void ReportPageNotPrefetchedNetworkType(int type) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "ResourcePrefetchPredictor.PageNotPrefetchedNetworkType",
+      type,
+      CONNECTION_COUNT);
+}
+
 }  // namespace
 
 namespace predictors {
@@ -525,6 +569,8 @@ void ResourcePrefetchPredictor::OnNavigationComplete(
   RecordNavigationEvent(NAVIGATION_EVENT_ONLOAD_TRACKED_URL);
 
   // Report any stats.
+  base::TimeDelta plt = base::TimeTicks::Now() - navigation_id.creation_time;
+  ReportPageLoadTimeStats(plt);
   if (prefetch_manager_.get()) {
     ResultsMap::iterator results_it = results_map_.find(navigation_id);
     bool have_prefetch_results = results_it != results_map_.end();
@@ -534,6 +580,9 @@ void ResourcePrefetchPredictor::OnNavigationComplete(
       ReportAccuracyStats(results_it->second->key_type,
                           *(nav_it->second),
                           results_it->second->requests.get());
+      ReportPageLoadTimePrefetchedStats(results_it->second->key_type, plt);
+    } else {
+      ReportPageLoadTimeNotPrefetchedStats(plt);
     }
   } else {
     scoped_ptr<ResourcePrefetcher::RequestVector> requests(
@@ -1003,7 +1052,92 @@ void ResourcePrefetchPredictor::LearnNavigation(
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-// Accuracy measurement.
+// Page load time and accuracy measurement.
+
+// This is essentially UMA_HISTOGRAM_MEDIUM_TIMES, but it avoids using the
+// STATIC_HISTOGRAM_POINTER_BLOCK in UMA_HISTOGRAM definitions.
+#define RPP_HISTOGRAM_MEDIUM_TIMES(name, page_load_time) \
+{ \

[Alexei Svitkine (slow), 2014/10/20 18:30:35]

Nit: Make this a do { } while (0) loop.

Also, indent everything below the macro 4 more.

[Zhen Wang, 2014/10/27 14:12:46]

Done.

By the way, why do we prefer the style of "do { } while (0) loop" for macros?

+  base::HistogramBase* histogram = base::Histogram::FactoryTimeGet( \
+      name, \
+      base::TimeDelta::FromMilliseconds(10), \
+      base::TimeDelta::FromMinutes(3), \
+      50, \
+      base::HistogramBase::kUmaTargetedHistogramFlag); \
+  histogram->AddTime(page_load_time); \
+}
+
+void ResourcePrefetchPredictor::ReportPageLoadTimeStats(
+    base::TimeDelta plt) const {
+  net::NetworkChangeNotifier::ConnectionType connection_type =
+      net::NetworkChangeNotifier::GetConnectionType();
+
+  RPP_HISTOGRAM_MEDIUM_TIMES("ResourcePrefetchPredictor.PLT", plt);
+  RPP_HISTOGRAM_MEDIUM_TIMES(
+      "ResourcePrefetchPredictor.PLT" + GetNetTypeStr(), plt);
+  if (net::NetworkChangeNotifier::IsConnectionCellular(connection_type))
+    RPP_HISTOGRAM_MEDIUM_TIMES("ResourcePrefetchPredictor.PLT_Cellular", plt);
+}
+
+void ResourcePrefetchPredictor::ReportPageLoadTimePrefetchedStats(
+    PrefetchKeyType key_type,
+    base::TimeDelta plt) const {
+  net::NetworkChangeNotifier::ConnectionType connection_type =
+      net::NetworkChangeNotifier::GetConnectionType();
+  bool on_cellular =
+      net::NetworkChangeNotifier::IsConnectionCellular(connection_type);
+
+  ReportPagePrefetchedNetworkType(CONNECTION_ALL);
+  ReportPagePrefetchedNetworkType(
+      net::NetworkChangeNotifier::GetConnectionType());
+  if (on_cellular)
+    ReportPagePrefetchedNetworkType(CONNECTION_CELLULAR);
+
+  RPP_HISTOGRAM_MEDIUM_TIMES(
+      "ResourcePrefetchPredictor.PLT.PagePrefetched", plt);
+  RPP_HISTOGRAM_MEDIUM_TIMES(
+      "ResourcePrefetchPredictor.PLT.PagePrefetched"+ GetNetTypeStr(), plt);
+  if (on_cellular) {
+    RPP_HISTOGRAM_MEDIUM_TIMES(
+        "ResourcePrefetchPredictor.PLT.PagePrefetched_Cellular", plt);
+  }
+
+  std::string type =
+      key_type == PREFETCH_KEY_TYPE_HOST ? "Host" : "Url";
+  RPP_HISTOGRAM_MEDIUM_TIMES(
+      "ResourcePrefetchPredictor.PLT.PagePrefetched." + type, plt);
+  RPP_HISTOGRAM_MEDIUM_TIMES(
+      "ResourcePrefetchPredictor.PLT.PagePrefetched." + type + GetNetTypeStr(),
+      plt);
+  if (on_cellular) {
+    RPP_HISTOGRAM_MEDIUM_TIMES(
+        "ResourcePrefetchPredictor.PLT.PagePrefetched." + type + "_Cellular",
+        plt);
+  }
+}
+
+void ResourcePrefetchPredictor::ReportPageLoadTimeNotPrefetchedStats(

[Alexei Svitkine (slow), 2014/10/20 18:30:35]

It seems the logic of this function mirrors pretty closely what's done in
ReportPageLoadTimePrefetchedStats() except for PagePrefetched vs.
PageNotPrefetched.

Can you make a helper function?

I imagine it would need to take this following params:
  const std::string& prefetched_str,  (either PagePrefetched or
PageNotPrefetched)
  base::Callback<void(int)> record_network_type_callback,
     (either ReportPageNotPrefetchedNetworkType or
ReportPagePrefetchedNetworkType).

One other suggestion: maybe in all of these histograms, make the middle string
shorter - i.e. instead of PagePrefetched/PageNotPrefetched, use simply
Prefetched/NotPrefetched.

[Zhen Wang, 2014/10/27 14:12:46]

Done.

+    base::TimeDelta plt) const {
+  net::NetworkChangeNotifier::ConnectionType connection_type =
+      net::NetworkChangeNotifier::GetConnectionType();
+  bool on_cellular =
+      net::NetworkChangeNotifier::IsConnectionCellular(connection_type);
+
+  ReportPageNotPrefetchedNetworkType(CONNECTION_ALL);
+  ReportPageNotPrefetchedNetworkType(
+      net::NetworkChangeNotifier::GetConnectionType());

[Alexei Svitkine (slow), 2014/10/20 18:30:35]

Nit: Use connection_type which you have as a variable above.

[Zhen Wang, 2014/10/27 14:12:46]

Done.

+  if (on_cellular)
+    ReportPageNotPrefetchedNetworkType(CONNECTION_CELLULAR);
+
+  RPP_HISTOGRAM_MEDIUM_TIMES(
+      "ResourcePrefetchPredictor.PLT.PageNotPrefetched", plt);
+  RPP_HISTOGRAM_MEDIUM_TIMES(
+      "ResourcePrefetchPredictor.PLT.PageNotPrefetched" + GetNetTypeStr(), plt);

[Alexei Svitkine (slow), 2014/10/20 18:30:35]

Nit: I'd find it clearer if you make the "_" be part of this string at the end,
rather than returned by GetNetTypeStr().

[Zhen Wang, 2014/10/27 14:12:46]

Done.

+  if (on_cellular) {
+    RPP_HISTOGRAM_MEDIUM_TIMES(
+        "ResourcePrefetchPredictor.PLT.PageNotPrefetched_Cellular", plt);
+  }
+}
 
 void ResourcePrefetchPredictor::ReportAccuracyStats(
     PrefetchKeyType key_type,
@@ -1217,6 +1351,7 @@ void ResourcePrefetchPredictor::ReportPredictedAccuracyStatsHelper(
         prefetch_network * 100.0 / total_resources_fetched_from_network);
   }
 
+#undef RPP_HISTOGRAM_MEDIUM_TIMES
 #undef RPP_PREDICTED_HISTOGRAM_PERCENTAGE
 #undef RPP_PREDICTED_HISTOGRAM_COUNTS
 }