[Sync] Optimize position calculation for bookmarks during remote update

ComputeChildNodeIndex performs linear search to find appropriate index
for the updated or created remote bookmark node. It might lead to
quadratic time during some updates. In practice, this only affects huge
quantities of local bookmarks and updates.

This CL changes the algorithm to logarithmic search.

Bug: 1084150
Change-Id: I605228ad4cd7db82a1e6777a5f4b35538783dde7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2336084
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795019}

components/sync_bookmarks/bookmark_remote_updates_handler.cc

@@ -4,6 +4,7 @@
 
 #include "components/sync_bookmarks/bookmark_remote_updates_handler.h"
 
+#include <algorithm>
 #include <memory>
 #include <set>
 #include <string>
@@ -94,27 +95,40 @@ void TraverseAndAppendChildren(
   }
 }
 
+syncer::UniquePosition ComputeUniquePositionForTrackedBookmarkNode(
+    const SyncedBookmarkTracker* bookmark_tracker,
+    const bookmarks::BookmarkNode* bookmark_node) {
+  DCHECK(bookmark_tracker);
+
+  const SyncedBookmarkTracker::Entity* child_entity =
+      bookmark_tracker->GetEntityForBookmarkNode(bookmark_node);
+  DCHECK(child_entity);
+  // TODO(crbug.com/1113139): precompute UniquePosition to prevent its
+  // calculation on each remote update.
+  return syncer::UniquePosition::FromProto(
+      child_entity->metadata()->unique_position());
+}
+
 size_t ComputeChildNodeIndex(const bookmarks::BookmarkNode* parent,
                              const sync_pb::UniquePosition& unique_position,
                              const SyncedBookmarkTracker* bookmark_tracker) {
-  // TODO(crbug.com/1084150): remove after investigations are completed.
-  TRACE_EVENT0("sync", "ComputeChildNodeIndex");
+  DCHECK(parent);
+  DCHECK(bookmark_tracker);
 
   const syncer::UniquePosition position =
       syncer::UniquePosition::FromProto(unique_position);
-  for (size_t i = 0; i < parent->children().size(); ++i) {
-    const bookmarks::BookmarkNode* child = parent->children()[i].get();
-    const SyncedBookmarkTracker::Entity* child_entity =
-        bookmark_tracker->GetEntityForBookmarkNode(child);
-    DCHECK(child_entity);
-    const syncer::UniquePosition child_position =
-        syncer::UniquePosition::FromProto(
-            child_entity->metadata()->unique_position());
-    if (position.LessThan(child_position)) {
-      return i;
-    }
-  }
-  return parent->children().size();
+
+  auto iter = std::partition_point(
+      parent->children().begin(), parent->children().end(),
+      [bookmark_tracker,
+       position](const std::unique_ptr<bookmarks::BookmarkNode>& child) {
+        // Return true for all |parent|'s children whose position is less than
+        // |position|.
+        return !position.LessThan(ComputeUniquePositionForTrackedBookmarkNode(
+            bookmark_tracker, child.get()));
+      });
+
+  return iter - parent->children().begin();
 }
 
 void ApplyRemoteUpdate(
@@ -428,6 +442,14 @@ BookmarkRemoteUpdatesHandler::ReorderUpdatesForTest(
   return ReorderUpdates(updates);
 }
 
+// static
+size_t BookmarkRemoteUpdatesHandler::ComputeChildNodeIndexForTest(
+    const bookmarks::BookmarkNode* parent,
+    const sync_pb::UniquePosition& unique_position,
+    const SyncedBookmarkTracker* bookmark_tracker) {
+  return ComputeChildNodeIndex(parent, unique_position, bookmark_tracker);
+}
+
 // static
 std::vector<const syncer::UpdateResponseData*>
 BookmarkRemoteUpdatesHandler::ReorderUpdates(

components/sync_bookmarks/bookmark_remote_updates_handler.h

@@ -56,6 +56,11 @@ class BookmarkRemoteUpdatesHandler {
     return valid_updates_without_full_title_;
   }
 
+  static size_t ComputeChildNodeIndexForTest(
+      const bookmarks::BookmarkNode* parent,
+      const sync_pb::UniquePosition& unique_position,
+      const SyncedBookmarkTracker* bookmark_tracker);
+
  private:
   // Reorders incoming updates such that parent creation is before child
   // creation and child deletion is before parent deletion, and deletions should

components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc

@@ -2035,6 +2035,94 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   EXPECT_TRUE(entity->has_final_guid());
 }
 
+TEST(BookmarkRemoteUpdatesHandlerTest,
+     ShouldComputeRightChildNodeIndexForEmptyParent) {
+  const std::string suffix = syncer::UniquePosition::RandomSuffix();
+  const syncer::UniquePosition pos1 =
+      syncer::UniquePosition::InitialPosition(suffix);
+
+  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model =
+      bookmarks::TestBookmarkClient::CreateModel();
+  std::unique_ptr<SyncedBookmarkTracker> tracker =
+      SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata(
+          bookmark_model.get(),
+          CreateMetadataForPermanentNodes(bookmark_model.get()));
+
+  const bookmarks::BookmarkNode* bookmark_bar_node =
+      bookmark_model->bookmark_bar_node();
+
+  // Should always return 0 for any UniquePosition in the initial state.
+  EXPECT_EQ(0u, BookmarkRemoteUpdatesHandler::ComputeChildNodeIndexForTest(
+                    bookmark_bar_node, pos1.ToProto(), tracker.get()));
+}
+
+TEST(BookmarkRemoteUpdatesHandlerTest, ShouldComputeRightChildNodeIndex) {
+  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model =
+      bookmarks::TestBookmarkClient::CreateModel();
+
+  const bookmarks::BookmarkNode* bookmark_bar_node =
+      bookmark_model->bookmark_bar_node();
+  const std::string suffix = syncer::UniquePosition::RandomSuffix();
+
+  const syncer::UniquePosition pos1 =
+      syncer::UniquePosition::InitialPosition(suffix);
+  const syncer::UniquePosition pos2 =
+      syncer::UniquePosition::After(pos1, suffix);
+  const syncer::UniquePosition pos3 =
+      syncer::UniquePosition::After(pos2, suffix);
+
+  // Create 3 nodes using remote update.
+  const bookmarks::BookmarkNode* node1 = bookmark_model->AddFolder(
+      bookmark_bar_node, /*index=*/0, /*title=*/base::string16());
+  const bookmarks::BookmarkNode* node2 = bookmark_model->AddFolder(
+      bookmark_bar_node, /*index=*/1, /*title=*/base::string16());
+  const bookmarks::BookmarkNode* node3 = bookmark_model->AddFolder(
+      bookmark_bar_node, /*index=*/2, /*title=*/base::string16());
+
+  sync_pb::BookmarkModelMetadata model_metadata =
+      CreateMetadataForPermanentNodes(bookmark_model.get());
+  *model_metadata.add_bookmarks_metadata() =
+      CreateNodeMetadata(node1->id(), "folder1_id", pos1);
+  *model_metadata.add_bookmarks_metadata() =
+      CreateNodeMetadata(node2->id(), "folder2_id", pos2);
+  *model_metadata.add_bookmarks_metadata() =
+      CreateNodeMetadata(node3->id(), "folder3_id", pos3);
+
+  std::unique_ptr<SyncedBookmarkTracker> tracker =
+      SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata(
+          bookmark_model.get(), std::move(model_metadata));
+
+  // Check for the same position as existing bookmarks have. In practice this
+  // shouldn't happen.
+  EXPECT_EQ(1u, BookmarkRemoteUpdatesHandler::ComputeChildNodeIndexForTest(
+                    bookmark_bar_node, pos1.ToProto(), tracker.get()));
+  EXPECT_EQ(2u, BookmarkRemoteUpdatesHandler::ComputeChildNodeIndexForTest(
+                    bookmark_bar_node, pos2.ToProto(), tracker.get()));
+  EXPECT_EQ(3u, BookmarkRemoteUpdatesHandler::ComputeChildNodeIndexForTest(
+                    bookmark_bar_node, pos3.ToProto(), tracker.get()));
+
+  EXPECT_EQ(0u, BookmarkRemoteUpdatesHandler::ComputeChildNodeIndexForTest(
+                    bookmark_bar_node,
+                    syncer::UniquePosition::Before(pos1, suffix).ToProto(),
+                    tracker.get()));
+  EXPECT_EQ(1u, BookmarkRemoteUpdatesHandler::ComputeChildNodeIndexForTest(
+                    bookmark_bar_node,
+                    syncer::UniquePosition::Between(/*before=*/pos1,
+                                                    /*after=*/pos2, suffix)
+                        .ToProto(),
+                    tracker.get()));
+  EXPECT_EQ(2u, BookmarkRemoteUpdatesHandler::ComputeChildNodeIndexForTest(
+                    bookmark_bar_node,
+                    syncer::UniquePosition::Between(/*before=*/pos2,
+                                                    /*after=*/pos3, suffix)
+                        .ToProto(),
+                    tracker.get()));
+  EXPECT_EQ(3u, BookmarkRemoteUpdatesHandler::ComputeChildNodeIndexForTest(
+                    bookmark_bar_node,
+                    syncer::UniquePosition::After(pos3, suffix).ToProto(),
+                    tracker.get()));
+}
+
 }  // namespace
 
 }  // namespace sync_bookmarks