diff options
author | stanisc <stanisc@chromium.org> | 2015-03-16 21:40:19 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-03-17 04:40:48 +0000 |
commit | a3ed68bbb72ec9fdf16d866f41cb0951d006088b (patch) | |
tree | f98fe2e5bf32c5652167782e64967a04a8fa0a96 /sync/syncable | |
parent | 69ca66410b63d9fd7ce1ef620541783233349362 (diff) | |
download | chromium_src-a3ed68bbb72ec9fdf16d866f41cb0951d006088b.zip chromium_src-a3ed68bbb72ec9fdf16d866f41cb0951d006088b.tar.gz chromium_src-a3ed68bbb72ec9fdf16d866f41cb0951d006088b.tar.bz2 |
Sync: Avoid 3 passes over SyncDarta DB when loading the directory from 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}
Diffstat (limited to 'sync/syncable')
-rw-r--r-- | sync/syncable/deferred_on_disk_directory_backing_store.cc | 3 | ||||
-rw-r--r-- | sync/syncable/deferred_on_disk_directory_backing_store.h | 1 | ||||
-rw-r--r-- | sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc | 15 | ||||
-rw-r--r-- | sync/syncable/directory.cc | 12 | ||||
-rw-r--r-- | sync/syncable/directory.h | 5 | ||||
-rw-r--r-- | sync/syncable/directory_backing_store.cc | 35 | ||||
-rw-r--r-- | sync/syncable/directory_backing_store.h | 17 | ||||
-rw-r--r-- | sync/syncable/directory_backing_store_unittest.cc | 47 | ||||
-rw-r--r-- | sync/syncable/directory_unittest.cc | 81 | ||||
-rw-r--r-- | sync/syncable/in_memory_directory_backing_store.cc | 5 | ||||
-rw-r--r-- | sync/syncable/in_memory_directory_backing_store.h | 1 | ||||
-rw-r--r-- | sync/syncable/invalid_directory_backing_store.cc | 1 | ||||
-rw-r--r-- | sync/syncable/invalid_directory_backing_store.h | 1 | ||||
-rw-r--r-- | sync/syncable/mutable_entry.cc | 6 | ||||
-rw-r--r-- | sync/syncable/on_disk_directory_backing_store.cc | 11 | ||||
-rw-r--r-- | sync/syncable/on_disk_directory_backing_store.h | 9 |
16 files changed, 179 insertions, 71 deletions
diff --git a/sync/syncable/deferred_on_disk_directory_backing_store.cc b/sync/syncable/deferred_on_disk_directory_backing_store.cc index 0367e5b..fbb7845 100644 --- a/sync/syncable/deferred_on_disk_directory_backing_store.cc +++ b/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; diff --git a/sync/syncable/deferred_on_disk_directory_backing_store.h b/sync/syncable/deferred_on_disk_directory_backing_store.h index c697120..b50da38 100644 --- a/sync/syncable/deferred_on_disk_directory_backing_store.h +++ b/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; diff --git a/sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc b/sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc index fbba75e..020eeb4 100644 --- a/sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc +++ b/sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc @@ -31,6 +31,7 @@ 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_; }; @@ -38,8 +39,9 @@ class DeferredOnDiskDirectoryBackingStoreTest : public testing::Test { 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)); diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc index 1261695..9a689ac 100644 --- a/sync/syncable/directory.cc +++ b/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; diff --git a/sync/syncable/directory.h b/sync/syncable/directory.h index 98706b6..89eec17 100644 --- a/sync/syncable/directory.h +++ b/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*, diff --git a/sync/syncable/directory_backing_store.cc b/sync/syncable/directory_backing_store.cc index f87933b..9d18a2d 100644 --- a/sync/syncable/directory_backing_store.cc +++ b/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); diff --git a/sync/syncable/directory_backing_store.h b/sync/syncable/directory_backing_store.h index 254fff4..36a2f0a 100644 --- a/sync/syncable/directory_backing_store.h +++ b/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, diff --git a/sync/syncable/directory_backing_store_unittest.cc b/sync/syncable/directory_backing_store_unittest.cc index 8f679f1..7394b0d 100644 --- a/sync/syncable/directory_backing_store_unittest.cc +++ b/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()); } diff --git a/sync/syncable/directory_unittest.cc b/sync/syncable/directory_unittest.cc index 5f8cbed..aa974d5 100644 --- a/sync/syncable/directory_unittest.cc +++ b/sync/syncable/directory_unittest.cc @@ -552,6 +552,87 @@ TEST_F(SyncableDirectoryTest, ManageDeleteJournals) { } } +TEST_F(SyncableDirectoryTest, TestPurgeDeletedEntriesOnReload) { + sync_pb::EntitySpecifics specifics; + AddDefaultFieldValue(PREFERENCES, &specifics); + + const int kClientCount = 2; + const int kServerCount = 5; + const int kTestCount = kClientCount + kServerCount; + int64 handles[kTestCount]; + + // The idea is to recreate various combinations of IDs, IS_DEL, + // IS_UNSYNCED, and IS_UNAPPLIED_UPDATE flags to test all combinations + // for DirectoryBackingStore::SafeToPurgeOnLoading. + // 0: client ID, IS_DEL, IS_UNSYNCED + // 1: client ID, IS_UNSYNCED + // 2: server ID, IS_DEL, IS_UNSYNCED, IS_UNAPPLIED_UPDATE + // 3: server ID, IS_DEL, IS_UNSYNCED + // 4: server ID, IS_DEL, IS_UNAPPLIED_UPDATE + // 5: server ID, IS_DEL + // 6: server ID + { + WriteTransaction trans(FROM_HERE, UNITTEST, dir().get()); + + for (int i = 0; i < kTestCount; i++) { + std::string name = base::StringPrintf("item%d", i); + MutableEntry item(&trans, CREATE, PREFERENCES, trans.root_id(), name); + ASSERT_TRUE(item.good()); + + handles[i] = item.GetMetahandle(); + + if (i < kClientCount) { + item.PutId(TestIdFactory::FromNumber(i - kClientCount)); + } else { + item.PutId(TestIdFactory::FromNumber(i)); + } + + item.PutUniqueClientTag(name); + item.PutIsUnsynced(true); + item.PutSpecifics(specifics); + item.PutServerSpecifics(specifics); + + if (i >= kClientCount) { + item.PutBaseVersion(10); + item.PutServerVersion(10); + } + + // Set flags + if (i != 1 && i != 6) + item.PutIsDel(true); + + if (i >= 4) + item.PutIsUnsynced(false); + + if (i == 2 || i == 4) + item.PutIsUnappliedUpdate(true); + } + } + ASSERT_EQ(OPENED, SimulateSaveAndReloadDir()); + + // Expect items 0 and 5 to be purged according to + // DirectoryBackingStore::SafeToPurgeOnLoading: + // - Item 0 is an item with IS_DEL flag and client ID. + // - Item 5 is an item with IS_DEL flag which has both + // IS_UNSYNCED and IS_UNAPPLIED_UPDATE unset. + std::vector<int64> expected_purged; + expected_purged.push_back(0); + expected_purged.push_back(5); + + std::vector<int64> actually_purged; + { + ReadTransaction trans(FROM_HERE, dir().get()); + for (int i = 0; i < kTestCount; i++) { + Entry item(&trans, GET_BY_HANDLE, handles[i]); + if (!item.good()) { + actually_purged.push_back(i); + } + } + } + + EXPECT_EQ(expected_purged, actually_purged); +} + TEST_F(SyncableDirectoryTest, TestBasicLookupNonExistantID) { ReadTransaction rtrans(FROM_HERE, dir().get()); Entry e(&rtrans, GET_BY_ID, TestIdFactory::FromNumber(-99)); diff --git a/sync/syncable/in_memory_directory_backing_store.cc b/sync/syncable/in_memory_directory_backing_store.cc index 57995f8..8f84baa 100644 --- a/sync/syncable/in_memory_directory_backing_store.cc +++ b/sync/syncable/in_memory_directory_backing_store.cc @@ -16,6 +16,7 @@ InMemoryDirectoryBackingStore::InMemoryDirectoryBackingStore( DirOpenResult InMemoryDirectoryBackingStore::Load( Directory::MetahandlesMap* handles_map, JournalIndex* delete_journals, + MetahandleSet* metahandles_to_purge, Directory::KernelLoadInfo* kernel_load_info) { if (!db_->is_open()) { if (!db_->OpenInMemory()) @@ -32,9 +33,7 @@ DirOpenResult InMemoryDirectoryBackingStore::Load( } } - if (!DropDeletedEntries()) - return FAILED_DATABASE_CORRUPT; - if (!LoadEntries(handles_map)) + if (!LoadEntries(handles_map, metahandles_to_purge)) return FAILED_DATABASE_CORRUPT; if (!LoadDeleteJournals(delete_journals)) return FAILED_DATABASE_CORRUPT; diff --git a/sync/syncable/in_memory_directory_backing_store.h b/sync/syncable/in_memory_directory_backing_store.h index 0a0f9ba..d22e65f 100644 --- a/sync/syncable/in_memory_directory_backing_store.h +++ b/sync/syncable/in_memory_directory_backing_store.h @@ -26,6 +26,7 @@ class SYNC_EXPORT_PRIVATE InMemoryDirectoryBackingStore explicit InMemoryDirectoryBackingStore(const std::string& dir_name); DirOpenResult Load(Directory::MetahandlesMap* handles_map, JournalIndex* delete_journals, + MetahandleSet* metahandles_to_purge, Directory::KernelLoadInfo* kernel_load_info) override; void request_consistent_cache_guid() { diff --git a/sync/syncable/invalid_directory_backing_store.cc b/sync/syncable/invalid_directory_backing_store.cc index 6345521..1844d84 100644 --- a/sync/syncable/invalid_directory_backing_store.cc +++ b/sync/syncable/invalid_directory_backing_store.cc @@ -17,6 +17,7 @@ InvalidDirectoryBackingStore::~InvalidDirectoryBackingStore() { DirOpenResult InvalidDirectoryBackingStore::Load( Directory::MetahandlesMap* handles_map, JournalIndex* delete_journals, + MetahandleSet* metahandles_to_purge, Directory::KernelLoadInfo* kernel_load_info) { return FAILED_OPEN_DATABASE; } diff --git a/sync/syncable/invalid_directory_backing_store.h b/sync/syncable/invalid_directory_backing_store.h index fd99653..2a6d065 100644 --- a/sync/syncable/invalid_directory_backing_store.h +++ b/sync/syncable/invalid_directory_backing_store.h @@ -19,6 +19,7 @@ class SYNC_EXPORT_PRIVATE InvalidDirectoryBackingStore ~InvalidDirectoryBackingStore() override; DirOpenResult Load(Directory::MetahandlesMap* handles_map, JournalIndex* delete_journals, + MetahandleSet* metahandles_to_purge, Directory::KernelLoadInfo* kernel_load_info) override; private: diff --git a/sync/syncable/mutable_entry.cc b/sync/syncable/mutable_entry.cc index 89c8b05..d627cf4 100644 --- a/sync/syncable/mutable_entry.cc +++ b/sync/syncable/mutable_entry.cc @@ -184,9 +184,9 @@ void MutableEntry::PutIsDel(bool value) { // - Ensure that the item is never committed to the server. // - Allow any items with the same UNIQUE_CLIENT_TAG created on other // clients to override this entry. - // - Let us delete this entry permanently through - // DirectoryBackingStore::DropDeletedEntries() when we next restart sync. - // This will save memory and avoid crbug.com/125381. + // - Let us delete this entry permanently when we next restart sync - see + // DirectoryBackingStore::SafeToPurgeOnLoading. + // This will avoid crbug.com/125381. // Note: do not unset IsUnsynced if the syncer is in the middle of // attempting to commit this entity. if (!GetId().ServerKnows() && !GetSyncing()) { diff --git a/sync/syncable/on_disk_directory_backing_store.cc b/sync/syncable/on_disk_directory_backing_store.cc index 1bebf9a..dcbaedd 100644 --- a/sync/syncable/on_disk_directory_backing_store.cc +++ b/sync/syncable/on_disk_directory_backing_store.cc @@ -37,6 +37,7 @@ OnDiskDirectoryBackingStore::~OnDiskDirectoryBackingStore() { } DirOpenResult OnDiskDirectoryBackingStore::TryLoad( Directory::MetahandlesMap* handles_map, JournalIndex* delete_journals, + MetahandleSet* metahandles_to_purge, Directory::KernelLoadInfo* kernel_load_info) { DCHECK(CalledOnValidThread()); if (!db_->is_open()) { @@ -47,9 +48,7 @@ DirOpenResult OnDiskDirectoryBackingStore::TryLoad( if (!InitializeTables()) return FAILED_OPEN_DATABASE; - if (!DropDeletedEntries()) - return FAILED_DATABASE_CORRUPT; - if (!LoadEntries(handles_map)) + if (!LoadEntries(handles_map, metahandles_to_purge)) return FAILED_DATABASE_CORRUPT; if (!LoadDeleteJournals(delete_journals)) return FAILED_DATABASE_CORRUPT; @@ -65,9 +64,10 @@ DirOpenResult OnDiskDirectoryBackingStore::TryLoad( DirOpenResult OnDiskDirectoryBackingStore::Load( Directory::MetahandlesMap* handles_map, JournalIndex* delete_journals, + MetahandleSet* metahandles_to_purge, Directory::KernelLoadInfo* kernel_load_info) { DirOpenResult result = TryLoad(handles_map, delete_journals, - kernel_load_info); + metahandles_to_purge, kernel_load_info); if (result == OPENED) { UMA_HISTOGRAM_ENUMERATION( "Sync.DirectoryOpenResult", FIRST_TRY_SUCCESS, RESULT_COUNT); @@ -89,7 +89,8 @@ DirOpenResult OnDiskDirectoryBackingStore::Load( db_->set_histogram_tag("SyncDirectory"); base::DeleteFile(backing_filepath_, false); - result = TryLoad(handles_map, delete_journals, kernel_load_info); + result = TryLoad(handles_map, delete_journals, metahandles_to_purge, + kernel_load_info); if (result == OPENED) { UMA_HISTOGRAM_ENUMERATION( "Sync.DirectoryOpenResult", SECOND_TRY_SUCCESS, RESULT_COUNT); diff --git a/sync/syncable/on_disk_directory_backing_store.h b/sync/syncable/on_disk_directory_backing_store.h index fdeda4d..43bc74d 100644 --- a/sync/syncable/on_disk_directory_backing_store.h +++ b/sync/syncable/on_disk_directory_backing_store.h @@ -22,14 +22,15 @@ class SYNC_EXPORT_PRIVATE OnDiskDirectoryBackingStore ~OnDiskDirectoryBackingStore() override; DirOpenResult Load(Directory::MetahandlesMap* handles_map, JournalIndex* delete_journals, + MetahandleSet* metahandles_to_purge, Directory::KernelLoadInfo* kernel_load_info) override; // A helper function that will make one attempt to load the directory. // Unlike Load(), it does not attempt to recover from failure. - DirOpenResult TryLoad( - Directory::MetahandlesMap* handles_map, - JournalIndex* delete_journals, - Directory::KernelLoadInfo* kernel_load_info); + DirOpenResult TryLoad(Directory::MetahandlesMap* handles_map, + JournalIndex* delete_journals, + MetahandleSet* metahandles_to_purge, + Directory::KernelLoadInfo* kernel_load_info); protected: // Subclasses may override this to avoid a possible DCHECK. |