PinManager: Merge files_to_pin_ into files_to_track_

Instead of having two indexed collections files_to_pin_ and
files_to_track_, all the files are now indexed in files_to_track_.

As a consequence, files_to_pin_ is now a simple vector of stable IDs of
files that might be pinned at some point.

Bug: b:266348875
Test: chromeos_unittests --gtest_filter=DriveFsPinManagerTest.*
Change-Id: I3e01ab59c4a5221c80220d07305c4f8ebb74e707
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4197766
Reviewed-by: Ben Reich <benreich@chromium.org>
Commit-Queue: François Degros <fdegros@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1098468}

chromeos/ash/components/drivefs/drivefs_pin_manager.cc

@@ -6,8 +6,6 @@
 
 #include <locale>
 #include <type_traits>
-#include <utility>
-#include <vector>
 
 #include "base/files/file_path.h"
 #include "base/functional/bind.h"
@@ -301,28 +299,41 @@ Progress& Progress::operator=(const Progress&) = default;
 // queue.
 constexpr base::TimeDelta kPeriodicRemovalInterval = base::Seconds(10);
 
-bool PinManager::Add(const Id id, const std::string& path, const int64_t size) {
+bool PinManager::Add(const Id id,
+                     const std::string& path,
+                     const int64_t size,
+                     const bool pinned) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK_GE(size, 0) << " for " << id << " " << Quote(path);
 
-  const auto [it, ok] =
-      files_to_pin_.try_emplace(id, File{.path = path, .total = size});
+  const auto [it, ok] = files_to_track_.try_emplace(
+      id, File{.path = path, .total = size, .pinned = pinned});
   DCHECK_EQ(id, it->first);
   if (!ok) {
     LOG_IF(ERROR, !ok) << "Cannot add " << id << " " << Quote(path)
                        << " with size " << HumanReadableSize(size)
-                       << " to the files to pin: Conflicting entry "
+                       << " to the files to track: Conflicting entry "
                        << it->second;
     return false;
   }
 
   VLOG(3) << "Added " << id << " " << Quote(path) << " with size "
-          << HumanReadableSize(size) << " to the files to pin";
+          << HumanReadableSize(size) << " to the files to track";
   progress_.bytes_to_pin += size;
   progress_.required_space += RoundToBlockSize(size);
   progress_.files_to_pin++;
   DCHECK_EQ(static_cast<size_t>(progress_.files_to_pin),
-            files_to_pin_.size() + files_to_track_.size());
+            files_to_track_.size());
+
+  if (pinned) {
+    progress_.syncing_files++;
+    DCHECK_EQ(progress_.syncing_files, CountPinnedFiles());
+  } else {
+    files_to_pin_.push_back(id);
+    DCHECK_LE(files_to_pin_.size(),
+              static_cast<size_t>(progress_.files_to_pin));
+  }
+
   return true;
 }
 
@@ -343,10 +354,12 @@ bool PinManager::Remove(const Id id,
     Update(*it, path, transferred, transferred);
   }
 
+  if (it->second.pinned) {
+    progress_.syncing_files--;
+  }
+
   files_to_track_.erase(it);
-  progress_.syncing_files--;
-  DCHECK_EQ(static_cast<size_t>(progress_.syncing_files),
-            files_to_track_.size());
+  DCHECK_EQ(progress_.syncing_files, CountPinnedFiles());
   VLOG(3) << "Stopped tracking " << id << " " << Quote(path);
   return true;
 }
@@ -541,26 +554,13 @@ void PinManager::OnSearchResultForSizeCalculation(
 
       VLOG(1) << "Already pinned but not available offline yet: " << id << " "
               << Quote(path);
-      const auto [it, ok] = files_to_track_.try_emplace(
-          id, File{.path = path.value(), .total = size, .pinned = true});
-      DCHECK(ok);
-      DCHECK_EQ(it->first, id);
-      progress_.syncing_files++;
-      DCHECK_EQ(static_cast<size_t>(progress_.syncing_files),
-                files_to_track_.size());
-      progress_.bytes_to_pin += size;
-      progress_.required_space += RoundToBlockSize(size);
-      progress_.files_to_pin++;
-      DCHECK_EQ(static_cast<size_t>(progress_.files_to_pin),
-                files_to_pin_.size() + files_to_track_.size());
-      continue;
+    } else {
+      VLOG_IF(1, md.available_offline)
+          << "Not pinned yet but already available offline: " << id << " "
+          << Quote(path) << ": " << Quote(md);
     }
 
-    VLOG_IF(1, md.available_offline)
-        << "Not pinned yet but already available offline: " << id << " "
-        << Quote(path) << ": " << Quote(md);
-
-    Add(id, path.value(), size);
+    Add(id, path.value(), size, md.pinned);
   }
 
   NotifyProgress();
@@ -637,13 +637,14 @@ void PinManager::StartPinning() {
     return Complete(Stage::kSuccess);
   }
 
-  if (files_to_track_.empty() && files_to_pin_.empty()) {
+  if (files_to_track_.empty()) {
     VLOG(1) << "Nothing to pin or track";
+    DCHECK(files_to_pin_.empty());
+    DCHECK_EQ(progress_.files_to_pin, 0);
+    DCHECK_EQ(progress_.syncing_files, 0);
     return Complete(Stage::kSuccess);
   }
 
-  VLOG(1) << "Pinning and tracking "
-          << (files_to_pin_.size() + files_to_track_.size()) << " files...";
   timer_ = base::ElapsedTimer();
   progress_.stage = Stage::kSyncing;
   NotifyProgress();
@@ -665,27 +666,33 @@ void PinManager::PinSomeFiles() {
     return Complete(Stage::kSuccess);
   }
 
-  while (files_to_track_.size() < 50 && !files_to_pin_.empty()) {
-    Files::node_type node = files_to_pin_.extract(files_to_pin_.begin());
-    DCHECK(node);
-    const Id id = node.key();
-    File& file = node.mapped();
+  while (progress_.syncing_files < 50 && !files_to_pin_.empty()) {
+    const Id id = files_to_pin_.back();
+    files_to_pin_.pop_back();
+
+    const Files::iterator it = files_to_track_.find(id);
+    if (it == files_to_track_.end()) {
+      VLOG(2) << "Not tracked: " << id;
+      continue;
+    }
+
+    DCHECK_EQ(it->first, id);
+    File& file = it->second;
     const std::string& path = file.path;
 
+    if (file.pinned) {
+      VLOG(2) << "Already pinned: " << id << " " << Quote(path);
+      continue;
+    }
+
     VLOG(2) << "Pinning " << id << " " << Quote(path);
     drivefs_->SetPinnedByStableId(
         static_cast<int64_t>(id), true,
         base::BindOnce(&PinManager::OnFilePinned, GetWeakPtr(), id, path));
 
-    DCHECK(!file.pinned);
-    DCHECK(!file.in_progress);
     file.pinned = true;
-    const Files::insert_return_type ir =
-        files_to_track_.insert(std::move(node));
-    DCHECK(ir.inserted) << " for " << id << " " << path;
     progress_.syncing_files++;
-    DCHECK_EQ(static_cast<size_t>(progress_.syncing_files),
-              files_to_track_.size());
+    DCHECK_EQ(progress_.syncing_files, CountPinnedFiles());
   }
 
   VLOG(1) << "Progress "

chromeos/ash/components/drivefs/drivefs_pin_manager.h

@@ -5,6 +5,7 @@
 #ifndef CHROMEOS_ASH_COMPONENTS_DRIVEFS_DRIVEFS_PIN_MANAGER_H_
 #define CHROMEOS_ASH_COMPONENTS_DRIVEFS_DRIVEFS_PIN_MANAGER_H_
 
+#include <algorithm>
 #include <ostream>
 #include <utility>
 #include <vector>
@@ -203,7 +204,7 @@ class COMPONENT_EXPORT(CHROMEOS_ASH_COMPONENTS_DRIVEFS) PinManager
   }
 
  private:
-  // Struct keeping track of the progress of a file being synced.
+  // Progress of a file being synced or to be synced.
   struct File {
     // Path inside the Drive folder.
     // TODO(b/265209836) Remove this field when not needed anymore.
@@ -230,22 +231,21 @@ class COMPONENT_EXPORT(CHROMEOS_ASH_COMPONENTS_DRIVEFS) PinManager
 
   using Files = std::map<Id, File>;
 
-  // Adds an item to the files to pin.  Does nothing if an item with the same ID
-  // already exists in files_to_pin_. Updates the total number of bytes to
-  // transfer and the required space. Returns whether an item was actually
-  // added.
-  bool Add(Id id, const std::string& path, int64_t size);
+  // Adds an item to the files to track.  Does nothing if an item with the same
+  // ID already exists in the map. Updates the total number of bytes to transfer
+  // and the required space. Returns whether an item was actually added.
+  bool Add(Id id, const std::string& path, int64_t size, bool pinned);
 
-  // Removes an item from the map. Does nothing if the item is not in the map.
-  // Updates the total number of bytes transferred so far. If `transferred` is
-  // negative, use the total expected size. Returns whether an item was actually
-  // removed.
+  // Removes an item from the files to track. Does nothing if the item is not in
+  // the map. Updates the total number of bytes transferred so far. If
+  // `transferred` is negative, use the total expected size. Returns whether an
+  // item was actually removed.
   bool Remove(Id id, const std::string& path, int64_t transferred = -1);
 
-  // Updates an item in the map. Does nothing if the item is not in the map.
-  // Updates the total number of bytes transferred so far. Updates the required
-  // space. If `transferred` or `total` is negative, then the matching argument
-  // is ignored. Returns whether anything has actually been updated.
+  // Updates an item in the files to track. Does nothing if the item is not in
+  // the map. Updates the total number of bytes transferred so far. Updates the
+  // required space. If `transferred` or `total` is negative, then the matching
+  // argument is ignored. Returns whether anything has actually been updated.
   bool Update(Id id,
               const std::string& path,
               int64_t transferred,
@@ -300,6 +300,16 @@ class COMPONENT_EXPORT(CHROMEOS_ASH_COMPONENTS_DRIVEFS) PinManager
   // Report progress to all the observers.
   void NotifyProgress();
 
+  // Counts the files that have been marked as pinned and that are still being
+  // tracked. Should always be equal to progress_.syncing_files. For debugging
+  // only.
+  int CountPinnedFiles() const {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    return std::count_if(
+        files_to_track_.cbegin(), files_to_track_.cend(),
+        [](const Files::value_type& entry) { return entry.second.pinned; });
+  }
+
   SEQUENCE_CHECKER(sequence_checker_);
 
   // Should the feature actually pin files, or should it stop after checking the
@@ -321,8 +331,12 @@ class COMPONENT_EXPORT(CHROMEOS_ASH_COMPONENTS_DRIVEFS) PinManager
   mojo::Remote<mojom::SearchQuery> search_query_;
   base::ElapsedTimer timer_;
 
-  // Map that tracks the in-progress files indexed by their stable ID.
-  Files files_to_pin_ GUARDED_BY_CONTEXT(sequence_checker_);
+  // Stable IDs of the files to pin, and which are not already marked as pinned.
+  std::vector<Id> files_to_pin_ GUARDED_BY_CONTEXT(sequence_checker_);
+
+  // Map that tracks the in-progress files indexed by their stable ID. This
+  // contains all the files, either pinned or not, that are not completely
+  // cached yet.
   Files files_to_track_ GUARDED_BY_CONTEXT(sequence_checker_);
 
   base::WeakPtrFactory<PinManager> weak_ptr_factory_{this};

chromeos/ash/components/drivefs/drivefs_pin_manager_unittest.cc

@@ -48,6 +48,7 @@ using std::vector;
 using testing::_;
 using testing::AnyNumber;
 using testing::DoAll;
+using testing::ElementsAre;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Return;
@@ -265,24 +266,25 @@ TEST_F(DriveFsPinManagerTest, Add) {
   EXPECT_THAT(manager.files_to_track_, IsEmpty());
 
   // Add an item.
-  EXPECT_TRUE(manager.Add(id1, path1, size1));
-  EXPECT_THAT(manager.files_to_pin_, SizeIs(1));
-  EXPECT_THAT(manager.files_to_track_, IsEmpty());
+  EXPECT_TRUE(manager.Add(id1, path1, size1, false));
+  EXPECT_THAT(manager.files_to_pin_, ElementsAre(id1));
+  EXPECT_THAT(manager.files_to_track_, SizeIs(1));
 
   // Try to add a conflicting item with the same ID, but different path and
   // size.
-  EXPECT_FALSE(manager.Add(id1, path2, size2));
-  EXPECT_THAT(manager.files_to_pin_, SizeIs(1));
-  EXPECT_THAT(manager.files_to_track_, IsEmpty());
+  EXPECT_FALSE(manager.Add(id1, path2, size2, true));
+  EXPECT_THAT(manager.files_to_pin_, ElementsAre(id1));
+  EXPECT_THAT(manager.files_to_track_, SizeIs(1));
 
   {
-    const auto it = manager.files_to_pin_.find(id1);
-    ASSERT_NE(it, manager.files_to_pin_.end());
+    const auto it = manager.files_to_track_.find(id1);
+    ASSERT_NE(it, manager.files_to_track_.end());
     const auto& [id, file] = *it;
     EXPECT_EQ(id, id1);
     EXPECT_EQ(file.path, path1);
     EXPECT_EQ(file.total, size1);
     EXPECT_EQ(file.transferred, 0);
+    EXPECT_FALSE(file.pinned);
     EXPECT_FALSE(file.in_progress);
   }
 
@@ -292,22 +294,25 @@ TEST_F(DriveFsPinManagerTest, Add) {
     EXPECT_EQ(progress.pinned_bytes, 0);
     EXPECT_EQ(progress.bytes_to_pin, size1);
     EXPECT_EQ(progress.required_space, 698249216);
+    EXPECT_EQ(progress.syncing_files, 0);
+    EXPECT_EQ(progress.files_to_pin, 1);
   }
 
-  // Add a second item.
-  EXPECT_TRUE(manager.Add(id2, path2, size2));
-  EXPECT_THAT(manager.files_to_pin_, SizeIs(2));
-  EXPECT_THAT(manager.files_to_track_, IsEmpty());
+  // Add a second item, but which is already pinned this time.
+  EXPECT_TRUE(manager.Add(id2, path2, size2, true));
+  EXPECT_THAT(manager.files_to_pin_, ElementsAre(id1));
+  EXPECT_THAT(manager.files_to_track_, SizeIs(2));
 
   {
-    const auto it = manager.files_to_pin_.find(id2);
-    ASSERT_NE(it, manager.files_to_pin_.end());
+    const auto it = manager.files_to_track_.find(id2);
+    ASSERT_NE(it, manager.files_to_track_.end());
     const auto& [id, file] = *it;
     EXPECT_EQ(id, id2);
     EXPECT_EQ(file.path, path2);
     EXPECT_EQ(file.total, size2);
     EXPECT_EQ(file.transferred, 0);
     EXPECT_FALSE(file.in_progress);
+    EXPECT_TRUE(file.pinned);
   }
 
   {
@@ -316,6 +321,8 @@ TEST_F(DriveFsPinManagerTest, Add) {
     EXPECT_EQ(progress.pinned_bytes, 0);
     EXPECT_EQ(progress.bytes_to_pin, size1 + size2);
     EXPECT_EQ(progress.required_space, 777216000);
+    EXPECT_EQ(progress.syncing_files, 1);
+    EXPECT_EQ(progress.files_to_pin, 2);
   }
 }
 
@@ -548,6 +555,7 @@ TEST_F(DriveFsPinManagerTest, Remove) {
         id1, PinManager::File{.path = path1,
                               .transferred = 1200,
                               .total = 3000,
+                              .pinned = true,
                               .in_progress = true});
     ASSERT_TRUE(ok);
     manager.progress_.syncing_files++;
@@ -600,9 +608,9 @@ TEST_F(DriveFsPinManagerTest, Remove) {
         id1, PinManager::File{.path = path1,
                               .transferred = 1200,
                               .total = 3000,
+                              .pinned = false,
                               .in_progress = true});
     ASSERT_TRUE(ok);
-    manager.progress_.syncing_files++;
   }
 
   EXPECT_THAT(manager.files_to_track_, SizeIs(1));
@@ -626,6 +634,7 @@ TEST_F(DriveFsPinManagerTest, Remove) {
         id1, PinManager::File{.path = path1,
                               .transferred = 5000,
                               .total = 6000,
+                              .pinned = true,
                               .in_progress = true});
     ASSERT_TRUE(ok);
     manager.progress_.syncing_files++;
@@ -673,13 +682,13 @@ TEST_F(DriveFsPinManagerTest, OnSyncingEvent) {
   // Put in place a couple of files to track.
   {
     const auto [it, ok] = manager.files_to_track_.try_emplace(
-        id1, PinManager::File{.path = path1, .total = 10000});
+        id1, PinManager::File{.path = path1, .total = 10000, .pinned = true});
     ASSERT_TRUE(ok);
     manager.progress_.syncing_files++;
   }
   {
     const auto [it, ok] = manager.files_to_track_.try_emplace(
-        id2, PinManager::File{.path = path2, .total = 20000});
+        id2, PinManager::File{.path = path2, .total = 20000, .pinned = true});
     ASSERT_TRUE(ok);
     manager.progress_.syncing_files++;
   }
@@ -740,6 +749,7 @@ TEST_F(DriveFsPinManagerTest, OnSyncingEvent) {
     EXPECT_EQ(file.path, path1);
     EXPECT_EQ(file.total, 10000);
     EXPECT_EQ(file.transferred, 0);
+    EXPECT_TRUE(file.pinned);
     EXPECT_TRUE(file.in_progress);
   }
 
@@ -775,6 +785,7 @@ TEST_F(DriveFsPinManagerTest, OnSyncingEvent) {
     EXPECT_EQ(file.path, path1);
     EXPECT_EQ(file.total, 10000);
     EXPECT_EQ(file.transferred, 5000);
+    EXPECT_TRUE(file.pinned);
     EXPECT_TRUE(file.in_progress);
   }