From 414df42f095ab7188272918cdc727f10e76bd3c2 Mon Sep 17 00:00:00 2001 From: "rlarocque@chromium.org" Date: Tue, 28 May 2013 22:28:54 +0000 Subject: [PATCH] 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 --- .../sync/glue/generic_change_processor.cc | 10 +- .../glue/generic_change_processor_unittest.cc | 109 ++++++++++++++++++ chrome/chrome_tests_unit.gypi | 1 + sync/internal_api/base_node.cc | 4 + sync/internal_api/public/base_node.h | 5 + sync/syncable/entry.cc | 4 + sync/syncable/entry.h | 6 + 7 files changed, 135 insertions(+), 4 deletions(-) create mode 100644 chrome/browser/sync/glue/generic_change_processor_unittest.cc diff --git a/chrome/browser/sync/glue/generic_change_processor.cc b/chrome/browser/sync/glue/generic_change_processor.cc index 38d952c8aba9..41a14110f8d9 100644 --- a/chrome/browser/sync/glue/generic_change_processor.cc +++ b/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 child_ids; + root.GetChildIds(&child_ids); + + for (std::vector::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(); } diff --git a/chrome/browser/sync/glue/generic_change_processor_unittest.cc b/chrome/browser/sync/glue/generic_change_processor_unittest.cc new file mode 100644 index 000000000000..6ab8a6182149 --- /dev/null +++ b/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 merge_result_ptr_factory_; + + syncer::FakeSyncableService fake_syncable_service_; + base::WeakPtrFactory + syncable_service_ptr_factory_; + + DataTypeErrorHandlerMock data_type_error_handler_; + syncer::TestUserShare test_user_share_; + + scoped_ptr 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(kNumChildNodes), sync_data.size()); + } +} + +} // namespace + +} // namespace browser_sync + diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index 6db344775139..d24f7e574a22 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/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', diff --git a/sync/internal_api/base_node.cc b/sync/internal_api/base_node.cc index 88dec8cbd36f..f9bf79b67ab8 100644 --- a/sync/internal_api/base_node.cc +++ b/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* result) const { + GetEntry()->GetChildHandles(result); +} + int BaseNode::GetTotalNodeCount() const { syncable::BaseTransaction* trans = GetTransaction()->GetWrappedTrans(); diff --git a/sync/internal_api/public/base_node.h b/sync/internal_api/public/base_node.h index 3c56b5ebe18e..6c54ce2ad108 100644 --- a/sync/internal_api/public/base_node.h +++ b/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* result) const; + // Returns the total number of nodes including and beneath this node. // Recursively iterates through all children. int GetTotalNodeCount() const; diff --git a/sync/syncable/entry.cc b/sync/syncable/entry.cc index 2d98c6fdff6d..ac6244852c51 100644 --- a/sync/syncable/entry.cc +++ b/sync/syncable/entry.cc @@ -104,6 +104,10 @@ Id Entry::GetFirstChildId() const { return dir()->GetFirstChildId(basetrans_, kernel_); } +void Entry::GetChildHandles(std::vector* result) const { + dir()->GetChildHandlesById(basetrans_, Get(ID), result); +} + bool Entry::ShouldMaintainPosition() const { return kernel_->ShouldMaintainPosition(); } diff --git a/sync/syncable/entry.h b/sync/syncable/entry.h index f9d3f90cc97b..26ccc0a7b8a6 100644 --- a/sync/syncable/entry.h +++ b/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* result) const; + inline bool ExistsOnClientBecauseNameIsNonEmpty() const { DCHECK(kernel_); return !kernel_->ref(NON_UNIQUE_NAME).empty();