Throttle DownloadDB updates

This CL stops DownloadDBCache to immediately updating the DB.
Instead, it caches the change and only update the DB when timer fires.
Some of the updates are considered high priority and will thus
flush to DB immediately.

Bug: 850990
Change-Id: I124ac1c69e5c7e6bd609d656be3ac1a938f78c63
Reviewed-on: https://chromium-review.googlesource.com/1102084Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569736}

components/download/database/download_db.h

@@ -32,10 +32,13 @@ class DownloadDB {
   // Initializes this db asynchronously, callback will be run on completion.
   virtual void Initialize(InitializeCallback callback) = 0;
 
-  // Adds or updates |entry| in the storage asynchronously and returns whether
-  // or not that was successful.
+  // Adds or updates |entry| in the storage.
   virtual void AddOrReplace(const DownloadDBEntry& entry) = 0;
 
+  // Adds or updates multiple entries in the storage.
+  virtual void AddOrReplaceEntries(
+      const std::vector<DownloadDBEntry>& entry) = 0;
+
   // Retrieves all entries with the given |download_namespace|.
   virtual void LoadEntries(LoadEntriesCallback callback) = 0;
 

components/download/database/download_db_impl.cc

@@ -85,11 +85,19 @@ void DownloadDBImpl::DestroyAndReinitialize(InitializeCallback callback) {
 }
 
 void DownloadDBImpl::AddOrReplace(const DownloadDBEntry& entry) {
+  AddOrReplaceEntries(std::vector<DownloadDBEntry>{entry});
+}
+
+void DownloadDBImpl::AddOrReplaceEntries(
+    const std::vector<DownloadDBEntry>& entries) {
   DCHECK(IsInitialized());
   auto entries_to_save = std::make_unique<ProtoKeyEntryVector>();
-  download_pb::DownloadDBEntry proto =
-      DownloadDBConversions::DownloadDBEntryToProto(entry);
-  entries_to_save->emplace_back(GetEntryKey(entry.GetGuid()), std::move(proto));
+  for (const auto& entry : entries) {
+    download_pb::DownloadDBEntry proto =
+        DownloadDBConversions::DownloadDBEntryToProto(entry);
+    entries_to_save->emplace_back(GetEntryKey(entry.GetGuid()),
+                                  std::move(proto));
+  }
   db_->UpdateEntries(std::move(entries_to_save),
                      std::make_unique<ProtoKeyVector>(),
                      base::BindOnce(&DownloadDBImpl::OnUpdateDone,

components/download/database/download_db_impl.h

@@ -36,6 +36,8 @@ class DownloadDBImpl : public DownloadDB {
   bool IsInitialized() override;
   void Initialize(InitializeCallback callback) override;
   void AddOrReplace(const DownloadDBEntry& entry) override;
+  void AddOrReplaceEntries(
+      const std::vector<DownloadDBEntry>& entries) override;
   void LoadEntries(LoadEntriesCallback callback) override;
   void Remove(const std::string& guid) override;
 

components/download/internal/common/download_db_cache.cc

@@ -12,6 +12,47 @@ namespace download {
 
 namespace {
 
+// Interval between download DB updates.
+// TODO(qinmin): make this finch configurable.
+const int kUpdateDBIntervalMs = 10000;
+
+enum class ShouldUpdateDownloadDBResult {
+  NO_UPDATE,
+  UPDATE,
+  UPDATE_IMMEDIATELY,
+};
+
+ShouldUpdateDownloadDBResult ShouldUpdateDownloadDB(
+    base::Optional<DownloadDBEntry> previous,
+    const DownloadDBEntry& current) {
+  if (!previous)
+    return ShouldUpdateDownloadDBResult::UPDATE_IMMEDIATELY;
+
+  base::Optional<InProgressInfo> previous_info;
+  if (previous->download_info)
+    previous_info = previous->download_info->in_progress_info;
+  base::FilePath previous_path =
+      previous_info ? previous_info->current_path : base::FilePath();
+
+  base::Optional<InProgressInfo> current_info;
+  if (current.download_info)
+    current_info = current.download_info->in_progress_info;
+
+  base::FilePath current_path =
+      current_info ? current_info->current_path : base::FilePath();
+
+  // When download path is determined, Chrome should commit the history
+  // immediately. Otherwise the file will be left permanently on the external
+  // storage if Chrome crashes right away.
+  if (current_path != previous_path)
+    return ShouldUpdateDownloadDBResult::UPDATE_IMMEDIATELY;
+
+  if (previous.value() == current)
+    return ShouldUpdateDownloadDBResult::NO_UPDATE;
+
+  return ShouldUpdateDownloadDBResult::UPDATE;
+}
+
 bool GetFetchErrorBody(base::Optional<DownloadDBEntry> entry) {
   if (!entry)
     return false;
@@ -112,19 +153,44 @@ void DownloadDBCache::AddOrReplaceEntry(const DownloadDBEntry& entry) {
   if (!entry.download_info)
     return;
   const std::string& guid = entry.download_info->guid;
-  base::Optional<DownloadDBEntry> current = RetrieveEntry(guid);
-  if (!current || entry != current.value()) {
-    entries_.emplace(guid, entry);
-    download_db_->AddOrReplace(entry);
+  ShouldUpdateDownloadDBResult result =
+      ShouldUpdateDownloadDB(RetrieveEntry(guid), entry);
+  if (result == ShouldUpdateDownloadDBResult::NO_UPDATE)
+    return;
+  if (!update_timer_.IsRunning() &&
+      result == ShouldUpdateDownloadDBResult::UPDATE) {
+    update_timer_.Start(FROM_HERE,
+                        base::TimeDelta::FromMilliseconds(kUpdateDBIntervalMs),
+                        this, &DownloadDBCache::UpdateDownloadDB);
+  }
+
+  entries_[guid] = entry;
+  updated_guids_.emplace(guid);
+  if (result == ShouldUpdateDownloadDBResult::UPDATE_IMMEDIATELY) {
+    UpdateDownloadDB();
+    update_timer_.Stop();
   }
 }
 
 void DownloadDBCache::RemoveEntry(const std::string& guid) {
   entries_.erase(guid);
+  updated_guids_.erase(guid);
   if (download_db_)
     download_db_->Remove(guid);
 }
 
+void DownloadDBCache::UpdateDownloadDB() {
+  DCHECK(!updated_guids_.empty());
+
+  std::vector<DownloadDBEntry> entries;
+  for (auto guid : updated_guids_) {
+    base::Optional<DownloadDBEntry> entry = RetrieveEntry(guid);
+    DCHECK(entry);
+    entries.emplace_back(entry.value());
+  }
+  download_db_->AddOrReplaceEntries(entries);
+}
+
 void DownloadDBCache::OnDownloadUpdated(DownloadItem* download) {
   // TODO(crbug.com/778425): Properly handle fail/resume/retry for downloads
   // that are in the INTERRUPTED state for a long time.
@@ -136,8 +202,6 @@ void DownloadDBCache::OnDownloadUpdated(DownloadItem* download) {
   DownloadUrlParameters::RequestHeadersType request_header_type =
       GetRequestHeadersType(current);
   DownloadSource download_source = GetDownloadSource(current);
-  // TODO(http://crbug.com/850990): Throttle the database updates, it is very
-  // costly.
   DownloadDBEntry entry = CreateDownloadDBEntryFromItem(
       *download, download_source, fetch_error_body, request_header_type);
   AddOrReplaceEntry(entry);
@@ -166,7 +230,7 @@ void DownloadDBCache::OnDownloadDBEntriesLoaded(
     initialized_ = true;
     for (auto& entry : *entries) {
       CleanUpInProgressEntry(entry);
-      entries_.emplace(entry.download_info->guid, entry);
+      entries_[entry.download_info->guid] = entry;
     }
     std::move(callback).Run(std::move(entries));
   }
@@ -174,4 +238,9 @@ void DownloadDBCache::OnDownloadDBEntriesLoaded(
   // http://crbug.com/847661.
 }
 
+void DownloadDBCache::SetTimerTaskRunnerForTesting(
+    scoped_refptr<base::SequencedTaskRunner> task_runner) {
+  update_timer_.SetTaskRunner(task_runner);
+}
+
 }  //  namespace download

components/download/internal/common/download_db_cache.h

@@ -7,11 +7,13 @@
 
 #include <map>
 #include <memory>
+#include <set>
 #include <vector>
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
+#include "base/timer/timer.h"
 #include "components/download/public/common/download_export.h"
 #include "components/download/public/common/download_item.h"
 
@@ -41,6 +43,9 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadDBCache
   friend class DownloadDBCacheTest;
   friend class InProgressDownloadManager;
 
+  // Update all the entries in |download_db_|.
+  void UpdateDownloadDB();
+
   // DownloadItem::Observer
   void OnDownloadUpdated(DownloadItem* download) override;
   void OnDownloadRemoved(DownloadItem* download) override;
@@ -54,6 +59,9 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadDBCache
       bool success,
       std::unique_ptr<std::vector<DownloadDBEntry>> entries);
 
+  void SetTimerTaskRunnerForTesting(
+      scoped_refptr<base::SequencedTaskRunner> task_runner);
+
   // Whether this object has already been initialized.
   bool initialized_;
 
@@ -64,6 +72,12 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadDBCache
   // All in progress downloads stored in |download_db_|.
   DownloadDBEntryMap entries_;
 
+  // GUIDs of updated entries.
+  std::set<std::string> updated_guids_;
+
+  // Used to trigger db updates.
+  base::OneShotTimer update_timer_;
+
   base::WeakPtrFactory<DownloadDBCache> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadDBCache);

components/download/internal/common/download_db_cache_unittest.cc

@@ -8,6 +8,9 @@
 
 #include "base/bind.h"
 #include "base/guid.h"
+#include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
+#include "base/test/test_mock_time_task_runner.h"
 #include "components/download/database/download_db_conversions.h"
 #include "components/download/database/download_db_entry.h"
 #include "components/download/database/download_db_impl.h"
@@ -39,7 +42,8 @@ std::string GetKey(const std::string& guid) {
 
 class DownloadDBCacheTest : public testing::Test {
  public:
-  DownloadDBCacheTest() : db_(nullptr) {}
+  DownloadDBCacheTest()
+      : db_(nullptr), task_runner_(new base::TestMockTimeTaskRunner) {}
 
   ~DownloadDBCacheTest() override = default;
 
@@ -52,6 +56,7 @@ class DownloadDBCacheTest : public testing::Test {
         DownloadNamespace::NAMESPACE_BROWSER_DOWNLOAD,
         base::FilePath(FILE_PATH_LITERAL("/test/db/fakepath")), std::move(db));
     db_cache_ = std::make_unique<DownloadDBCache>(std::move(download_db));
+    db_cache_->SetTimerTaskRunnerForTesting(task_runner_);
   }
 
   void InitCallback(std::vector<DownloadDBEntry>* loaded_entries,
@@ -81,6 +86,8 @@ class DownloadDBCacheTest : public testing::Test {
   std::map<std::string, download_pb::DownloadDBEntry> db_entries_;
   leveldb_proto::test::FakeDB<download_pb::DownloadDBEntry>* db_;
   std::unique_ptr<DownloadDBCache> db_cache_;
+  scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
   DISALLOW_COPY_AND_ASSIGN(DownloadDBCacheTest);
 };
 
@@ -103,7 +110,8 @@ TEST_F(DownloadDBCacheTest, InitializeAndRetrieve) {
   }
 }
 
-TEST_F(DownloadDBCacheTest, AddOrReplace) {
+// Test that new entry is added immediately to the database
+TEST_F(DownloadDBCacheTest, AddNewEntry) {
   PrepopulateSampleEntries();
   CreateDBCache();
   std::vector<DownloadDBEntry> loaded_entries;
@@ -117,6 +125,84 @@ TEST_F(DownloadDBCacheTest, AddOrReplace) {
   DownloadDBEntry new_entry = CreateDownloadDBEntry();
   db_cache_->AddOrReplaceEntry(new_entry);
   ASSERT_EQ(new_entry, db_cache_->RetrieveEntry(new_entry.GetGuid()));
+  db_->UpdateCallback(true);
+  loaded_entries.clear();
+  DownloadDB* download_db = GetDownloadDB();
+  download_db->LoadEntries(base::BindOnce(&DownloadDBCacheTest::InitCallback,
+                                          base::Unretained(this),
+                                          &loaded_entries));
+  db_->LoadCallback(true);
+  ASSERT_EQ(loaded_entries.size(), 3u);
+}
+
+// Test that modifying an existing entry could take some time to update the DB.
+TEST_F(DownloadDBCacheTest, ModifyExistingEntry) {
+  PrepopulateSampleEntries();
+  CreateDBCache();
+  std::vector<DownloadDBEntry> loaded_entries;
+  db_cache_->Initialize(base::BindOnce(&DownloadDBCacheTest::InitCallback,
+                                       base::Unretained(this), &loaded_entries,
+                                       true));
+  db_->InitCallback(true);
+  db_->LoadCallback(true);
+  ASSERT_EQ(loaded_entries.size(), 2u);
+
+  loaded_entries[0].download_info->id = 100;
+  loaded_entries[1].download_info->id = 100;
+  db_cache_->AddOrReplaceEntry(loaded_entries[0]);
+  db_cache_->AddOrReplaceEntry(loaded_entries[1]);
+
+  ASSERT_EQ(task_runner_->GetPendingTaskCount(), 1u);
+  ASSERT_GT(task_runner_->NextPendingTaskDelay(), base::TimeDelta());
+  task_runner_->FastForwardUntilNoTasksRemain();
+  db_->UpdateCallback(true);
+
+  loaded_entries.clear();
+  DownloadDB* download_db = GetDownloadDB();
+  download_db->LoadEntries(base::BindOnce(&DownloadDBCacheTest::InitCallback,
+                                          base::Unretained(this),
+                                          &loaded_entries));
+  db_->LoadCallback(true);
+  ASSERT_EQ(loaded_entries.size(), 2u);
+  ASSERT_EQ(loaded_entries[0].download_info->id, 100);
+  ASSERT_EQ(loaded_entries[1].download_info->id, 100);
+}
+
+// Test that modifying current path will immediately update the DB.
+TEST_F(DownloadDBCacheTest, FilePathChange) {
+  DownloadDBEntry entry = CreateDownloadDBEntry();
+  InProgressInfo info;
+  base::FilePath test_path = base::FilePath(FILE_PATH_LITERAL("/tmp"));
+  info.current_path = test_path;
+  entry.download_info->in_progress_info = info;
+  db_entries_.insert(
+      std::make_pair(GetKey(entry.GetGuid()),
+                     DownloadDBConversions::DownloadDBEntryToProto(entry)));
+  CreateDBCache();
+  std::vector<DownloadDBEntry> loaded_entries;
+  db_cache_->Initialize(base::BindOnce(&DownloadDBCacheTest::InitCallback,
+                                       base::Unretained(this), &loaded_entries,
+                                       true));
+  db_->InitCallback(true);
+  db_->LoadCallback(true);
+  ASSERT_EQ(loaded_entries.size(), 1u);
+  ASSERT_EQ(loaded_entries[0].download_info->in_progress_info->current_path,
+            test_path);
+
+  test_path = base::FilePath(FILE_PATH_LITERAL("/test"));
+  loaded_entries[0].download_info->in_progress_info->current_path = test_path;
+  db_cache_->AddOrReplaceEntry(loaded_entries[0]);
+  db_->UpdateCallback(true);
+
+  loaded_entries.clear();
+  DownloadDB* download_db = GetDownloadDB();
+  download_db->LoadEntries(base::BindOnce(&DownloadDBCacheTest::InitCallback,
+                                          base::Unretained(this),
+                                          &loaded_entries));
+  db_->LoadCallback(true);
+  ASSERT_EQ(loaded_entries.size(), 1u);
+  ASSERT_EQ(loaded_entries[0].download_info->in_progress_info->current_path,
+            test_path);
 }
 
 TEST_F(DownloadDBCacheTest, RemoveEntry) {