[Sync] Add framework for avoid posting tasks on threads with no work to do

Add abstract GetGroupsToChange() method to ModelChangingSyncerCommand.
Use that to figure out which worker threads to post work on (instead of
posting on all of them).

Implement GetGroupsToChange() for each ModelChangingSyncerCommand.

Add GetEnabledGroups() and GetEnabledGroupsWithConflicts() functions to SyncSession.

Key unapplied updates index by type for ApplyUpdatesCommand.

Make the abstract methods of ModelChangingSyncerCommand protected.

Add HasCustomGroupsToChange() method to ModelChangingSyncerCommand and have all
commands default to returning false to track down the perf regression introduced
by the last couple of times this was landed (112178, 112815).

BUG=97832
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112178

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112743

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112815

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

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

3 files changed, 106 insertions, 45 deletions

diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc
index 0050fbf..6a931f3 100644
--- a/chrome/browser/sync/syncable/syncable.cc
+++ b/chrome/browser/sync/syncable/syncable.cc
@@ -258,6 +258,20 @@ EntryKernel::EntryKernel() : dirty_(false) {
 
 EntryKernel::~EntryKernel() {}
 
+syncable::ModelType EntryKernel::GetServerModelType() const {
+  ModelType specifics_type = GetModelTypeFromSpecifics(ref(SERVER_SPECIFICS));
+  if (specifics_type != UNSPECIFIED)
+    return specifics_type;
+  if (ref(ID).IsRoot())
+    return TOP_LEVEL_FOLDER;
+  // Loose check for server-created top-level folders that aren't
+  // bound to a particular model type.
+  if (!ref(UNIQUE_SERVER_TAG).empty() && ref(SERVER_IS_DIR))
+    return TOP_LEVEL_FOLDER;
+
+  return UNSPECIFIED;
+}
+
 bool EntryKernel::ContainsString(const std::string& lowercase_query) const {
   // TODO(lipalani) - figure out what to do if the node is encrypted.
   const sync_pb::EntitySpecifics& specifics = ref(SPECIFICS);
@@ -328,6 +342,7 @@ StringValue* IdToValue(const Id& id) {
 DictionaryValue* EntryKernel::ToValue() const {
   DictionaryValue* kernel_info = new DictionaryValue();
   kernel_info->SetBoolean("isDirty", is_dirty());
+  kernel_info->Set("serverModelType", ModelTypeToValue(GetServerModelType()));
 
   // Int64 fields.
   SetFieldValues(*this, kernel_info,
@@ -433,7 +448,6 @@ Directory::Kernel::Kernel(
       ids_index(new Directory::IdsIndex),
       parent_id_child_index(new Directory::ParentIdChildIndex),
       client_tag_index(new Directory::ClientTagIndex),
-      unapplied_update_metahandles(new MetahandleSet),
       unsynced_metahandles(new MetahandleSet),
       dirty_metahandles(new MetahandleSet),
       metahandles_to_purge(new MetahandleSet),
@@ -459,7 +473,6 @@ void Directory::Kernel::Release() {
 Directory::Kernel::~Kernel() {
   CHECK_EQ(0, refcount);
   delete unsynced_metahandles;
-  delete unapplied_update_metahandles;
   delete dirty_metahandles;
   delete metahandles_to_purge;
   delete parent_id_child_index;
@@ -496,10 +509,13 @@ void Directory::InitializeIndices() {
         kernel_->parent_id_child_index);
     InitializeIndexEntry<IdIndexer>(entry, kernel_->ids_index);
     InitializeIndexEntry<ClientTagIndexer>(entry, kernel_->client_tag_index);
+    const int64 metahandle = entry->ref(META_HANDLE);
     if (entry->ref(IS_UNSYNCED))
-      kernel_->unsynced_metahandles->insert(entry->ref(META_HANDLE));
-    if (entry->ref(IS_UNAPPLIED_UPDATE))
-      kernel_->unapplied_update_metahandles->insert(entry->ref(META_HANDLE));
+      kernel_->unsynced_metahandles->insert(metahandle);
+    if (entry->ref(IS_UNAPPLIED_UPDATE)) {
+      const ModelType type = entry->GetServerModelType();
+      kernel_->unapplied_update_metahandles[type].insert(metahandle);
+    }
     DCHECK(!entry->is_dirty());
   }
 }
@@ -702,11 +718,12 @@ bool Directory::SafeToPurgeFromMemory(const EntryKernel* const entry) const {
       !entry->ref(IS_UNSYNCED);
 
   if (safe) {
-    int64 handle = entry->ref(META_HANDLE);
+    const int64 handle = entry->ref(META_HANDLE);
+    const ModelType type = entry->GetServerModelType();
     CHECK_EQ(kernel_->dirty_metahandles->count(handle), 0U);
     // TODO(tim): Bug 49278.
     CHECK(!kernel_->unsynced_metahandles->count(handle));
-    CHECK(!kernel_->unapplied_update_metahandles->count(handle));
+    CHECK(!kernel_->unapplied_update_metahandles[type].count(handle));
   }
 
   return safe;
@@ -837,7 +854,8 @@ void Directory::PurgeEntriesWithTypeIn(const std::set<ModelType>& types) {
           DCHECK_EQ(entry->ref(UNIQUE_CLIENT_TAG).empty(), !num_erased);
           num_erased = kernel_->unsynced_metahandles->erase(handle);
           DCHECK_EQ(entry->ref(IS_UNSYNCED), num_erased > 0);
-          num_erased = kernel_->unapplied_update_metahandles->erase(handle);
+          num_erased =
+              kernel_->unapplied_update_metahandles[server_type].erase(handle);
           DCHECK_EQ(entry->ref(IS_UNAPPLIED_UPDATE), num_erased > 0);
           num_erased = kernel_->parent_id_child_index->erase(entry);
           DCHECK_EQ(entry->ref(IS_DEL), !num_erased);
@@ -1007,13 +1025,33 @@ int64 Directory::unsynced_entity_count() const {
   return kernel_->unsynced_metahandles->size();
 }
 
-void Directory::GetUnappliedUpdateMetaHandles(BaseTransaction* trans,
+syncable::ModelTypeBitSet
+    Directory::GetServerTypesWithUnappliedUpdates(
+        BaseTransaction* trans) const {
+  syncable::ModelTypeBitSet server_types;
+  ScopedKernelLock lock(this);
+  for (int i = 0; i < MODEL_TYPE_COUNT; ++i) {
+    if (!kernel_->unapplied_update_metahandles[i].empty()) {
+      server_types.set(i);
+    }
+  }
+  return server_types;
+}
+
+void Directory::GetUnappliedUpdateMetaHandles(
+    BaseTransaction* trans,
+    syncable::ModelTypeBitSet server_types,
     UnappliedUpdateMetaHandles* result) {
   result->clear();
   ScopedKernelLock lock(this);
-  copy(kernel_->unapplied_update_metahandles->begin(),
-       kernel_->unapplied_update_metahandles->end(),
-       back_inserter(*result));
+  for (int i = 0; i < MODEL_TYPE_COUNT; ++i) {
+    const ModelType type = ModelTypeFromInt(i);
+    if (server_types.test(type)) {
+      std::copy(kernel_->unapplied_update_metahandles[type].begin(),
+                kernel_->unapplied_update_metahandles[type].end(),
+                back_inserter(*result));
+    }
+  }
 }
 
 
@@ -1370,8 +1408,6 @@ DictionaryValue* Entry::ToValue() const {
   entry_info->SetBoolean("good", good());
   if (good()) {
     entry_info->Set("kernel", kernel_->ToValue());
-    entry_info->Set("serverModelType",
-                    ModelTypeToValue(GetServerModelTypeHelper()));
     entry_info->Set("modelType",
                     ModelTypeToValue(GetModelType()));
     entry_info->SetBoolean("existsOnClientBecauseNameIsNonEmpty",
@@ -1387,7 +1423,7 @@ const string& Entry::Get(StringField field) const {
 }
 
 syncable::ModelType Entry::GetServerModelType() const {
-  ModelType specifics_type = GetServerModelTypeHelper();
+  ModelType specifics_type = kernel_->GetServerModelType();
   if (specifics_type != UNSPECIFIED)
     return specifics_type;
 
@@ -1403,20 +1439,6 @@ syncable::ModelType Entry::GetServerModelType() const {
   return UNSPECIFIED;
 }
 
-syncable::ModelType Entry::GetServerModelTypeHelper() const {
-  ModelType specifics_type = GetModelTypeFromSpecifics(Get(SERVER_SPECIFICS));
-  if (specifics_type != UNSPECIFIED)
-    return specifics_type;
-  if (IsRoot())
-    return TOP_LEVEL_FOLDER;
-  // Loose check for server-created top-level folders that aren't
-  // bound to a particular model type.
-  if (!Get(UNIQUE_SERVER_TAG).empty() && Get(SERVER_IS_DIR))
-    return TOP_LEVEL_FOLDER;
-
-  return UNSPECIFIED;
-}
-
 syncable::ModelType Entry::GetModelType() const {
   ModelType specifics_type = GetModelTypeFromSpecifics(Get(SPECIFICS));
   if (specifics_type != UNSPECIFIED)
@@ -1603,8 +1625,32 @@ bool MutableEntry::Put(ProtoField field,
   // TODO(ncarter): This is unfortunately heavyweight.  Can we do
   // better?
   if (kernel_->ref(field).SerializeAsString() != value.SerializeAsString()) {
+    const bool update_unapplied_updates_index =
+        (field == SERVER_SPECIFICS) && kernel_->ref(IS_UNAPPLIED_UPDATE);
+    if (update_unapplied_updates_index) {
+      // Remove ourselves from unapplied_update_metahandles with our
+      // old server type.
+      const syncable::ModelType old_server_type =
+          kernel_->GetServerModelType();
+      const int64 metahandle = kernel_->ref(META_HANDLE);
+      size_t erase_count =
+          dir()->kernel_->unapplied_update_metahandles[old_server_type]
+          .erase(metahandle);
+      DCHECK_EQ(erase_count, 1u);
+    }
+
     kernel_->put(field, value);
     kernel_->mark_dirty(dir()->kernel_->dirty_metahandles);
+
+    if (update_unapplied_updates_index) {
+      // Add ourselves back into unapplied_update_metahandles with our
+      // new server type.
+      const syncable::ModelType new_server_type =
+          kernel_->GetServerModelType();
+      const int64 metahandle = kernel_->ref(META_HANDLE);
+      dir()->kernel_->unapplied_update_metahandles[new_server_type]
+          .insert(metahandle);
+    }
   }
   return true;
 }
@@ -1667,10 +1713,16 @@ bool MutableEntry::Put(IndexedBitField field, bool value) {
   DCHECK(kernel_);
   if (kernel_->ref(field) != value) {
     MetahandleSet* index;
-    if (IS_UNSYNCED == field)
+    if (IS_UNSYNCED == field) {
       index = dir()->kernel_->unsynced_metahandles;
-    else
-      index = dir()->kernel_->unapplied_update_metahandles;
+    } else {
+      // Use kernel_->GetServerModelType() instead of
+      // GetServerModelType() as we may trigger some DCHECKs in the
+      // latter.
+      index =
+          &dir()->kernel_->unapplied_update_metahandles[
+              kernel_->GetServerModelType()];
+    }
 
     ScopedKernelLock lock(dir());
     if (value)
diff --git a/chrome/browser/sync/syncable/syncable.h b/chrome/browser/sync/syncable/syncable.h
index 64c518b..73e71d5 100644
--- a/chrome/browser/sync/syncable/syncable.h
+++ b/chrome/browser/sync/syncable/syncable.h
@@ -371,6 +371,8 @@ struct EntryKernel {
     return id_fields[field - ID_FIELDS_BEGIN];
   }
 
+  syncable::ModelType GetServerModelType() const;
+
   // Does a case in-sensitive search for a given string, which must be
   // lower case.
   bool ContainsString(const std::string& lowercase_query) const;
@@ -487,9 +489,6 @@ class Entry {
   EntryKernel* kernel_;
 
  private:
-  // Like GetServerModelType() but without the DCHECKs.
-  ModelType GetServerModelTypeHelper() const;
-
   DISALLOW_COPY_AND_ASSIGN(Entry);
 };
 
@@ -969,9 +968,17 @@ class Directory {
   void GetUnsyncedMetaHandles(BaseTransaction* trans,
                               UnsyncedMetaHandles* result);
 
-  // Get all the metahandles for unapplied updates
+  // Returns all server types with unapplied updates.  A subset of
+  // those types can then be passed into
+  // GetUnappliedUpdateMetaHandles() below.
+  syncable::ModelTypeBitSet GetServerTypesWithUnappliedUpdates(
+      BaseTransaction* trans) const;
+
+  // Get all the metahandles for unapplied updates for a given set of
+  // server types.
   typedef std::vector<int64> UnappliedUpdateMetaHandles;
   void GetUnappliedUpdateMetaHandles(BaseTransaction* trans,
+                                     syncable::ModelTypeBitSet server_types,
                                      UnappliedUpdateMetaHandles* result);
 
   // Checks tree metadata consistency.
@@ -1103,7 +1110,8 @@ class Directory {
     EntryKernel needle;
 
     // 3 in-memory indices on bits used extremely frequently by the syncer.
-    MetahandleSet* const unapplied_update_metahandles;
+    // |unapplied_update_metahandles| is keyed by the server model type.
+    MetahandleSet unapplied_update_metahandles[MODEL_TYPE_COUNT];
     MetahandleSet* const unsynced_metahandles;
     // Contains metahandles that are most likely dirty (though not
     // necessarily).  Dirtyness is confirmed in TakeSnapshotForSaveChanges().
diff --git a/chrome/browser/sync/syncable/syncable_unittest.cc b/chrome/browser/sync/syncable/syncable_unittest.cc
index 1b6fc38..b7fcb6e 100644
--- a/chrome/browser/sync/syncable/syncable_unittest.cc
+++ b/chrome/browser/sync/syncable/syncable_unittest.cc
@@ -47,8 +47,8 @@ TEST_F(SyncableKernelTest, ToValue) {
   if (value.get()) {
     // Not much to check without repeating the ToValue() code.
     EXPECT_TRUE(value->HasKey("isDirty"));
-    // The extra +1 is for "isDirty".
-    EXPECT_EQ(BIT_TEMPS_END - BEGIN_FIELDS + 1,
+    // The extra +2 is for "isDirty" and "serverModelType".
+    EXPECT_EQ(BIT_TEMPS_END - BEGIN_FIELDS + 2,
               static_cast<int>(value->size()));
   } else {
     ADD_FAILURE();
@@ -370,7 +370,6 @@ TEST_F(SyncableGeneralTest, ToValue) {
     scoped_ptr<DictionaryValue> value(me.ToValue());
     ExpectDictBooleanValue(true, *value, "good");
     EXPECT_TRUE(value->HasKey("kernel"));
-    ExpectDictStringValue("Unspecified", *value, "serverModelType");
     ExpectDictStringValue("Unspecified", *value, "modelType");
     ExpectDictBooleanValue(true, *value, "existsOnClientBecauseNameIsNonEmpty");
     ExpectDictBooleanValue(false, *value, "isRoot");
@@ -880,10 +879,12 @@ TEST_F(SyncableDirectoryTest, TestGetUnsynced) {
 TEST_F(SyncableDirectoryTest, TestGetUnappliedUpdates) {
   Directory::UnappliedUpdateMetaHandles handles;
   int64 handle1, handle2;
+  syncable::ModelTypeBitSet all_types;
+  all_types.set();
   {
     WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get());
 
-    dir_->GetUnappliedUpdateMetaHandles(&trans, &handles);
+    dir_->GetUnappliedUpdateMetaHandles(&trans, all_types, &handles);
     ASSERT_TRUE(0 == handles.size());
 
     MutableEntry e1(&trans, CREATE, trans.root_id(), "abba");
@@ -905,7 +906,7 @@ TEST_F(SyncableDirectoryTest, TestGetUnappliedUpdates) {
   {
     WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get());
 
-    dir_->GetUnappliedUpdateMetaHandles(&trans, &handles);
+    dir_->GetUnappliedUpdateMetaHandles(&trans, all_types, &handles);
     ASSERT_TRUE(0 == handles.size());
 
     MutableEntry e3(&trans, GET_BY_HANDLE, handle1);
@@ -915,7 +916,7 @@ TEST_F(SyncableDirectoryTest, TestGetUnappliedUpdates) {
   dir_->SaveChanges();
   {
     WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get());
-    dir_->GetUnappliedUpdateMetaHandles(&trans, &handles);
+    dir_->GetUnappliedUpdateMetaHandles(&trans, all_types, &handles);
     ASSERT_TRUE(1 == handles.size());
     ASSERT_TRUE(handle1 == handles[0]);
 
@@ -926,7 +927,7 @@ TEST_F(SyncableDirectoryTest, TestGetUnappliedUpdates) {
   dir_->SaveChanges();
   {
     WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get());
-    dir_->GetUnappliedUpdateMetaHandles(&trans, &handles);
+    dir_->GetUnappliedUpdateMetaHandles(&trans, all_types, &handles);
     ASSERT_TRUE(2 == handles.size());
     if (handle1 == handles[0]) {
       ASSERT_TRUE(handle2 == handles[1]);
@@ -942,7 +943,7 @@ TEST_F(SyncableDirectoryTest, TestGetUnappliedUpdates) {
   dir_->SaveChanges();
   {
     WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get());
-    dir_->GetUnappliedUpdateMetaHandles(&trans, &handles);
+    dir_->GetUnappliedUpdateMetaHandles(&trans, all_types, &handles);
     ASSERT_TRUE(1 == handles.size());
     ASSERT_TRUE(handle2 == handles[0]);
   }