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..918dc0a1625768c3078799af3bd33cf01002ebd1 100644
--- a/net/base/network_quality_estimator.cc
+++ b/net/base/network_quality_estimator.cc
@@ -4,14 +4,26 @@
 
 #include "net/base/network_quality_estimator.h"
 
-#include <string>
-
+#include "base/logging.h"
 #include "base/metrics/histogram.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 = 5;
+
+}  // namespace
+
 namespace net {
 
 NetworkQualityEstimator::NetworkQualityEstimator()
@@ -27,7 +39,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 +124,8 @@ void NetworkQualityEstimator::OnConnectionTypeChanged(
                             fastest_RTT_since_last_connection_change_);
         break;
       default:
-        NOTREACHED();
+        NOTREACHED() << "Unexpected connection type = "
+                     << current_connection_type_;
         break;
     }
   }
@@ -149,15 +165,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(
+    std::string network_name) {
+  current_network_name_ = network_name;
 }
 
 NetworkQuality NetworkQualityEstimator::GetEstimate() const {
@@ -175,4 +208,107 @@ NetworkQuality NetworkQualityEstimator::GetEstimate() const {
                         peak_kbps_since_last_connection_change_, 0.1);
 }
 
+void NetworkQualityEstimator::UpdateCurrentNetworkName() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  current_network_name_ = std::string();
+  if (current_connection_type_ ==
+          NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN ||

[bengr, 2015/05/29 17:38:09]

Using a switch might be cleaner.

[tbansal1, 2015/05/29 19:13:36]

Done.

+      current_connection_type_ ==
+          NetworkChangeNotifier::ConnectionType::CONNECTION_NONE ||
+      current_connection_type_ ==
+          NetworkChangeNotifier::ConnectionType::CONNECTION_BLUETOOTH) {
+    return;
+  }
+
+  if (current_connection_type_ ==
+      NetworkChangeNotifier::ConnectionType::CONNECTION_ETHERNET) {
+    current_network_name_ = "ethernet";
+    return;
+  }
+
+#if defined(OS_ANDROID)
+  if (current_connection_type_ ==
+      NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI)
+    return GetWifiSSID();
+  if (current_connection_type_ ==
+          NetworkChangeNotifier::ConnectionType::CONNECTION_2G ||
+      current_connection_type_ ==
+          NetworkChangeNotifier::ConnectionType::CONNECTION_3G ||
+      current_connection_type_ ==
+          NetworkChangeNotifier::ConnectionType::CONNECTION_4G) {
+    current_network_name_ = android::GetTelephonyNetworkOperator();
+    return;
+  }
+  NOTREACHED() << "Unexpected connection type = " << current_connection_type_;
+#endif  // OS_ANDROID
+}
+
+bool NetworkQualityEstimator::ReadCachedNetworkQualityEstimate() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (current_network_name_ == "")

[bengr, 2015/05/29 17:38:09]

if (current_network_name_.empty())

[tbansal1, 2015/05/29 19:13:36]

Done.

+    return false;
+
+  for (const auto& network_quality : cached_network_quality_) {
+    if (!network_quality.MatchesNetwork(current_connection_type_,
+                                        current_network_name_))
+      continue;

[bengr, 2015/05/29 17:38:09]

Add curly braces.

[tbansal1, 2015/05/29 19:13:36]

Done.

+
+    // TOOD(tbansal): Populate these values back into the median computing

[bengr, 2015/05/29 17:38:09]

Why are you not doing this in this CL?

[tbansal1, 2015/05/29 19:13:36]

I need to populate these values back into CircularBuffer
(aka DecayingHistogram) which I do not have yet.

+    // 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_ == "")

[bengr, 2015/05/29 17:38:09]

current_network_name_.empty()

[tbansal1, 2015/05/29 19:13:36]

Done.

+    return;
+
+  // TODO(tbansal): Following variables should be initialized using the median
+  /// values reported by NetworkQualityEstimator.
+  int median_kbps = 0;
+  int median_rtt_milliseconds = 0;
+
+  // 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;

[bengr, 2015/05/29 17:38:09]

Add curly braces.

[tbansal1, 2015/05/29 19:13:36]

Done.

+
+    network_quality.UpdateNetworkQuality(median_kbps, median_rtt_milliseconds);
+    return;
+  }
+
+  if (cached_network_quality_.size() < kMaximumNetworkQualityCacheSize) {
+    cached_network_quality_.push_back(
+        CachedNetworkQuality(current_connection_type_, current_network_name_,
+                             median_kbps, median_rtt_milliseconds));
+    return;
+  }
+
+  DCHECK_EQ(kMaximumNetworkQualityCacheSize, cached_network_quality_.size());
+
+  // Overwrite the oldest entry.

[bengr, 2015/05/29 17:38:09]

I guess this is ok, but how often will this code be needed in practice? Can't we
just make kMaximumNetworkQualityCacheSize == 25ish, and then ignore additional
networks?

Also, how does this logic change once this persists?

[tbansal1, 2015/05/29 19:13:36]

If we write the in-memory cache to prefs, 25 still would not be enough. So, at
some point, we need to get rid of LRU entries.
Not sure what it means by persists in this context?

+  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_milliseconds);
+
+  DCHECK_EQ(kMaximumNetworkQualityCacheSize, cached_network_quality_.size());
+}
+
 }  // namespace net