Add in-memory caching of network quality estimates across network changes.

net/base/network_quality_estimator.cc

Index: net/base/network_quality_estimator.cc
diff --git a/net/base/network_quality_estimator.cc b/net/base/network_quality_estimator.cc
index e012a99d72fdcc752cf4e9406f8e61651315598a..f3e36c13f44019c62aa973b751d185355f91c097 100644
--- a/net/base/network_quality_estimator.cc
+++ b/net/base/network_quality_estimator.cc
@@ -4,14 +4,27 @@
 
 #include "net/base/network_quality_estimator.h"
 
-#include <string>
-
+#include "base/logging.h"
 #include "base/metrics/histogram.h"
+#include "build/build_config.h"
 #include "net/base/net_util.h"
 #include "net/base/network_quality.h"
 #include "net/url_request/url_request.h"
 #include "url/gurl.h"
 
+#if defined(OS_ANDROID)
+#include "net/android/network_library.h"
+#endif  // OS_ANDROID
+
+namespace {
+
+// Maximum size of the cache that holds network quality estimates.
+// Smaller size may reduce the cache hit rate due to frequent evictions.
+// Larger size may affect performance.
+const uint32_t kMaximumNetworkQualityCacheSize = 10;
+
+}  // namespace
+
 namespace net {
 
 NetworkQualityEstimator::NetworkQualityEstimator()
@@ -27,7 +40,10 @@ NetworkQualityEstimator::NetworkQualityEstimator(
       peak_kbps_since_last_connection_change_(0) {
   static_assert(kMinRequestDurationMicroseconds > 0,
                 "Minimum request duration must be > 0");
+  static_assert(kMaximumNetworkQualityCacheSize > 0,
+                "Size of the network quality cache must be > 0");
   NetworkChangeNotifier::AddConnectionTypeObserver(this);
+  UpdateCurrentNetworkName();
 }
 
 NetworkQualityEstimator::~NetworkQualityEstimator() {
@@ -109,7 +125,8 @@ void NetworkQualityEstimator::OnConnectionTypeChanged(
                             fastest_RTT_since_last_connection_change_);
         break;
       default:
-        NOTREACHED();
+        NOTREACHED() << "Unexpected connection type = "
+                     << current_connection_type_;
         break;
     }
   }
@@ -149,15 +166,32 @@ void NetworkQualityEstimator::OnConnectionTypeChanged(
                              peak_kbps_since_last_connection_change_);
         break;
       default:
-        NOTREACHED();
+        NOTREACHED() << "Unexpected connection type = "
+                     << current_connection_type_;
         break;
     }
   }
 
+  // Write the estimates of the previous network to the cache.
+  CacheNetworkQualityEstimate();
+
   last_connection_change_ = base::TimeTicks::Now();
   bytes_read_since_last_connection_change_ = false;
   peak_kbps_since_last_connection_change_ = 0;
   current_connection_type_ = type;
+  UpdateCurrentNetworkName();
+
+  // Read any cached estimates for the new network.
+  ReadCachedNetworkQualityEstimate();
+}
+
+uint32_t NetworkQualityEstimator::GetCacheSizeForTests() const {
+  return cached_network_quality_.size();
+}
+
+void NetworkQualityEstimator::SetCurrentNetworkNameForTests(
+    const std::string& network_name) {
+  current_network_name_ = network_name;
 }
 
 NetworkQuality NetworkQualityEstimator::GetEstimate() const {
@@ -175,4 +209,135 @@ NetworkQuality NetworkQualityEstimator::GetEstimate() const {
                         peak_kbps_since_last_connection_change_, 0.1);
 }
 
+void NetworkQualityEstimator::UpdateCurrentNetworkName() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  current_network_name_ = std::string();
+  switch (current_connection_type_) {
+    case NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN:
+    case NetworkChangeNotifier::ConnectionType::CONNECTION_NONE:
+    case NetworkChangeNotifier::ConnectionType::CONNECTION_BLUETOOTH:
+      return;
+    case NetworkChangeNotifier::ConnectionType::CONNECTION_ETHERNET:
+      current_network_name_ = "ethernet";
+      return;
+#if defined(OS_ANDROID)
+    case NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI:
+      current_network_name_ = GetWifiSSID();
+      return;
+    case NetworkChangeNotifier::ConnectionType::CONNECTION_2G:
+    case NetworkChangeNotifier::ConnectionType::CONNECTION_3G:
+    case NetworkChangeNotifier::ConnectionType::CONNECTION_4G:
+      current_network_name_ = android::GetTelephonyNetworkOperator();
+      return;
+#endif  // OS_ANDROID
+    default:
+#if defined(OS_ANDROID)
+      NOTREACHED() << "Unexpected connection type = "
+                   << current_connection_type_;
+#endif  // OS_ANDROID
+      return;
+  }
+}
+
+bool NetworkQualityEstimator::ReadCachedNetworkQualityEstimate() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (current_network_name_.empty())
+    return false;
+
+  for (const auto& network_quality : cached_network_quality_) {
+    if (!network_quality.MatchesNetwork(current_connection_type_,
+                                        current_network_name_)) {
+      continue;
+    }
+
+    // TOOD(tbansal): Populate these values back into the median computing
+    // algorithm.
+    // Add UMA to record how frequently match happens.
+    // int64 median_kbps = network_quality.median_kbps;
+    // int64 median_rtt_msec = network_quality.median_rtt_milliseconds;
+    return true;
+  }
+
+  return false;
+}
+
+void NetworkQualityEstimator::CacheNetworkQualityEstimate() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (current_network_name_.empty())
+    return;
+
+  // TODO(tbansal): Following variables should be initialized using the median
+  /// values reported by NetworkQualityEstimator.
+  int median_kbps = 0;
+  base::TimeDelta median_rtt = base::TimeDelta();

[bengr, 2015/06/01 20:39:34]

No need to initialize. The default constructor will do that for you.

[tbansal1, 2015/06/01 21:56:51]

Done.

+
+  // If this network is already in the cache, overwrite that entry.
+  for (auto& network_quality : cached_network_quality_) {
+    if (!network_quality.MatchesNetwork(current_connection_type_,
+                                        current_network_name_)) {
+      continue;
+    }
+
+    network_quality.UpdateNetworkQuality(median_kbps, median_rtt);
+    return;
+  }
+
+  if (cached_network_quality_.size() < kMaximumNetworkQualityCacheSize) {
+    cached_network_quality_.push_back(
+        CachedNetworkQuality(current_connection_type_, current_network_name_,
+                             median_kbps, median_rtt));
+    return;
+  }
+
+  DCHECK_EQ(kMaximumNetworkQualityCacheSize, cached_network_quality_.size());
+
+  // Overwrite the oldest entry.
+  int oldest_entry_index = 0;
+  for (size_t i = 0; i < cached_network_quality_.size(); ++i) {
+    if (cached_network_quality_[i].last_updated_ <
+        cached_network_quality_[oldest_entry_index].last_updated_)
+      oldest_entry_index = i;
+  }
+
+  cached_network_quality_[oldest_entry_index] = CachedNetworkQuality(
+      current_connection_type_, current_network_name_, median_kbps, median_rtt);
+
+  DCHECK_EQ(kMaximumNetworkQualityCacheSize, cached_network_quality_.size());
+}
+
+NetworkQualityEstimator::CachedNetworkQuality::CachedNetworkQuality(
+    NetworkChangeNotifier::ConnectionType connection_type,
+    const std::string& network_name,
+    int median_kbps,
+    const base::TimeDelta& median_rtt)
+    : median_kbps_(median_kbps),
+      median_rtt_(median_rtt),
+      last_updated_(base::TimeTicks::Now()),
+      connection_type_(connection_type),
+      network_name_(network_name) {
+  // Networks with empty names should not be cached since they are harder to

[bengr, 2015/06/01 20:39:34]

I disagree. I think a cached entry is helpful regardless, even if it has no
label.

[tbansal1, 2015/06/01 21:56:51]

Done.

+  // disambiguate.
+  DCHECK_NE(network_name_, std::string());
+}
+
+NetworkQualityEstimator::CachedNetworkQuality::~CachedNetworkQuality() {
+}
+
+void NetworkQualityEstimator::CachedNetworkQuality::UpdateNetworkQuality(
+    int median_kbps,
+    const base::TimeDelta& median_rtt) {

[bengr, 2015/06/01 20:39:34]

Would be good to sanity check the parameters, e.g., with DCHECKs.

[tbansal1, 2015/06/01 21:56:51]

Added a TODO in ReadCachedNetworkQualityEstimate().

I did not add DCHECK because some of the estimates can
legitimately be 0.
e.g., if the responses were small, then RTT estimate would be
non-zero but the Kbps estimate would be zero.

[bengr, 2015/06/01 23:21:10]

I'm not sure what you mean. rtt and kbps should never be negative, e.g. Also,
both should probably have an upper bound as well, though I'm not sure what good
values would be. Maybe 5 minutes for rtt?

[tbansal1, 2015/06/02 18:18:05]

Added DCHECKs. Also, added filters in NotifyDataReceived().

+  median_kbps_ = median_kbps;
+  median_rtt_ = median_rtt;
+  last_updated_ = base::TimeTicks::Now();
+}
+
+bool NetworkQualityEstimator::CachedNetworkQuality::MatchesNetwork(
+    NetworkChangeNotifier::ConnectionType connection_type,
+    const std::string& network_name) const {
+  return connection_type == connection_type_ && network_name == network_name_;
+}
+
 }  // namespace net