Store network quality samples so we can compute percentiles.

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 6415ee6e6d7eb36b47ab4fd568d3537283b27729..805ee3080171ced0c8182696805115b17cf426f0 100644
--- a/net/base/network_quality_estimator.cc
+++ b/net/base/network_quality_estimator.cc
@@ -15,6 +15,10 @@
 
 namespace net {
 
+// Maximum number of observations to hold in the ObservationBuffer.
+const size_t NetworkQualityEstimator::ObservationBuffer::kMaximumObservations =
+    500;
+
 NetworkQualityEstimator::NetworkQualityEstimator()
     : NetworkQualityEstimator(false) {
 }
@@ -24,7 +28,7 @@ NetworkQualityEstimator::NetworkQualityEstimator(
     : allow_localhost_requests_(allow_local_host_requests_for_tests),
       last_connection_change_(base::TimeTicks::Now()),
       current_connection_type_(NetworkChangeNotifier::GetConnectionType()),
-      bytes_read_since_last_connection_change_(false),
+      have_received_bytes_since_last_connection_change_(false),
       peak_kbps_since_last_connection_change_(0) {
   static_assert(kMinRequestDurationMicroseconds > 0,
                 "Minimum request duration must be > 0");
@@ -36,10 +40,13 @@ NetworkQualityEstimator::~NetworkQualityEstimator() {
   NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
 }
 
-void NetworkQualityEstimator::NotifyDataReceived(const URLRequest& request,
-                                                 int64_t prefilter_bytes_read) {
+void NetworkQualityEstimator::NotifyDataReceived(
+    const URLRequest& request,
+    int64_t cumulative_prefilter_bytes_read,
+    int64_t prefiltered_bytes_read) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK_GT(prefilter_bytes_read, 0);
+  DCHECK_GT(cumulative_prefilter_bytes_read, 0);
+  DCHECK_GT(prefiltered_bytes_read, 0);
 
   if (!request.url().is_valid() ||
       (!allow_localhost_requests_ && IsLocalhost(request.url().host())) ||
@@ -54,61 +61,68 @@ void NetworkQualityEstimator::NotifyDataReceived(const URLRequest& request,
   base::TimeTicks now = base::TimeTicks::Now();
   base::TimeDelta request_duration = now - request.creation_time();
   DCHECK_GE(request_duration, base::TimeDelta());

[mmenke, 2015/06/11 15:11:01]

If this is 0, we're in trouble, aren't we?  Should we exit early in that case?

[tbansal1, 2015/06/11 19:27:50]

We may not be in trouble. It will cause peak_rtt to be 0. It won't affect kbps
computation (that requires the time duration to be greater than
|kMinRequestDurationMicroseconds|.

I am assuming this will happen rarely enough that median RTT computation won't
be affected. Also, this being zero is not really any different from this being
very low (say 1 microsecond).

-  if (!bytes_read_since_last_connection_change_)
-    fastest_RTT_since_last_connection_change_ = request_duration;
+  if (!have_received_bytes_since_last_connection_change_)
+    fastest_rtt_since_last_connection_change_ = request_duration;
+
+  have_received_bytes_since_last_connection_change_ = true;
+  if (request_duration < fastest_rtt_since_last_connection_change_)
+    fastest_rtt_since_last_connection_change_ = request_duration;

[mmenke, 2015/06/11 15:11:01]

Suggest only doing this if cumulative_prefilter_bytes_read !=
prefiltered_bytes_read.

Also note that if there are redirects, this can give very inaccurate numbers. 
Could use request.GetLoadTimeInfo and just find the difference between now and
send_start.

Those changes don't really belong in this CL, but should give better estimates. 
Note that I'm not sure if we currently set send_start in the case of QUIC.

[tbansal1, 2015/06/11 19:27:50]

Can you please explain why this should be done only when 
cumulative_prefilter_bytes_read !=
prefiltered_bytes_read.

Also, I am already working on using LoadTimingInfo instead of the
creation_time()
https://codereview.chromium.org/1162293004/

[mmenke, 2015/06/11 19:51:18]

RTT should be from when we send the headers to when we get the first part of the
response, not from when the request is created to when we receive a random byte
from it.

Consider:

t=0:  Create request.
t=1:  Send request.
t=2:  Get first bytes, call this with, say "NotifyDataReceived(_, 10, 10)";
t=3:  Get more bytes, call this with, say "NotifyDataReceived(_, 20, 10)";
t=4:  Get more bytes, call this with, say "NotifyDataReceived(_, 30, 10)";
t=5:  ...
...

We should only record an rtt of 1 (t=3 - t=2), but with this code, we'd record
rtts of 2, 3, 4, 5, 6, 7, 8, ....

[tbansal1, 2015/06/11 21:16:29]

Right, it makes sense to do RTT only when cumulative_prefilter_bytes_read ==
prefiltered_bytes_read.
I did that only for RTT samples and not for fastest_rtt.
Now, doing this for fastest_rtt also.

I think I got confused because initially you said we should do this when
cumulative_prefilter_bytes_read !=
prefiltered_bytes_read (!= instead of ==)

 
-  bytes_read_since_last_connection_change_ = true;
-  if (request_duration < fastest_RTT_since_last_connection_change_)
-    fastest_RTT_since_last_connection_change_ = request_duration;
+  // Only add RTT observation if this is the first read for this response.
+  if (cumulative_prefilter_bytes_read == prefiltered_bytes_read)
+    rtt_msec_observations_.AddObservation(request_duration.InMilliseconds());
 
   // Ignore tiny transfers which will not produce accurate rates.
   // Ignore short duration transfers.
-  if (prefilter_bytes_read >= kMinTransferSizeInBytes &&
+  if (cumulative_prefilter_bytes_read >= kMinTransferSizeInBytes &&
       request_duration >=
           base::TimeDelta::FromMicroseconds(kMinRequestDurationMicroseconds)) {
-    uint64_t kbps = static_cast<uint64_t>(prefilter_bytes_read * 8 * 1000 /
-                                          request_duration.InMicroseconds());
+    int32_t kbps =
+        static_cast<int32_t>(cumulative_prefilter_bytes_read * 8 * 1000 /
+                             request_duration.InMicroseconds());

[mmenke, 2015/06/11 15:11:01]

Should handle overflow here.  Unlikely, even for local transfers, but best to be
careful.  Alternatively, could expose a double instead of an int32

[tbansal1, 2015/06/11 19:27:50]

I was handling the overflow in AddObservation() but it seems better to handle it
here itself. Done.

     if (kbps > peak_kbps_since_last_connection_change_)
       peak_kbps_since_last_connection_change_ = kbps;
+
+    kbps_observations_.AddObservation(kbps);
   }
 }
 
 void NetworkQualityEstimator::OnConnectionTypeChanged(
     NetworkChangeNotifier::ConnectionType type) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (bytes_read_since_last_connection_change_) {
+  if (have_received_bytes_since_last_connection_change_) {
     switch (current_connection_type_) {
       case NetworkChangeNotifier::CONNECTION_UNKNOWN:
         UMA_HISTOGRAM_TIMES("NQE.FastestRTT.Unknown",
-                            fastest_RTT_since_last_connection_change_);
+                            fastest_rtt_since_last_connection_change_);
         break;
       case NetworkChangeNotifier::CONNECTION_ETHERNET:
         UMA_HISTOGRAM_TIMES("NQE.FastestRTT.Ethernet",
-                            fastest_RTT_since_last_connection_change_);
+                            fastest_rtt_since_last_connection_change_);
         break;
       case NetworkChangeNotifier::CONNECTION_WIFI:
         UMA_HISTOGRAM_TIMES("NQE.FastestRTT.Wifi",
-                            fastest_RTT_since_last_connection_change_);
+                            fastest_rtt_since_last_connection_change_);
         break;
       case NetworkChangeNotifier::CONNECTION_2G:
         UMA_HISTOGRAM_TIMES("NQE.FastestRTT.2G",
-                            fastest_RTT_since_last_connection_change_);
+                            fastest_rtt_since_last_connection_change_);
         break;
       case NetworkChangeNotifier::CONNECTION_3G:
         UMA_HISTOGRAM_TIMES("NQE.FastestRTT.3G",
-                            fastest_RTT_since_last_connection_change_);
+                            fastest_rtt_since_last_connection_change_);
         break;
       case NetworkChangeNotifier::CONNECTION_4G:
         UMA_HISTOGRAM_TIMES("NQE.FastestRTT.4G",
-                            fastest_RTT_since_last_connection_change_);
+                            fastest_rtt_since_last_connection_change_);
         break;
       case NetworkChangeNotifier::CONNECTION_NONE:
         UMA_HISTOGRAM_TIMES("NQE.FastestRTT.None",
-                            fastest_RTT_since_last_connection_change_);
+                            fastest_rtt_since_last_connection_change_);
         break;
       case NetworkChangeNotifier::CONNECTION_BLUETOOTH:
         UMA_HISTOGRAM_TIMES("NQE.FastestRTT.Bluetooth",
-                            fastest_RTT_since_last_connection_change_);
+                            fastest_rtt_since_last_connection_change_);
         break;
       default:
         NOTREACHED();
@@ -157,24 +171,53 @@ void NetworkQualityEstimator::OnConnectionTypeChanged(
   }
 
   last_connection_change_ = base::TimeTicks::Now();
-  bytes_read_since_last_connection_change_ = false;
+  have_received_bytes_since_last_connection_change_ = false;
   peak_kbps_since_last_connection_change_ = 0;
   current_connection_type_ = type;

[mmenke, 2015/06/11 15:11:01]

Should we throw out our observations here?  If so, should also have a test for
that.

[tbansal1, 2015/06/11 19:27:50]

Yes, we should. This was part of the bigger CL (that also computes the
percentiles). Forgot to move it here from the bigger CL. Added now, also added
test.

 }
 
-NetworkQuality NetworkQualityEstimator::GetEstimate() const {
+NetworkQuality NetworkQualityEstimator::GetPeakEstimate() const {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (!bytes_read_since_last_connection_change_) {
-    return NetworkQuality(fastest_RTT_since_last_connection_change_, 0,
-                          peak_kbps_since_last_connection_change_, 0);
-  }
-  if (!peak_kbps_since_last_connection_change_) {
-    return NetworkQuality(fastest_RTT_since_last_connection_change_, 0.1,
-                          peak_kbps_since_last_connection_change_, 0);
-  }
-  return NetworkQuality(fastest_RTT_since_last_connection_change_, 0.1,
-                        peak_kbps_since_last_connection_change_, 0.1);
+  if (!have_received_bytes_since_last_connection_change_)
+    return NetworkQuality(base::TimeDelta(), 0);

[mmenke, 2015/06/11 15:11:01]

Returning 0 as the default peak transfer rate seems reasonable.  Returning 0 as
the default observed rtt seems questionable.

[tbansal1, 2015/06/11 19:27:50]

Done. Default rtt is now base::TimeDelta::Max(). This simplified the code.
thanks.

+  if (!peak_kbps_since_last_connection_change_)
+    return NetworkQuality(fastest_rtt_since_last_connection_change_, 0);
+  return NetworkQuality(fastest_rtt_since_last_connection_change_,
+                        peak_kbps_since_last_connection_change_);
+}
+
+NetworkQualityEstimator::Observation::Observation(int32_t value,
+                                                  base::TimeTicks timestamp)
+    : value(value), timestamp(timestamp) {
+  DCHECK_GE(value, 0);
+}
+
+NetworkQualityEstimator::Observation::~Observation() {
+}
+
+NetworkQualityEstimator::ObservationBuffer::ObservationBuffer() {
+  static_assert(kMaximumObservations > 0U,
+                "Minimum size of observation buffer must be > 0");

[mmenke, 2015/06/11 15:11:01]

This should go just under the line where you define kMaximumObservations.  It
doesn't map to code, it's a compile-time assertion.

[tbansal1, 2015/06/11 19:27:50]

Problem there is that it does not compile because ObservationBuffer and
kMaximumObservations are private. Here, it compiles and also gets triggered if
the condition is not true.

+}
+
+NetworkQualityEstimator::ObservationBuffer::~ObservationBuffer() {
+}
+
+void NetworkQualityEstimator::ObservationBuffer::AddObservation(int32_t value) {
+  DCHECK_LE(observations_.size(), kMaximumObservations);
+  if (value < 0)

[mmenke, 2015/06/11 15:11:01]

When can this happen?

[tbansal1, 2015/06/11 19:27:50]

This is for the case if there is an overflow on the kbps computation. Overflow
may cause |value| to be negative. Moved this check up in the
NotifyDataReceived()

+    return;
+  // Pop the oldest element if the buffer is already full.
+  if (observations_.size() == kMaximumObservations)
+    observations_.pop_front();
+
+  observations_.push_back(Observation(value, base::TimeTicks::Now()));

[mmenke, 2015/06/11 15:11:01]

Suggest passing base::TimeTicks::Now() in as an argument - OSes *probably* all
cache it, but best to be safe.  Could optionally just pass in an Observation to
this method.

[tbansal1, 2015/06/11 19:27:50]

Done.

+  DCHECK_LE(observations_.size(), kMaximumObservations);
+}
+
+size_t NetworkQualityEstimator::ObservationBuffer::size() const {

[mmenke, 2015/06/11 15:11:01]

If this isn't inlined in the header, it should be named Size().

[tbansal1, 2015/06/11 19:27:50]

Done.

+  return observations_.size();
 }
 
 }  // namespace net