Sync: Avoid 3 passes over SyncDarta DB when loading the directory fro…

…m the disk

According to profiling the first two read passes over SyncData DB
are from DirectoryBackingStore::DropDeletedEntries. This function
executes two SQL queries that drop entries pending deletion.
Since it is uncommon to have entries pending deletion it is far
more efficient to remove DirectoryBackingStore::DropDeletedEntries
and instead check for the entries matching this criteria inside
DirectoryBackingStore::LoadEntries, skip those entries from being
loaded and put them straight into Directory's metahandles_to_purge
collection which sets them up for deletion during a subsequent database
save.

See the bug for the preliminary performance results.

BUG=464073

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

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

sync/syncable/deferred_on_disk_directory_backing_store.cc

@@ -52,6 +52,7 @@ bool DeferredOnDiskDirectoryBackingStore::SaveChanges(
 DirOpenResult DeferredOnDiskDirectoryBackingStore::Load(
     Directory::MetahandlesMap* handles_map,
     JournalIndex* delete_journals,
+    MetahandleSet* metahandles_to_purge,
     Directory::KernelLoadInfo* kernel_load_info) {
   // Open an in-memory database at first to create initial sync data needed by
   // Directory.
@@ -61,7 +62,7 @@ DirOpenResult DeferredOnDiskDirectoryBackingStore::Load(
 
   if (!InitializeTables())
     return FAILED_OPEN_DATABASE;
-  if (!LoadEntries(handles_map))
+  if (!LoadEntries(handles_map, metahandles_to_purge))
     return FAILED_DATABASE_CORRUPT;
   if (!LoadInfo(kernel_load_info))
     return FAILED_DATABASE_CORRUPT;

sync/syncable/deferred_on_disk_directory_backing_store.h

@@ -25,6 +25,7 @@ class SYNC_EXPORT_PRIVATE DeferredOnDiskDirectoryBackingStore
   ~DeferredOnDiskDirectoryBackingStore() override;
   DirOpenResult Load(Directory::MetahandlesMap* handles_map,
                      JournalIndex* delete_journals,
+                     MetahandleSet* metahandles_to_purge,
                      Directory::KernelLoadInfo* kernel_load_info) override;
   bool SaveChanges(const Directory::SaveChangesSnapshot& snapshot) override;
 

sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc

@@ -31,15 +31,17 @@ class DeferredOnDiskDirectoryBackingStoreTest : public testing::Test {
   base::FilePath db_path_;
   Directory::MetahandlesMap handles_map_;
   JournalIndex delete_journals_;
+  MetahandleSet metahandles_to_purge_;
   Directory::KernelLoadInfo kernel_load_info_;
 };
 
 // Test initialization of root entry when calling Load().
 TEST_F(DeferredOnDiskDirectoryBackingStoreTest, Load) {
   DeferredOnDiskDirectoryBackingStore store("test", db_path_);
   EXPECT_EQ(OPENED, store.Load(&handles_map_, &delete_journals_,
-                               &kernel_load_info_));
+                               &metahandles_to_purge_, &kernel_load_info_));
   EXPECT_TRUE(delete_journals_.empty());
+  EXPECT_TRUE(metahandles_to_purge_.empty());
   ASSERT_EQ(1u, handles_map_.size());   // root node
   ASSERT_TRUE(handles_map_.count(1));
   EntryKernel* root = handles_map_[1];
@@ -55,7 +57,7 @@ TEST_F(DeferredOnDiskDirectoryBackingStoreTest,
     // Open and close.
     DeferredOnDiskDirectoryBackingStore store("test", db_path_);
     EXPECT_EQ(OPENED, store.Load(&handles_map_, &delete_journals_,
-                                 &kernel_load_info_));
+                                 &metahandles_to_purge_, &kernel_load_info_));
   }
 
   EXPECT_FALSE(base::PathExists(db_path_));
@@ -68,7 +70,7 @@ TEST_F(DeferredOnDiskDirectoryBackingStoreTest,
     // Open and close.
     DeferredOnDiskDirectoryBackingStore store("test", db_path_);
     EXPECT_EQ(OPENED, store.Load(&handles_map_, &delete_journals_,
-                                 &kernel_load_info_));
+                                 &metahandles_to_purge_, &kernel_load_info_));
 
     Directory::SaveChangesSnapshot snapshot;
     store.SaveChanges(snapshot);
@@ -83,7 +85,7 @@ TEST_F(DeferredOnDiskDirectoryBackingStoreTest, PersistWhenSavingValidChanges) {
     // Open and close.
     DeferredOnDiskDirectoryBackingStore store("test", db_path_);
     EXPECT_EQ(OPENED, store.Load(&handles_map_, &delete_journals_,
-                                 &kernel_load_info_));
+                                 &metahandles_to_purge_, &kernel_load_info_));
 
     Directory::SaveChangesSnapshot snapshot;
     EntryKernel* entry = new EntryKernel();
@@ -98,8 +100,9 @@ TEST_F(DeferredOnDiskDirectoryBackingStoreTest, PersistWhenSavingValidChanges) {
 
   ASSERT_TRUE(base::PathExists(db_path_));
   OnDiskDirectoryBackingStore read_store("test", db_path_);
-  EXPECT_EQ(OPENED, read_store.Load(&handles_map_, &delete_journals_,
-                                    &kernel_load_info_));
+  EXPECT_EQ(OPENED,
+            read_store.Load(&handles_map_, &delete_journals_,
+                            &metahandles_to_purge_, &kernel_load_info_));
   ASSERT_EQ(2u, handles_map_.size());
   ASSERT_TRUE(handles_map_.count(1));     // root node
   ASSERT_TRUE(handles_map_.count(2));

sync/syncable/directory.cc

@@ -77,7 +77,8 @@ Directory::SaveChangesSnapshot::~SaveChangesSnapshot() {
 
 Directory::Kernel::Kernel(
     const std::string& name,
-    const KernelLoadInfo& info, DirectoryChangeDelegate* delegate,
+    const KernelLoadInfo& info,
+    DirectoryChangeDelegate* delegate,
     const WeakHandle<TransactionObserver>& transaction_observer)
     : next_write_transaction_id(0),
       name(name),
@@ -178,12 +179,13 @@ DirOpenResult Directory::OpenImpl(
 
   // Avoids mem leaks on failure.  Harmlessly deletes the empty hash map after
   // the swap in the success case.
-  STLValueDeleter<Directory::MetahandlesMap> deleter(&tmp_handles_map);
+  STLValueDeleter<MetahandlesMap> deleter(&tmp_handles_map);
 
   JournalIndex delete_journals;
+  MetahandleSet metahandles_to_purge;
 
-  DirOpenResult result =
-      store_->Load(&tmp_handles_map, &delete_journals, &info);
+  DirOpenResult result = store_->Load(&tmp_handles_map, &delete_journals,
+                                      &metahandles_to_purge, &info);
   if (OPENED != result)
     return result;
 
@@ -195,6 +197,8 @@ DirOpenResult Directory::OpenImpl(
   // prevent local ID reuse in the case of an early crash.  See the comments in
   // TakeSnapshotForSaveChanges() or crbug.com/142987 for more information.
   kernel_->info_status = KERNEL_SHARE_INFO_DIRTY;
+
+  kernel_->metahandles_to_purge.swap(metahandles_to_purge);
   if (!SaveChanges())
     return FAILED_INITIAL_WRITE;
 

sync/syncable/directory.h

@@ -599,11 +599,12 @@ class SYNC_EXPORT Directory {
   // processed |snapshot| failed, for example, due to no disk space.
   void HandleSaveChangesFailure(const SaveChangesSnapshot& snapshot);
 
-  // Used by CheckTreeInvariants
+  // Used by CheckTreeInvariants.
   void GetAllMetaHandles(BaseTransaction* trans, MetahandleSet* result);
+
+  // Used by VacuumAfterSaveChanges.
   bool SafeToPurgeFromMemory(WriteTransaction* trans,
                              const EntryKernel* const entry) const;
-
   // A helper used by GetTotalNodeCount.
   void GetChildSetForKernel(
       BaseTransaction*,

sync/syncable/directory_backing_store.cc

@@ -538,8 +538,8 @@ bool DirectoryBackingStore::RefreshColumns() {
   return true;
 }
 
-bool DirectoryBackingStore::LoadEntries(
-    Directory::MetahandlesMap* handles_map) {
+bool DirectoryBackingStore::LoadEntries(Directory::MetahandlesMap* handles_map,
+                                        MetahandleSet* metahandles_to_purge) {
   string select;
   select.reserve(kUpdateStatementBufferSize);
   select.append("SELECT ");
@@ -555,11 +555,25 @@ bool DirectoryBackingStore::LoadEntries(
       return false;
 
     int64 handle = kernel->ref(META_HANDLE);
-    (*handles_map)[handle] = kernel.release();
+    if (SafeToPurgeOnLoading(*kernel))
+      metahandles_to_purge->insert(handle);
+    else
+      (*handles_map)[handle] = kernel.release();
   }
   return s.Succeeded();
 }
 
+bool DirectoryBackingStore::SafeToPurgeOnLoading(
+    const EntryKernel& entry) const {
+  if (entry.ref(IS_DEL)) {
+    if (!entry.ref(IS_UNSYNCED) && !entry.ref(IS_UNAPPLIED_UPDATE))
+      return true;
+    else if (!entry.ref(ID).ServerKnows())
+      return true;
+  }
+  return false;
+}
+
 bool DirectoryBackingStore::LoadDeleteJournals(
     JournalIndex* delete_journals) {
   string select;
@@ -643,21 +657,6 @@ bool DirectoryBackingStore::SaveEntryToDB(sql::Statement* save_statement,
   return save_statement->Run();
 }
 
-bool DirectoryBackingStore::DropDeletedEntries() {
-  if (!db_->Execute("DELETE FROM metas "
-                    "WHERE is_del > 0 "
-                    "AND is_unsynced < 1 "
-                    "AND is_unapplied_update < 1")) {
-    return false;
-  }
-  if (!db_->Execute("DELETE FROM metas "
-                    "WHERE is_del > 0 "
-                    "AND id LIKE 'c%'")) {
-    return false;
-  }
-  return true;
-}
-
 bool DirectoryBackingStore::SafeDropTable(const char* table_name) {
   string query = "DROP TABLE IF EXISTS ";
   query.append(table_name);

sync/syncable/directory_backing_store.h

@@ -49,16 +49,18 @@ class SYNC_EXPORT_PRIVATE DirectoryBackingStore : public base::NonThreadSafe {
   virtual ~DirectoryBackingStore();
 
   // Loads and drops all currently persisted meta entries into |handles_map|
-  // and loads appropriate persisted kernel info into |info_bucket|.
+  // and loads appropriate persisted kernel info into |kernel_load_info|.
+  // The function determines which entries can be safely dropped and inserts
+  // their keys into |metahandles_to_purge|. It is up to the caller to
+  // perform the actual cleanup.
   //
-  // This function can perform some cleanup tasks behind the scenes.  It will
-  // clean up unused entries from the database and migrate to the latest
-  // database version.  The caller can safely ignore these details.
+  // This function will migrate to the latest database version.
   //
   // NOTE: On success (return value of OPENED), the buckets are populated with
   // newly allocated items, meaning ownership is bestowed upon the caller.
   virtual DirOpenResult Load(Directory::MetahandlesMap* handles_map,
                              JournalIndex* delete_journals,
+                             MetahandleSet* metahandles_to_purge,
                              Directory::KernelLoadInfo* kernel_load_info) = 0;
 
   // Updates the on-disk store with the input |snapshot| as a database
@@ -90,16 +92,15 @@ class SYNC_EXPORT_PRIVATE DirectoryBackingStore : public base::NonThreadSafe {
   bool CreateV75ModelsTable();
   bool CreateV81ModelsTable();
 
-  // We don't need to load any synced and applied deleted entries, we can
-  // in fact just purge them forever on startup.
-  bool DropDeletedEntries();
   // Drops a table if it exists, harmless if the table did not already exist.
   bool SafeDropTable(const char* table_name);
 
   // Load helpers for entries and attributes.
-  bool LoadEntries(Directory::MetahandlesMap* handles_map);
+  bool LoadEntries(Directory::MetahandlesMap* handles_map,
+                   MetahandleSet* metahandles_to_purge);
   bool LoadDeleteJournals(JournalIndex* delete_journals);
   bool LoadInfo(Directory::KernelLoadInfo* info);
+  bool SafeToPurgeOnLoading(const EntryKernel& entry) const;
 
   // Save/update helpers for entries.  Return false if sqlite commit fails.
   static bool SaveEntryToDB(sql::Statement* save_statement,

sync/syncable/directory_backing_store_unittest.cc

@@ -46,10 +46,11 @@ class MigrationTest : public testing::TestWithParam<int> {
   static bool LoadAndIgnoreReturnedData(DirectoryBackingStore *dbs) {
     Directory::MetahandlesMap tmp_handles_map;
     JournalIndex  delete_journals;
+    MetahandleSet metahandles_to_purge;
     STLValueDeleter<Directory::MetahandlesMap> deleter(&tmp_handles_map);
     Directory::KernelLoadInfo kernel_load_info;
-    return dbs->Load(&tmp_handles_map, &delete_journals, &kernel_load_info) ==
-        OPENED;
+    return dbs->Load(&tmp_handles_map, &delete_journals, &metahandles_to_purge,
+                     &kernel_load_info) == OPENED;
   }
 
   void SetUpVersion67Database(sql::Connection* connection);
@@ -3202,12 +3203,14 @@ TEST_F(DirectoryBackingStoreTest, MigrateVersion78To79) {
 
   // Ensure the next_id has been incremented.
   Directory::MetahandlesMap handles_map;
-  JournalIndex  delete_journals;;
+  JournalIndex delete_journals;
+  MetahandleSet metahandles_to_purge;
   STLValueDeleter<Directory::MetahandlesMap> deleter(&handles_map);
   Directory::KernelLoadInfo load_info;
 
   s.Clear();
-  ASSERT_TRUE(dbs->Load(&handles_map, &delete_journals, &load_info));
+  ASSERT_TRUE(dbs->Load(&handles_map, &delete_journals, &metahandles_to_purge,
+                        &load_info));
   EXPECT_LE(load_info.kernel_info.next_id, kInitialNextId - 65536);
 }
 
@@ -3225,11 +3228,13 @@ TEST_F(DirectoryBackingStoreTest, MigrateVersion79To80) {
 
   // Ensure the bag_of_chips has been set.
   Directory::MetahandlesMap handles_map;
-  JournalIndex  delete_journals;;
+  JournalIndex delete_journals;
+  MetahandleSet metahandles_to_purge;
   STLValueDeleter<Directory::MetahandlesMap> deleter(&handles_map);
   Directory::KernelLoadInfo load_info;
 
-  ASSERT_TRUE(dbs->Load(&handles_map, &delete_journals, &load_info));
+  ASSERT_TRUE(dbs->Load(&handles_map, &delete_journals, &metahandles_to_purge,
+                        &load_info));
   // Check that the initial value is the serialization of an empty ChipBag.
   sync_pb::ChipBag chip_bag;
   std::string serialized_chip_bag;
@@ -3438,11 +3443,13 @@ TEST_F(DirectoryBackingStoreTest, DetectInvalidPosition) {
 
   // Trying to unpack this entry should signal that the DB is corrupted.
   Directory::MetahandlesMap handles_map;
-  JournalIndex  delete_journals;;
+  JournalIndex delete_journals;
+  MetahandleSet metahandles_to_purge;
   STLValueDeleter<Directory::MetahandlesMap> deleter(&handles_map);
   Directory::KernelLoadInfo kernel_load_info;
   ASSERT_EQ(FAILED_DATABASE_CORRUPT,
-            dbs->Load(&handles_map, &delete_journals, &kernel_load_info));
+            dbs->Load(&handles_map, &delete_journals, &metahandles_to_purge,
+                      &kernel_load_info));
 }
 
 TEST_P(MigrationTest, ToCurrentVersion) {
@@ -3529,13 +3536,17 @@ TEST_P(MigrationTest, ToCurrentVersion) {
 
   syncable::Directory::KernelLoadInfo dir_info;
   Directory::MetahandlesMap handles_map;
-  JournalIndex  delete_journals;;
+  JournalIndex delete_journals;
+  MetahandleSet metahandles_to_purge;
   STLValueDeleter<Directory::MetahandlesMap> index_deleter(&handles_map);
 
   {
     scoped_ptr<TestDirectoryBackingStore> dbs(
         new TestDirectoryBackingStore(GetUsername(), &connection));
-    ASSERT_EQ(OPENED, dbs->Load(&handles_map, &delete_journals, &dir_info));
+    ASSERT_EQ(OPENED, dbs->Load(&handles_map, &delete_journals,
+                                &metahandles_to_purge, &dir_info));
+    if (!metahandles_to_purge.empty())
+      dbs->DeleteEntries(metahandles_to_purge);
     ASSERT_FALSE(dbs->needs_column_refresh_);
     ASSERT_EQ(kCurrentDBVersion, dbs->GetVersion());
   }
@@ -3903,20 +3914,22 @@ TEST_F(DirectoryBackingStoreTest, DeleteEntries) {
       new TestDirectoryBackingStore(GetUsername(), &connection));
   Directory::MetahandlesMap handles_map;
   JournalIndex  delete_journals;
+  MetahandleSet metahandles_to_purge;
   Directory::KernelLoadInfo kernel_load_info;
   STLValueDeleter<Directory::MetahandlesMap> index_deleter(&handles_map);
 
-  dbs->Load(&handles_map, &delete_journals, &kernel_load_info);
+  dbs->Load(&handles_map, &delete_journals, &metahandles_to_purge,
+            &kernel_load_info);
   size_t initial_size = handles_map.size();
   ASSERT_LT(0U, initial_size) << "Test requires handles_map to delete.";
   int64 first_to_die = handles_map.begin()->second->ref(META_HANDLE);
   MetahandleSet to_delete;
   to_delete.insert(first_to_die);
-  EXPECT_TRUE(dbs->DeleteEntries(TestDirectoryBackingStore::METAS_TABLE,
-                                 to_delete));
+  EXPECT_TRUE(dbs->DeleteEntries(to_delete));
 
   STLDeleteValues(&handles_map);
-  dbs->LoadEntries(&handles_map);
+  metahandles_to_purge.clear();
+  dbs->LoadEntries(&handles_map, &metahandles_to_purge);
 
   EXPECT_EQ(initial_size - 1, handles_map.size());
   bool delete_failed = false;
@@ -3935,11 +3948,11 @@ TEST_F(DirectoryBackingStoreTest, DeleteEntries) {
     to_delete.insert(it->first);
   }
 
-  EXPECT_TRUE(dbs->DeleteEntries(TestDirectoryBackingStore::METAS_TABLE,
-                                 to_delete));
+  EXPECT_TRUE(dbs->DeleteEntries(to_delete));
 
   STLDeleteValues(&handles_map);
-  dbs->LoadEntries(&handles_map);
+  metahandles_to_purge.clear();
+  dbs->LoadEntries(&handles_map, &metahandles_to_purge);
   EXPECT_EQ(0U, handles_map.size());
 }