downloads: Improve download rate estimation.

Previously, download rate was estimated as bytes_received/(current_time - download_start_time)

Although simple, this would react slowly to dynamic changes in download time, or user initiated pauses.

With this change, a ring buffer of the past 10 seconds of download time is used instead, and estimates are derived from that. It handles dynamic changes to bandwidth well. Pauses are handled reasonably well, as estimates are accurate after ten seconds - a better solution would be to clear the data rates when the user resumes from a pause. The main downside with this approach is it takes about 100 bytes of memory per active download, as well as adds some complexity.

BUG=16390
TEST=content_unittests

Review URL: https://chromiumcodereview.appspot.com/14697023

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@202874 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/download/base_file.cc

@@ -218,11 +218,6 @@ DownloadInterruptReason BaseFile::AnnotateWithSourceInformation() {
 }
 #endif
 
-int64 BaseFile::CurrentSpeed() const {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  return CurrentSpeedAtTime(base::TimeTicks::Now());
-}
-
 bool BaseFile::GetHash(std::string* hash) {
   DCHECK(!detached_);
   hash->assign(reinterpret_cast<const char*>(sha256_hash_),
@@ -339,12 +334,6 @@ void BaseFile::ClearStream() {
   bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_FILE_OPENED);
 }
 
-int64 BaseFile::CurrentSpeedAtTime(base::TimeTicks current_time) const {
-  base::TimeDelta diff = current_time - start_tick_;
-  int64 diff_ms = diff.InMilliseconds();
-  return diff_ms == 0 ? 0 : bytes_so_far() * 1000 / diff_ms;
-}
-
 DownloadInterruptReason BaseFile::LogNetError(
     const char* operation,
     net::Error error) {

content/browser/download/base_file.h

@@ -73,9 +73,6 @@ class CONTENT_EXPORT BaseFile {
   // DownloadInterruptReason indicating the result of the operation.
   DownloadInterruptReason AnnotateWithSourceInformation();
 
-  // Calculate and return the current speed in bytes per second.
-  int64 CurrentSpeed() const;
-
   base::FilePath full_path() const { return full_path_; }
   bool in_progress() const { return file_stream_.get() != NULL; }
   int64 bytes_so_far() const { return bytes_so_far_; }

content/browser/download/base_file_unittest.cc

@@ -612,46 +612,6 @@ TEST_F(BaseFileTest, IsEmptyHash) {
   EXPECT_FALSE(BaseFile::IsEmptyHash(std::string()));
 }
 
-// Test that calculating speed after no writes.
-TEST_F(BaseFileTest, SpeedWithoutWrite) {
-  ASSERT_TRUE(InitializeFile());
-  base::TimeTicks current = StartTick() + kElapsedTimeDelta;
-  ASSERT_EQ(0, CurrentSpeedAtTime(current));
-  base_file_->Finish();
-}
-
-// Test that calculating speed after a single write.
-TEST_F(BaseFileTest, SpeedAfterSingleWrite) {
-  ASSERT_TRUE(InitializeFile());
-  ASSERT_TRUE(AppendDataToFile(kTestData1));
-  base::TimeTicks current = StartTick() + kElapsedTimeDelta;
-  int64 expected_speed = kTestDataLength1 / kElapsedTimeSeconds;
-  ASSERT_EQ(expected_speed, CurrentSpeedAtTime(current));
-  base_file_->Finish();
-}
-
-// Test that calculating speed after a multiple writes.
-TEST_F(BaseFileTest, SpeedAfterMultipleWrite) {
-  ASSERT_TRUE(InitializeFile());
-  ASSERT_TRUE(AppendDataToFile(kTestData1));
-  ASSERT_TRUE(AppendDataToFile(kTestData2));
-  ASSERT_TRUE(AppendDataToFile(kTestData3));
-  ASSERT_TRUE(AppendDataToFile(kTestData4));
-  base::TimeTicks current = StartTick() + kElapsedTimeDelta;
-  int64 expected_speed = (kTestDataLength1 + kTestDataLength2 +
-      kTestDataLength3 + kTestDataLength4) / kElapsedTimeSeconds;
-  ASSERT_EQ(expected_speed, CurrentSpeedAtTime(current));
-  base_file_->Finish();
-}
-
-// Test that calculating speed after no delay - should not divide by 0.
-TEST_F(BaseFileTest, SpeedAfterNoElapsedTime) {
-  ASSERT_TRUE(InitializeFile());
-  ASSERT_TRUE(AppendDataToFile(kTestData1));
-  ASSERT_EQ(0, CurrentSpeedAtTime(StartTick()));
-  base_file_->Finish();
-}
-
 // Test that a temporary file is created in the default download directory.
 TEST_F(BaseFileTest, CreatedInDefaultDirectory) {
   ASSERT_TRUE(base_file_->full_path().empty());

content/browser/download/download_file_impl.cc

@@ -98,6 +98,7 @@ DownloadInterruptReason DownloadFileImpl::AppendDataToFile(
                          base::TimeDelta::FromMilliseconds(kUpdatePeriodMs),
                          this, &DownloadFileImpl::SendUpdate);
   }
+  rate_estimator_.Increment(data_len);
   return file_.AppendDataToFile(data, data_len);
 }
 
@@ -187,7 +188,7 @@ bool DownloadFileImpl::InProgress() const {
 }
 
 int64 DownloadFileImpl::CurrentSpeed() const {
-  return file_.CurrentSpeed();
+  return rate_estimator_.GetCountPerSecond();
 }
 
 bool DownloadFileImpl::GetHash(std::string* hash) {

content/browser/download/download_file_impl.h

@@ -14,6 +14,7 @@
 #include "base/timer.h"
 #include "content/browser/byte_stream.h"
 #include "content/browser/download/base_file.h"
+#include "content/browser/download/rate_estimator.h"
 #include "content/public/browser/download_save_info.h"
 #include "net/base/net_log.h"
 
@@ -95,6 +96,7 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public DownloadFile {
   size_t bytes_seen_;
   base::TimeDelta disk_writes_time_;
   base::TimeTicks download_start_;
+  RateEstimator rate_estimator_;
 
   net::BoundNetLog bound_net_log_;
 

content/browser/download/rate_estimator.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/download/rate_estimator.h"
+
+#include "base/logging.h"
+
+using base::TimeDelta;
+using base::TimeTicks;
+
+namespace content {
+
+namespace {
+
+static const int kDefaultBucketTimeSeconds = 1;
+static const size_t kDefaultNumBuckets = 10;
+
+} // namespace
+
+RateEstimator::RateEstimator()
+    : history_(kDefaultNumBuckets),
+      bucket_time_(TimeDelta::FromSeconds(kDefaultBucketTimeSeconds)),
+      oldest_index_(0),
+      bucket_count_(1) {
+  ResetBuckets(TimeTicks::Now());
+}
+
+RateEstimator::RateEstimator(TimeDelta bucket_time,
+                             size_t num_buckets,
+                             TimeTicks now)
+    : history_(num_buckets),
+      bucket_time_(bucket_time),
+      oldest_index_(0),
+      bucket_count_(1) {
+  DCHECK(bucket_time_.InSeconds() > 0);
+  ResetBuckets(now);
+}
+
+RateEstimator::~RateEstimator() {
+}
+
+void RateEstimator::Increment(uint32 count) {
+  Increment(count, TimeTicks::Now());
+}
+
+void RateEstimator::Increment(uint32 count, TimeTicks now) {
+  ClearOldBuckets(now);
+  int64 seconds_since_oldest = (now - oldest_time_).InSeconds();
+  DCHECK(seconds_since_oldest >= 0);
+  int64 delta_buckets = seconds_since_oldest / bucket_time_.InSeconds();
+  DCHECK(delta_buckets >= 0);
+  size_t index_offset = static_cast<size_t>(delta_buckets);
+  DCHECK(index_offset <= history_.size());
+  int current_index = (oldest_index_ + delta_buckets) % history_.size();
+  history_[current_index] += count;
+}
+
+uint64 RateEstimator::GetCountPerSecond() const {
+  return GetCountPerSecond(TimeTicks::Now());
+}
+
+uint64 RateEstimator::GetCountPerSecond(TimeTicks now) const {
+  const_cast<RateEstimator*>(this)->ClearOldBuckets(now);
+  // TODO(cbentzel): Support fractional seconds for active bucket?
+  // We explicitly don't check for overflow here. If it happens, unsigned
+  // arithmetic at least guarantees behavior by wrapping around. The estimate
+  // will be off, but the code will still be valid.
+  uint64 total_count = 0;
+  for (size_t i = 0; i < bucket_count_; ++i) {
+    size_t index = (oldest_index_ + i) % history_.size();
+    total_count += history_[index];
+  }
+  return total_count / (bucket_count_ * bucket_time_.InSeconds());
+}
+
+void RateEstimator::ClearOldBuckets(TimeTicks now) {
+  int64 seconds_since_oldest = (now - oldest_time_).InSeconds();
+
+  int64 delta_buckets = seconds_since_oldest / bucket_time_.InSeconds();
+
+  // It's possible (although unlikely) for there to be rollover with TimeTicks.
+  // If that's the case, just reset the history.
+  if (delta_buckets < 0) {
+    ResetBuckets(now);
+    return;
+  }
+  size_t delta_index = static_cast<size_t>(delta_buckets);
+
+  // If we are within the current window, keep the existing data.
+  if (delta_index < history_.size()) {
+    bucket_count_ = delta_index + 1;
+    return;
+  }
+
+  // If it's been long enough that all history data is too stale, just
+  // clear all the buckets.
+  size_t extra_buckets = delta_index - history_.size() + 1;
+  if (extra_buckets > history_.size()) {
+    ResetBuckets(now);
+    return;
+  }
+
+  // Clear out stale buckets in the history.
+  bucket_count_ = history_.size();
+  for (size_t i = 0; i < extra_buckets; ++i) {
+    history_[oldest_index_] = 0;
+    oldest_index_ = (oldest_index_ + 1) % history_.size();
+    oldest_time_ = oldest_time_ + bucket_time_;
+  }
+}
+
+void RateEstimator::ResetBuckets(TimeTicks now) {
+  for (size_t i = 0; i < history_.size(); ++i) {
+    history_[i] = 0;
+  }
+  oldest_index_ = 0;
+  bucket_count_ = 1;
+  oldest_time_ = now;
+}
+
+}  // namespace content

content/browser/download/rate_estimator.h

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_BROWSER_DOWNLOAD_RATE_ESTIMATOR_H_
+#define CONTENT_BROWSER_DOWNLOAD_RATE_ESTIMATOR_H_
+
+#include <string>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/time.h"
+#include "content/common/content_export.h"
+
+namespace content {
+
+// RateEstimator generates rate estimates based on recent activity.
+//
+// Internally it uses a fixed-size ring buffer, and develops estimates
+// based on a small sliding window of activity.
+class CONTENT_EXPORT RateEstimator {
+ public:
+  RateEstimator();
+  RateEstimator(base::TimeDelta bucket_time,
+                size_t num_buckets,
+                base::TimeTicks now);
+  ~RateEstimator();
+
+  // Increment the counter by |count|. The first variant uses the current time,
+  // the second variant provides the time that |count| is observed.
+  void Increment(uint32 count);
+  void Increment(uint32 count, base::TimeTicks now);
+
+  // Get a rate estimate, in terms of counts/second. The first variant uses the
+  // current time, the second variant provides the time.
+  uint64 GetCountPerSecond() const;
+  uint64 GetCountPerSecond(base::TimeTicks now) const;
+
+ private:
+  void ClearOldBuckets(base::TimeTicks now);
+  void ResetBuckets(base::TimeTicks now);
+
+  std::vector<uint32> history_;
+  base::TimeDelta bucket_time_;
+  size_t oldest_index_;
+  size_t bucket_count_;
+  base::TimeTicks oldest_time_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_DOWNLOAD_RATE_ESTIMATOR_H_

content/browser/download/rate_estimator_unittest.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/download/rate_estimator.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+using base::TimeDelta;
+
+namespace content {
+
+TEST(RateEstimatorTest, RateEstimator) {
+  base::TimeTicks now;
+  RateEstimator estimator(TimeDelta::FromSeconds(1), 10u, now);
+  EXPECT_EQ(0u, estimator.GetCountPerSecond(now));
+
+  estimator.Increment(50u, now);
+  EXPECT_EQ(50u, estimator.GetCountPerSecond(now));
+
+  now += TimeDelta::FromMilliseconds(800);
+  estimator.Increment(50, now);
+  EXPECT_EQ(100u, estimator.GetCountPerSecond(now));
+
+  // Advance time.
+  now += TimeDelta::FromSeconds(3);
+  EXPECT_EQ(25u, estimator.GetCountPerSecond(now));
+  estimator.Increment(60, now);
+  EXPECT_EQ(40u, estimator.GetCountPerSecond(now));
+
+  // Advance time again.
+  now += TimeDelta::FromSeconds(4);
+  EXPECT_EQ(20u, estimator.GetCountPerSecond(now));
+
+  // Advance time to the end.
+  now += TimeDelta::FromSeconds(2);
+  EXPECT_EQ(16u, estimator.GetCountPerSecond(now));
+  estimator.Increment(100, now);
+  EXPECT_EQ(26u, estimator.GetCountPerSecond(now));
+
+  // Now wrap around to the start.
+  now += TimeDelta::FromSeconds(1);
+  EXPECT_EQ(16u, estimator.GetCountPerSecond(now));
+  estimator.Increment(100, now);
+  EXPECT_EQ(26u, estimator.GetCountPerSecond(now));
+
+  // Advance far into the future.
+  now += TimeDelta::FromSeconds(40);
+  EXPECT_EQ(0u, estimator.GetCountPerSecond(now));
+  estimator.Increment(100, now);
+  EXPECT_EQ(100u, estimator.GetCountPerSecond(now));
+
+  // Pretend that there is timeticks wrap around.
+  now = base::TimeTicks();
+  EXPECT_EQ(0u, estimator.GetCountPerSecond(now));
+}
+
+}  // namespace content

content/content_browser.gypi

@@ -418,6 +418,8 @@
     'browser/download/file_metadata_mac.mm',
     'browser/download/mhtml_generation_manager.cc',
     'browser/download/mhtml_generation_manager.h',
+    'browser/download/rate_estimator.cc',
+    'browser/download/rate_estimator.h',
     'browser/download/save_file.cc',
     'browser/download/save_file.h',
     'browser/download/save_file_manager.cc',

content/content_tests.gypi

@@ -269,6 +269,7 @@
         'browser/download/download_item_impl_unittest.cc',
         'browser/download/download_manager_impl_unittest.cc',
         'browser/download/file_metadata_unittest_linux.cc',
+        'browser/download/rate_estimator_unittest.cc',
         'browser/download/save_package_unittest.cc',
         'browser/gamepad/gamepad_provider_unittest.cc',
         'browser/gamepad/gamepad_test_helpers.cc',