Delay amortizing overhead that isn't reflected in pre-amortization bytes

This CL changes the TrafficStatsAmortizer from ignoring overhead that
isn't reflected in any pre-amortization bytes, and instead defers that
overhead to the next amortization run so that the next amortization run
can try to include the overhead.

This fixes an issue where the TrafficStatsAmortizer would ignore
overhead seen by TrafficStats but hadn't yet been reported by the
network stack, which would happen often on slow networks.

BUG=556836

Review URL: https://codereview.chromium.org/1453193003

Cr-Commit-Position: refs/heads/master@{#360277}

components/data_usage/android/traffic_stats_amortizer.cc

@@ -71,21 +71,14 @@ int64_t ScaleByteCount(int64_t bytes, double ratio, double* remainder) {
 // |pre_amortization_total| into each of the DataUse objects in
 // |data_use_sequence| by scaling the byte counts determined by the
 // |get_byte_count_fn| function (e.g. tx_bytes, rx_bytes) for each DataUse
-// appropriately.
+// appropriately. |pre_amortization_total| must not be 0.
 void AmortizeByteCountSequence(DataUseBuffer* data_use_sequence,
                                int64_t* (*get_byte_count_fn)(DataUse*),
                                int64_t pre_amortization_total,
                                int64_t desired_post_amortization_total) {
-  DCHECK_GE(pre_amortization_total, 0);
+  DCHECK_GT(pre_amortization_total, 0);
   DCHECK_GE(desired_post_amortization_total, 0);
 
-  if (pre_amortization_total == 0) {
-    // TODO(sclittle): If |desired_post_amortization_total| is non-zero, this
-    // could be ignoring overhead that should be amortized in somehow. Handle
-    // this case gracefully.
-    return;
-  }
-
   const double ratio = static_cast<double>(desired_post_amortization_total) /
                        static_cast<double>(pre_amortization_total);
 
@@ -104,27 +97,6 @@ int64_t* GetRxBytes(DataUse* data_use) {
   return &data_use->rx_bytes;
 }
 
-// Amortizes the difference between |desired_post_amortization_total_tx_bytes|
-// and |pre_amortization_total_tx_bytes| into each of the DataUse objects in
-// |data_use_sequence| by scaling the DataUse's |tx_bytes| appropriately. Does
-// the same with the |rx_bytes| using those respective parameters.
-void AmortizeDataUseSequence(DataUseBuffer* data_use_sequence,
-                             int64_t pre_amortization_total_tx_bytes,
-                             int64_t desired_post_amortization_total_tx_bytes,
-                             int64_t pre_amortization_total_rx_bytes,
-                             int64_t desired_post_amortization_total_rx_bytes) {
-  DCHECK(data_use_sequence);
-  DCHECK(!data_use_sequence->empty());
-
-  AmortizeByteCountSequence(data_use_sequence, &GetTxBytes,
-                            pre_amortization_total_tx_bytes,
-                            desired_post_amortization_total_tx_bytes);
-
-  AmortizeByteCountSequence(data_use_sequence, &GetRxBytes,
-                            pre_amortization_total_rx_bytes,
-                            desired_post_amortization_total_rx_bytes);
-}
-
 }  // namespace
 
 TrafficStatsAmortizer::TrafficStatsAmortizer()
@@ -252,17 +224,21 @@ void TrafficStatsAmortizer::AmortizeNow() {
     DCHECK_GE(current_traffic_stats_rx_bytes,
               last_amortization_traffic_stats_rx_bytes_);
 
-    int64_t desired_post_amortization_total_tx_bytes =
-        current_traffic_stats_tx_bytes -
-        last_amortization_traffic_stats_tx_bytes_;
-    int64_t desired_post_amortization_total_rx_bytes =
-        current_traffic_stats_rx_bytes -
-        last_amortization_traffic_stats_rx_bytes_;
-
-    AmortizeDataUseSequence(&buffered_data_use_, pre_amortization_tx_bytes_,
-                            desired_post_amortization_total_tx_bytes,
-                            pre_amortization_rx_bytes_,
-                            desired_post_amortization_total_rx_bytes);
+    // Only attempt to amortize network overhead from TrafficStats if any of
+    // those bytes are reflected in the pre-amortization byte totals. Otherwise,
+    // that network overhead will be amortized in a later amortization run.
+    if (pre_amortization_tx_bytes_ != 0) {
+      AmortizeByteCountSequence(&buffered_data_use_, &GetTxBytes,
+                                pre_amortization_tx_bytes_,
+                                current_traffic_stats_tx_bytes -
+                                    last_amortization_traffic_stats_tx_bytes_);
+    }
+    if (pre_amortization_rx_bytes_ != 0) {
+      AmortizeByteCountSequence(&buffered_data_use_, &GetRxBytes,
+                                pre_amortization_rx_bytes_,
+                                current_traffic_stats_rx_bytes -
+                                    last_amortization_traffic_stats_rx_bytes_);
+    }
   }
 
   // TODO(sclittle): Record some UMA about the delay before amortizing and how
@@ -272,21 +248,34 @@ void TrafficStatsAmortizer::AmortizeNow() {
   is_amortization_in_progress_ = false;
   current_amortization_run_start_time_ = base::TimeTicks();
 
+  // Don't update the previous amortization run's TrafficStats byte counts if
+  // none of the bytes since then are reflected in the pre-amortization byte
+  // totals. This way, the overhead that wasn't handled in this amortization run
+  // can be handled in a later amortization run that actually has bytes in that
+  // direction. This mitigates the problem of losing TrafficStats overhead bytes
+  // on slow networks due to TrafficStats seeing the bytes much earlier than the
+  // network stack reports them, or vice versa.
+  if (!are_last_amortization_traffic_stats_available_ ||
+      pre_amortization_tx_bytes_ != 0) {
+    last_amortization_traffic_stats_tx_bytes_ = current_traffic_stats_tx_bytes;
+  }
+  if (!are_last_amortization_traffic_stats_available_ ||
+      pre_amortization_rx_bytes_ != 0) {
+    last_amortization_traffic_stats_rx_bytes_ = current_traffic_stats_rx_bytes;
+  }
+
   are_last_amortization_traffic_stats_available_ =
       are_current_traffic_stats_available;
-  last_amortization_traffic_stats_tx_bytes_ = current_traffic_stats_tx_bytes;
-  last_amortization_traffic_stats_rx_bytes_ = current_traffic_stats_rx_bytes;
 
   pre_amortization_tx_bytes_ = 0;
   pre_amortization_rx_bytes_ = 0;
 
-  // Pass post-amortization DataUse objects to their respective callbacks.
   DataUseBuffer data_use_sequence;
   data_use_sequence.swap(buffered_data_use_);
-  for (auto& data_use_buffer_pair : data_use_sequence) {
-    scoped_ptr<DataUse> data_use(data_use_buffer_pair.first.release());
-    data_use_buffer_pair.second.Run(data_use.Pass());
-  }
+
+  // Pass post-amortization DataUse objects to their respective callbacks.
+  for (auto& data_use_buffer_pair : data_use_sequence)
+    data_use_buffer_pair.second.Run(data_use_buffer_pair.first.Pass());
 }
 
 }  // namespace android

components/data_usage/android/traffic_stats_amortizer_unittest.cc

@@ -340,28 +340,61 @@ TEST_F(TrafficStatsAmortizerTest, AmortizeWithZeroScaleFactor) {
 TEST_F(TrafficStatsAmortizerTest, AmortizeWithZeroPreAmortizationBytes) {
   SkipFirstAmortizationRun();
 
-  // Both byte counts should stay 0, even though TrafficStats saw bytes.
+  // Both byte counts should stay 0, even though TrafficStats saw bytes, which
+  // should be reflected in the next amortization run instead.
   amortizer()->AmortizeDataUse(CreateDataUse(0, 0),
                                ExpectDataUseCallback(CreateDataUse(0, 0)));
+  // Add the TrafficStats byte counts for this and the next amortization run.
   amortizer()->AddTrafficStats(100, 1000);
   AdvanceTime(kTrafficStatsQueryDelay);
   EXPECT_EQ(1, data_use_callback_call_count());
 
-  // This time, only TX bytes are 0, so RX bytes should double, but TX bytes
-  // should stay 0.
+  // Both byte counts should double, even though the TrafficStats byte counts
+  // actually updated during the previous amortization run.
+  amortizer()->AmortizeDataUse(CreateDataUse(50, 500),
+                               ExpectDataUseCallback(CreateDataUse(100, 1000)));
+  AdvanceTime(kTrafficStatsQueryDelay);
+  EXPECT_EQ(2, data_use_callback_call_count());
+}
+
+TEST_F(TrafficStatsAmortizerTest, AmortizeWithZeroTxPreAmortizationBytes) {
+  SkipFirstAmortizationRun();
+
+  // The count of transmitted bytes starts at 0, so it should stay 0, and be
+  // amortized in the next amortization run instead.
   amortizer()->AmortizeDataUse(CreateDataUse(0, 500),
                                ExpectDataUseCallback(CreateDataUse(0, 1000)));
+  // Add the TrafficStats byte counts for this and the next amortization run.
   amortizer()->AddTrafficStats(100, 1000);
   AdvanceTime(kTrafficStatsQueryDelay);
+  EXPECT_EQ(1, data_use_callback_call_count());
+
+  // The count of transmitted bytes should double, even though they actually
+  // updated during the previous amortization run.
+  amortizer()->AmortizeDataUse(CreateDataUse(50, 0),
+                               ExpectDataUseCallback(CreateDataUse(100, 0)));
+  AdvanceTime(kTrafficStatsQueryDelay);
   EXPECT_EQ(2, data_use_callback_call_count());
+}
 
-  // This time, only RX bytes are 0, so TX bytes should double, but RX bytes
-  // should stay 0.
+TEST_F(TrafficStatsAmortizerTest, AmortizeWithZeroRxPreAmortizationBytes) {
+  SkipFirstAmortizationRun();
+
+  // The count of received bytes starts at 0, so it should stay 0, and be
+  // amortized in the next amortization run instead.
   amortizer()->AmortizeDataUse(CreateDataUse(50, 0),
                                ExpectDataUseCallback(CreateDataUse(100, 0)));
+  // Add the TrafficStats byte counts for this and the next amortization run.
   amortizer()->AddTrafficStats(100, 1000);
   AdvanceTime(kTrafficStatsQueryDelay);
-  EXPECT_EQ(3, data_use_callback_call_count());
+  EXPECT_EQ(1, data_use_callback_call_count());
+
+  // The count of received bytes should double, even though they actually
+  // updated during the previous amortization run.
+  amortizer()->AmortizeDataUse(CreateDataUse(0, 500),
+                               ExpectDataUseCallback(CreateDataUse(0, 1000)));
+  AdvanceTime(kTrafficStatsQueryDelay);
+  EXPECT_EQ(2, data_use_callback_call_count());
 }
 
 TEST_F(TrafficStatsAmortizerTest, AmortizeAtMaxDelay) {
@@ -375,7 +408,7 @@ TEST_F(TrafficStatsAmortizerTest, AmortizeAtMaxDelay) {
   // kSmallDelay is a delay that's shorter than the delay before TrafficStats
   // would be queried, where kMaxAmortizationDelay is a multiple of kSmallDelay.
   const base::TimeDelta kSmallDelay = kMaxAmortizationDelay / 10;
-  EXPECT_LT(kSmallDelay, kMaxAmortizationDelay);
+  EXPECT_LT(kSmallDelay, kTrafficStatsQueryDelay);
 
   // Simulate multiple cases of extra bytes being reported, each before
   // TrafficStats would be queried, until kMaxAmortizationDelay has elapsed.