sync: Better iteration in GenericChangeProcessor

This change introduces a new function for fetching the handles of all
children of a sync node, then puts it to use in optimizing the
GenericChangeProcessor's GetSyncDataForType() function.

Prior to the UniquePosition changes, it was simple and cheap to fetch
the ID of a successor or predecessor item.  After the change, it
requires a few expensive map lookups.  In other words, GetSuccessorId()
has gone from being O(1) to O(lg(n)).  This is a especially a problem in
code paths where we use GetSuccessorId() to iterate over all nodes in a
folder.

The UniquePosition change also makes it pretty easy to fetch all child
nodes under a given parent.  We could easily return all the EntryKernels
under a given folder.  Unfortunately, the APIs don't make it easy to
expose that functionality.  Instead, we do something less efficient, but
still much better than the status quo: return the IDs of all the
children.  The caller will need to look up each entry at O(lg(n)) cost,
but at least it didn't have to do two lookups to fetch each ID.

This change should lead to a slight performance improvement in the
ModelAssociation time of types that use the GenericChangeProcessor.

BUG=178275, 241813

Review URL: https://chromiumcodereview.appspot.com/14667013

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

chrome/browser/sync/glue/generic_change_processor.cc

@@ -116,10 +116,13 @@ syncer::SyncError GenericChangeProcessor::GetSyncDataForType(
   // TODO(akalin): We'll have to do a tree traversal for bookmarks.
   DCHECK_NE(type, syncer::BOOKMARKS);
 
-  int64 sync_child_id = root.GetFirstChildId();
-  while (sync_child_id != syncer::kInvalidId) {
+  std::vector<int64> child_ids;
+  root.GetChildIds(&child_ids);
+
+  for (std::vector<int64>::iterator it = child_ids.begin();
+       it != child_ids.end(); ++it) {
     syncer::ReadNode sync_child_node(&trans);
-    if (sync_child_node.InitByIdLookup(sync_child_id) !=
+    if (sync_child_node.InitByIdLookup(*it) !=
             syncer::BaseNode::INIT_OK) {
       syncer::SyncError error(FROM_HERE,
                       "Failed to fetch child node for type " + type_name + ".",
@@ -128,7 +131,6 @@ syncer::SyncError GenericChangeProcessor::GetSyncDataForType(
     }
     current_sync_data->push_back(syncer::SyncData::CreateRemoteData(
         sync_child_node.GetId(), sync_child_node.GetEntitySpecifics()));
-    sync_child_id = sync_child_node.GetSuccessorId();
   }
   return syncer::SyncError();
 }

chrome/browser/sync/glue/generic_change_processor_unittest.cc

@@ -0,0 +1,109 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/sync/glue/generic_change_processor.h"
+
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop.h"
+#include "base/stringprintf.h"
+#include "chrome/browser/sync/glue/data_type_error_handler_mock.h"
+#include "sync/api/fake_syncable_service.h"
+#include "sync/api/sync_merge_result.h"
+#include "sync/internal_api/public/base/model_type.h"
+#include "sync/internal_api/public/read_node.h"
+#include "sync/internal_api/public/test/test_user_share.h"
+#include "sync/internal_api/public/user_share.h"
+#include "sync/internal_api/public/write_node.h"
+#include "sync/internal_api/public/write_transaction.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace browser_sync {
+
+namespace {
+
+class GenericChangeProcessorTest : public testing::Test {
+ public:
+  // It doesn't matter which type we use.  Just pick one.
+  static const syncer::ModelType kType = syncer::PREFERENCES;
+
+  GenericChangeProcessorTest() :
+      loop(base::MessageLoop::TYPE_UI),
+      sync_merge_result_(kType),
+      merge_result_ptr_factory_(&sync_merge_result_),
+      syncable_service_ptr_factory_(&fake_syncable_service_) {
+  }
+
+  virtual void SetUp() OVERRIDE {
+    test_user_share_.SetUp();
+    syncer::TestUserShare::CreateRoot(kType, test_user_share_.user_share());
+    change_processor_.reset(
+        new GenericChangeProcessor(
+            &data_type_error_handler_,
+            syncable_service_ptr_factory_.GetWeakPtr(),
+            merge_result_ptr_factory_.GetWeakPtr(),
+            test_user_share_.user_share()));
+  }
+
+  virtual void TearDown() OVERRIDE {
+    test_user_share_.TearDown();
+  }
+
+  void BuildChildNodes(int n) {
+    syncer::WriteTransaction trans(FROM_HERE, user_share());
+    syncer::ReadNode root(&trans);
+    ASSERT_EQ(syncer::BaseNode::INIT_OK,
+              root.InitByTagLookup(syncer::ModelTypeToRootTag(kType)));
+    for (int i = 0; i < n; ++i) {
+      syncer::WriteNode node(&trans);
+      node.InitUniqueByCreation(kType, root, base::StringPrintf("node%05d", i));
+    }
+  }
+
+  GenericChangeProcessor* change_processor() {
+    return change_processor_.get();
+  }
+
+  syncer::UserShare* user_share() {
+    return test_user_share_.user_share();
+  }
+
+ private:
+  MessageLoop loop;
+
+  syncer::SyncMergeResult sync_merge_result_;
+  base::WeakPtrFactory<syncer::SyncMergeResult> merge_result_ptr_factory_;
+
+  syncer::FakeSyncableService fake_syncable_service_;
+  base::WeakPtrFactory<syncer::FakeSyncableService>
+      syncable_service_ptr_factory_;
+
+  DataTypeErrorHandlerMock data_type_error_handler_;
+  syncer::TestUserShare test_user_share_;
+
+  scoped_ptr<GenericChangeProcessor> change_processor_;
+};
+
+// This test exercises GenericChangeProcessor's GetSyncDataForType function.
+// It's not a great test, but, by modifying some of the parameters, you could
+// turn it into a micro-benchmark for model association.
+TEST_F(GenericChangeProcessorTest, StressGetSyncDataForType) {
+  const int kNumChildNodes = 1000;
+  const int kRepeatCount = 1;
+
+  ASSERT_NO_FATAL_FAILURE(BuildChildNodes(kNumChildNodes));
+
+  for (int i = 0; i < kRepeatCount; ++i) {
+    syncer::SyncDataList sync_data;
+    change_processor()->GetSyncDataForType(kType, &sync_data);
+
+    // Start with a simple test.  We can add more in-depth testing later.
+    EXPECT_EQ(static_cast<size_t>(kNumChildNodes), sync_data.size());
+  }
+}
+
+}  // namespace
+
+}  // namespace browser_sync
+

chrome/chrome_tests_unit.gypi

@@ -1184,6 +1184,7 @@
         'browser/sync/glue/frontend_data_type_controller_mock.cc',
         'browser/sync/glue/frontend_data_type_controller_mock.h',
         'browser/sync/glue/frontend_data_type_controller_unittest.cc',
+        'browser/sync/glue/generic_change_processor_unittest.cc',
         'browser/sync/glue/model_association_manager_unittest.cc',
         'browser/sync/glue/model_associator_mock.cc',
         'browser/sync/glue/model_associator_mock.h',

sync/internal_api/base_node.cc

@@ -215,6 +215,10 @@ int64 BaseNode::GetFirstChildId() const {
   return IdToMetahandle(GetTransaction()->GetWrappedTrans(), id_string);
 }
 
+void BaseNode::GetChildIds(std::vector<int64>* result) const {
+  GetEntry()->GetChildHandles(result);
+}
+
 int BaseNode::GetTotalNodeCount() const {
   syncable::BaseTransaction* trans = GetTransaction()->GetWrappedTrans();
 

sync/internal_api/public/base_node.h

@@ -195,6 +195,11 @@ class SYNC_EXPORT BaseNode {
   // children, return 0.
   int64 GetFirstChildId() const;
 
+  // Returns the IDs of the children of this node.
+  // If this type supports user-defined positions the returned IDs will be in
+  // the correct order.
+  void GetChildIds(std::vector<int64>* result) const;
+
   // Returns the total number of nodes including and beneath this node.
   // Recursively iterates through all children.
   int GetTotalNodeCount() const;

sync/syncable/entry.cc

@@ -104,6 +104,10 @@ Id Entry::GetFirstChildId() const {
   return dir()->GetFirstChildId(basetrans_, kernel_);
 }
 
+void Entry::GetChildHandles(std::vector<int64>* result) const {
+  dir()->GetChildHandlesById(basetrans_, Get(ID), result);
+}
+
 bool Entry::ShouldMaintainPosition() const {
   return kernel_->ShouldMaintainPosition();
 }

sync/syncable/entry.h

@@ -109,6 +109,12 @@ class SYNC_EXPORT Entry {
   Id GetSuccessorId() const;
   Id GetFirstChildId() const;
 
+  // Returns a vector of this node's children's handles.
+  // Clears |result| if there are no children.  If this node is of a type that
+  // supports user-defined ordering then the resulting vector will be in the
+  // proper order.
+  void GetChildHandles(std::vector<int64>* result) const;
+
   inline bool ExistsOnClientBecauseNameIsNonEmpty() const {
     DCHECK(kernel_);
     return !kernel_->ref(NON_UNIQUE_NAME).empty();