We dump all of the originals_ metahandles to the dirty_metahandles index and then check later to see if they are actually dirty. Arguably this is unnecessary, but I've been lead to believe (and my quick debugging has shown) that metahandles used in a WriteTransaction tend to get dirtied. The majority of times the TakeSnapshot... loop executes now, the dirty_metahandles set is empty.There's also the benefit that it's pretty clear that the ScopedKernelLock, which is needed to look up an entry by metahandle, is held when we're iterating through the [possibly] dirty_metahandles in the TakeSnapshot... function. This is arguably less true of the WriteTransaction deconstructor. But if there's a different way to get the new dirty flag from an original then that's probably the way to go. I found out the hard way that checking the dirty flag of the original means nothing as I was leaking away metahandles.I added a short protected function to Directory that acts as a wrapper for the kernel_->dirty_metahandles->insert function. It seemed like the thing to do to decouple it somewhat. Plus we can check there to see if it's actually dirty if that's what we decide to do.We piggy-back on the transaction lock to save us thread-safety concerns in dealing with the dirty_metahandles set.

One more thing: if I'm screwing up any meta-stuff here please don't be afraid to share.  I'd hate to be writing too much, too little, or about the wrong stuff in these things :)
Added two unit tests to ensure that the WriteTransaction deconstructor and TakeSnapshotForSaveChanges function were cooperating.  One makes sure that all of the dirty metahandles show up again and the other makes sure that the non-dirty dirty_metahandles are disgarded.  I had to move some things out of the anonymous namespace in order for the FRIEND_TEST macro to work correctly.

Old patch: http://codereview.chromium.org/585007

BUG=19901
TEST=SyncableDirectoryTest.TakeSnapshotGetsAllDirtyHandlesTest, SyncableDirectoryTest.TakeSnapshotGetsOnlyDirtyHandlesTest
AUTHOR=Alex Gartrell (AlexGartrell@gmail.com)

chron submitting on original author's behalf.

Review URL: http://codereview.chromium.org/598006

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

4 files changed, 188 insertions, 10 deletions

diff --git a/AUTHORS b/AUTHORS
index ad09cd8..704fc82 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -63,3 +63,4 @@ Vernon Tang <vt@foilhead.net>
 Alexander Sulfrian <alexander@sulfrian.net>
 Philippe Beaudoin <philippe.beaudoin@gmail.com>
 Mark Hahnenberg <mhahnenb@gmail.com>
+Alex Gartrell <alexgartrell@gmail.com>
diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc
index ff0ab77..b3df77e 100755
--- a/chrome/browser/sync/syncable/syncable.cc
+++ b/chrome/browser/sync/syncable/syncable.cc
@@ -166,6 +166,7 @@ Directory::Kernel::Kernel(const FilePath& db_path,
       extended_attributes(new ExtendedAttributes),
       unapplied_update_metahandles(new MetahandleSet),
       unsynced_metahandles(new MetahandleSet),
+      dirty_metahandles(new MetahandleSet),
       channel(new Directory::Channel(syncable::DIRECTORY_DESTROYED)),
       changes_channel(new Directory::ChangesChannel(kShutdownChangesEvent)),
       last_sync_timestamp_(info.kernel_info.last_sync_timestamp),
@@ -196,6 +197,7 @@ Directory::Kernel::~Kernel() {
   delete changes_channel;
   delete unsynced_metahandles;
   delete unapplied_update_metahandles;
+  delete dirty_metahandles;
   delete extended_attributes;
   delete parent_id_child_index;
   delete client_tag_index;
@@ -232,6 +234,7 @@ void Directory::InitializeIndices() {
       kernel_->unsynced_metahandles->insert(entry->ref(META_HANDLE));
     if (entry->ref(IS_UNAPPLIED_UPDATE))
       kernel_->unapplied_update_metahandles->insert(entry->ref(META_HANDLE));
+    DCHECK(!entry->is_dirty());
   }
 }
 
@@ -484,6 +487,16 @@ void Directory::ReindexParentId(EntryKernel* const entry,
   CHECK(kernel_->parent_id_child_index->insert(entry).second);
 }
 
+void Directory::AddToDirtyMetahandles(int64 handle) {
+  kernel_->transaction_mutex.AssertAcquired();
+  kernel_->dirty_metahandles->insert(handle);
+}
+
+void Directory::ClearDirtyMetahandles() {
+  kernel_->transaction_mutex.AssertAcquired();
+  kernel_->dirty_metahandles->clear();
+}
+
 // static
 bool Directory::SafeToPurgeFromMemory(const EntryKernel* const entry) {
   return entry->ref(IS_DEL) && !entry->is_dirty() && !entry->ref(SYNCING) &&
@@ -495,14 +508,17 @@ void Directory::TakeSnapshotForSaveChanges(SaveChangesSnapshot* snapshot) {
   ScopedKernelLock lock(this);
   // Deep copy dirty entries from kernel_->metahandles_index into snapshot and
   // clear dirty flags.
-  for (MetahandlesIndex::iterator i = kernel_->metahandles_index->begin();
-       i != kernel_->metahandles_index->end(); ++i) {
-    EntryKernel* entry = *i;
+
+  for (MetahandleSet::const_iterator i = kernel_->dirty_metahandles->begin();
+       i != kernel_->dirty_metahandles->end(); ++i) {
+    EntryKernel* entry = GetEntryByHandle(*i, &lock);
+    // Skip over false positives; it happens relatively infrequently.
     if (!entry->is_dirty())
       continue;
     snapshot->dirty_metas.insert(snapshot->dirty_metas.end(), *entry);
     entry->clear_dirty();
   }
+  ClearDirtyMetahandles();
 
   // Do the same for extended attributes.
   for (ExtendedAttributes::iterator i = kernel_->extended_attributes->begin();
@@ -986,6 +1002,11 @@ WriteTransaction::~WriteTransaction() {
     else
       directory()->CheckTreeInvariants(this, originals_);
   }
+
+  for (OriginalEntries::const_iterator i = originals_->begin();
+      i != originals_->end(); ++i) {
+    directory()->AddToDirtyMetahandles(i->ref(META_HANDLE));
+  }
   UnlockAndLog(originals_);
 }
 
diff --git a/chrome/browser/sync/syncable/syncable.h b/chrome/browser/sync/syncable/syncable.h
index fd328b3..1bb529d 100755
--- a/chrome/browser/sync/syncable/syncable.h
+++ b/chrome/browser/sync/syncable/syncable.h
@@ -29,6 +29,7 @@
 #include "chrome/browser/sync/util/event_sys.h"
 #include "chrome/browser/sync/util/row_iterator.h"
 #include "chrome/browser/sync/util/sync_types.h"
+#include "testing/gtest/include/gtest/gtest_prod.h"  // For FRIEND_TEST
 
 struct PurgeInfo;
 
@@ -648,6 +649,10 @@ class Directory {
   friend class ScopedKernelLock;
   friend class ScopedKernelUnlock;
   friend class WriteTransaction;
+  friend class SyncableDirectoryTest;
+  FRIEND_TEST(SyncableDirectoryTest, TakeSnapshotGetsAllDirtyHandlesTest);
+  FRIEND_TEST(SyncableDirectoryTest, TakeSnapshotGetsOnlyDirtyHandlesTest);
+
  public:
   // Various data that the Directory::Kernel we are backing (persisting data
   // for) needs saved across runs of the application.
@@ -736,6 +741,8 @@ class Directory {
   EntryKernel* GetRootEntry();
   bool ReindexId(EntryKernel* const entry, const Id& new_id);
   void ReindexParentId(EntryKernel* const entry, const Id& new_parent_id);
+  void AddToDirtyMetahandles(int64 handle);
+  void ClearDirtyMetahandles();
 
   // These don't do semantic checking.
   // The semantic checking is implemented higher up.
@@ -930,12 +937,12 @@ class Directory {
     EntryKernel needle;
     ExtendedAttributes* const extended_attributes;
 
-    // 2 in-memory indices on bits used extremely frequently by the syncer.
+    // 3 in-memory indices on bits used extremely frequently by the syncer.
     MetahandleSet* const unapplied_update_metahandles;
     MetahandleSet* const unsynced_metahandles;
-    // TODO(timsteele): Add a dirty_metahandles index as we now may want to
-    // optimize the SaveChanges work of scanning all entries to find dirty ones
-    // due to the entire entry domain now being in-memory.
+    // Contains metahandles that are most likely dirty (though not
+    // necessarily).  Dirtyness is confirmed in TakeSnapshotForSaveChanges().
+    MetahandleSet* const dirty_metahandles;
 
     // TODO(ncarter): Figure out what the hell this is, and comment it.
     Channel* const channel;
diff --git a/chrome/browser/sync/syncable/syncable_unittest.cc b/chrome/browser/sync/syncable/syncable_unittest.cc
index d94815a..3d514e0e 100755
--- a/chrome/browser/sync/syncable/syncable_unittest.cc
+++ b/chrome/browser/sync/syncable/syncable_unittest.cc
@@ -243,9 +243,6 @@ TEST_F(SyncableGeneralTest, ClientIndexRebuildsDeletedProperly) {
   }
 }
 
-
-namespace {
-
 // A Directory whose backing store always fails SaveChanges by returning false.
 class TestUnsaveableDirectory : public Directory {
  public:
@@ -316,11 +313,159 @@ class SyncableDirectoryTest : public testing::Test {
       bool set_server_fields, bool is_dir, bool add_to_lru, int64 *meta_handle);
 };
 
+TEST_F(SyncableDirectoryTest, TakeSnapshotGetsAllDirtyHandlesTest) {
+  const int metahandles_to_create = 100;
+  std::vector<int64> expected_dirty_metahandles;
+  {
+    WriteTransaction trans(dir_.get(), UNITTEST, __FILE__, __LINE__);
+    for (int i = 0; i < metahandles_to_create; i++) {
+      MutableEntry e(&trans, CREATE, trans.root_id(), "foo");
+      expected_dirty_metahandles.push_back(e.Get(META_HANDLE));
+      e.Put(IS_UNSYNCED, true);
+    }
+  }
+  // Fake SaveChanges() and make sure we got what we expected.
+  {
+    Directory::SaveChangesSnapshot snapshot;
+    AutoLock scoped_lock(dir_->kernel_->save_changes_mutex);
+    dir_->TakeSnapshotForSaveChanges(&snapshot);
+    // Make sure there's an entry for each new metahandle.  Make sure all
+    // entries are marked dirty.
+    ASSERT_EQ(expected_dirty_metahandles.size(), snapshot.dirty_metas.size());
+    for (OriginalEntries::const_iterator i = snapshot.dirty_metas.begin();
+        i != snapshot.dirty_metas.end(); ++i) {
+      ASSERT_TRUE(i->is_dirty());
+    }
+    dir_->VacuumAfterSaveChanges(snapshot);
+  }
+  // Put a new value with existing transactions as well as adding new ones.
+  {
+    WriteTransaction trans(dir_.get(), UNITTEST, __FILE__, __LINE__);
+    std::vector<int64> new_dirty_metahandles;
+    for (std::vector<int64>::const_iterator i =
+        expected_dirty_metahandles.begin();
+        i != expected_dirty_metahandles.end(); ++i) {
+        // Change existing entries to directories to dirty them.
+        MutableEntry e1(&trans, GET_BY_HANDLE, *i);
+        e1.Put(IS_DIR, true);
+        e1.Put(IS_UNSYNCED, true);
+        // Add new entries
+        MutableEntry e2(&trans, CREATE, trans.root_id(), "bar");
+        e2.Put(IS_UNSYNCED, true);
+        new_dirty_metahandles.push_back(e2.Get(META_HANDLE));
+    }
+    expected_dirty_metahandles.insert(expected_dirty_metahandles.end(),
+        new_dirty_metahandles.begin(), new_dirty_metahandles.end());
+  }
+  // Fake SaveChanges() and make sure we got what we expected.
+  {
+    Directory::SaveChangesSnapshot snapshot;
+    AutoLock scoped_lock(dir_->kernel_->save_changes_mutex);
+    dir_->TakeSnapshotForSaveChanges(&snapshot);
+    // Make sure there's an entry for each new metahandle.  Make sure all
+    // entries are marked dirty.
+    EXPECT_EQ(expected_dirty_metahandles.size(), snapshot.dirty_metas.size());
+    for (OriginalEntries::const_iterator i = snapshot.dirty_metas.begin();
+        i != snapshot.dirty_metas.end(); ++i) {
+      EXPECT_TRUE(i->is_dirty());
+    }
+    dir_->VacuumAfterSaveChanges(snapshot);
+  }
+}
+
+TEST_F(SyncableDirectoryTest, TakeSnapshotGetsOnlyDirtyHandlesTest) {
+  const int metahandles_to_create = 100;
+  const unsigned int number_changed = 100u;  // half of 2 * metahandles_to_create
+  std::vector<int64> expected_dirty_metahandles;
+  {
+    WriteTransaction trans(dir_.get(), UNITTEST, __FILE__, __LINE__);
+    for (int i = 0; i < metahandles_to_create; i++) {
+      MutableEntry e(&trans, CREATE, trans.root_id(), "foo");
+      expected_dirty_metahandles.push_back(e.Get(META_HANDLE));
+      e.Put(IS_UNSYNCED, true);
+    }
+  }
+  dir_->SaveChanges();
+  // Put a new value with existing transactions as well as adding new ones.
+  {
+    WriteTransaction trans(dir_.get(), UNITTEST, __FILE__, __LINE__);
+    std::vector<int64> new_dirty_metahandles;
+    for (std::vector<int64>::const_iterator i =
+        expected_dirty_metahandles.begin();
+        i != expected_dirty_metahandles.end(); ++i) {
+        // Change existing entries to directories to dirty them.
+        MutableEntry e1(&trans, GET_BY_HANDLE, *i);
+        ASSERT_TRUE(e1.good());
+        e1.Put(IS_DIR, true);
+        e1.Put(IS_UNSYNCED, true);
+        // Add new entries
+        MutableEntry e2(&trans, CREATE, trans.root_id(), "bar");
+        e2.Put(IS_UNSYNCED, true);
+        new_dirty_metahandles.push_back(e2.Get(META_HANDLE));
+    }
+    expected_dirty_metahandles.insert(expected_dirty_metahandles.end(),
+        new_dirty_metahandles.begin(), new_dirty_metahandles.end());
+  }
+  dir_->SaveChanges();
+  // Don't make any changes whatsoever and ensure nothing comes back.
+  {
+    WriteTransaction trans(dir_.get(), UNITTEST, __FILE__, __LINE__);
+    for (std::vector<int64>::const_iterator i =
+        expected_dirty_metahandles.begin();
+        i != expected_dirty_metahandles.end(); ++i) {
+      MutableEntry e(&trans, GET_BY_HANDLE, *i);
+      ASSERT_TRUE(e.good());
+      // We aren't doing anything to dirty these entries.
+    }
+  }
+  // Fake SaveChanges() and make sure we got what we expected.
+  {
+    Directory::SaveChangesSnapshot snapshot;
+    AutoLock scoped_lock(dir_->kernel_->save_changes_mutex);
+    dir_->TakeSnapshotForSaveChanges(&snapshot);
+    // Make sure there are no dirty_metahandles.
+    EXPECT_EQ(0u, snapshot.dirty_metas.size());
+    dir_->VacuumAfterSaveChanges(snapshot);
+  }
+  {
+    WriteTransaction trans(dir_.get(), UNITTEST, __FILE__, __LINE__);
+    bool should_change = false;
+    for (std::vector<int64>::const_iterator i =
+        expected_dirty_metahandles.begin();
+        i != expected_dirty_metahandles.end(); ++i) {
+        // Maybe change entries by flipping IS_DIR.
+        MutableEntry e(&trans, GET_BY_HANDLE, *i);
+        ASSERT_TRUE(e.good());
+        should_change = !should_change;
+        if (should_change) {
+          bool not_dir = !e.Get(IS_DIR);
+          e.Put(IS_DIR, not_dir);
+          e.Put(IS_UNSYNCED, true);
+        }
+    }
+  }
+  // Fake SaveChanges() and make sure we got what we expected.
+  {
+    Directory::SaveChangesSnapshot snapshot;
+    AutoLock scoped_lock(dir_->kernel_->save_changes_mutex);
+    dir_->TakeSnapshotForSaveChanges(&snapshot);
+    // Make sure there's an entry for each changed metahandle.  Make sure all
+    // entries are marked dirty.
+    EXPECT_EQ(number_changed, snapshot.dirty_metas.size());
+    for (OriginalEntries::const_iterator i = snapshot.dirty_metas.begin();
+        i != snapshot.dirty_metas.end(); ++i) {
+      EXPECT_TRUE(i->is_dirty());
+    }
+    dir_->VacuumAfterSaveChanges(snapshot);
+  }
+}
+
 const FilePath::CharType SyncableDirectoryTest::kFilePath[] =
     FILE_PATH_LITERAL("Test.sqlite3");
 const char SyncableDirectoryTest::kName[] = "Foo";
 const Id SyncableDirectoryTest::kId(TestIdFactory::FromNumber(-99));
 
+namespace {
 TEST_F(SyncableDirectoryTest, TestBasicLookupNonExistantID) {
   ReadTransaction rtrans(dir_.get(), __FILE__, __LINE__);
   Entry e(&rtrans, GET_BY_ID, kId);
@@ -942,6 +1087,8 @@ TEST_F(SyncableDirectoryTest, GetModelType) {
   }
 }
 
+}  // namespace
+
 void SyncableDirectoryTest::ValidateEntry(BaseTransaction* trans, int64 id,
     bool check_name, string name, int64 base_version, int64 server_version,
     bool is_del) {
@@ -955,6 +1102,8 @@ void SyncableDirectoryTest::ValidateEntry(BaseTransaction* trans, int64 id,
   ASSERT_TRUE(is_del == e.Get(IS_DEL));
 }
 
+namespace {
+
 TEST(SyncableDirectoryManager, TestFileRelease) {
   DirectoryManager dm(FilePath(FILE_PATH_LITERAL(".")));
   ASSERT_TRUE(dm.Open("ScopeTest"));