[Sync] Speed up Javascript node operations

Changed getNodeById to getNodesById and added getChildNodeIds.  This
avoids having to loop through a node's children in Javascript, which
is slow.

This speeds up expanding the "Autofill" node in the sync node browser
by 40-50% (with ~2700 elements).  With a debug Chromium, time spent
retrieving the nodes goes from 30s to 18s, and with a release Chromium
time it goes from 2.9s to 1.3s.

This will also speed up search, which will be implemented shortly.

Add timer logs to some sync node browser operations.

Fix docs for some functions called from Javascript.
BUG=76812
TEST=

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

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

5 files changed, 263 insertions, 75 deletions

diff --git a/chrome/browser/resources/sync_internals/chrome_sync.js b/chrome/browser/resources/sync_internals/chrome_sync.js
index de1e292..e0eb4cf 100644
--- a/chrome/browser/resources/sync_internals/chrome_sync.js
+++ b/chrome/browser/resources/sync_internals/chrome_sync.js
@@ -140,7 +140,8 @@ var syncFunctions = [
 
   // Node lookup functions.
   'getRootNode',
-  'getNodeById',
+  'getNodesById',
+  'getChildNodeIds',
   'findNodesContainingString'
 ];
 
diff --git a/chrome/browser/resources/sync_internals/sync_node_browser.js b/chrome/browser/resources/sync_internals/sync_node_browser.js
index 4939337..3ce4469 100644
--- a/chrome/browser/resources/sync_internals/sync_node_browser.js
+++ b/chrome/browser/resources/sync_internals/sync_node_browser.js
@@ -18,6 +18,23 @@ cr.define('chrome.sync', function() {
   // hide all these details.
 
   /**
+   * Returns an object which measures elapsed time.
+   */
+  var makeTimer = function() {
+    var start = new Date();
+
+    return {
+      /**
+       * @return {number} The number of seconds since the timer was
+       * created.
+       */
+      get elapsedSeconds() {
+        return ((new Date()).getTime() - start.getTime()) / 1000.0;
+      }
+    };
+  };
+
+  /**
    * Gets all children of the given node and passes it to the given
    * callback.
    * @param {Object} nodeInfo The info for the node whose children we
@@ -26,16 +43,19 @@ cr.define('chrome.sync', function() {
    *     children.
    */
   function getSyncNodeChildren(nodeInfo, callback) {
-    var children = [];
-    function processChildInfo(childNodeInfo) {
-      if (!childNodeInfo) {
+    var timer = makeTimer();
+    chrome.sync.getChildNodeIds(nodeInfo.id, function(childNodeIds) {
+      console.debug('getChildNodeIds took ' +
+                    timer.elapsedSeconds + 's to retrieve ' +
+                    childNodeIds.length + ' ids');
+      timer = makeTimer();
+      chrome.sync.getNodesById(childNodeIds, function(children) {
+        console.debug('getNodesById took ' +
+                      timer.elapsedSeconds + 's to retrieve ' +
+                      children.length + ' nodes');
         callback(children);
-        return;
-      }
-      children.push(childNodeInfo);
-      chrome.sync.getNodeById(childNodeInfo.successorId, processChildInfo);
-    };
-    chrome.sync.getNodeById(nodeInfo.firstChildId, processChildInfo);
+      });
+    });
   }
 
   /**
@@ -59,10 +79,13 @@ cr.define('chrome.sync', function() {
       treeItem.addEventListener('expand', function(event) {
         if (!treeItem.triggeredLoad_) {
           getSyncNodeChildren(nodeInfo, function(children) {
+            var timer = makeTimer();
             for (var i = 0; i < children.length; ++i) {
               var childTreeItem = makeNodeTreeItem(children[i]);
               treeItem.add(childTreeItem);
             }
+            console.debug('adding ' + children.length + ' children took ' +
+                          timer.elapsedSeconds + 's');
           });
           treeItem.triggeredLoad_ = true;
         }
diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc
index 8c0eec1..ca11c34 100644
--- a/chrome/browser/sync/engine/syncapi.cc
+++ b/chrome/browser/sync/engine/syncapi.cc
@@ -1548,7 +1548,10 @@ class SyncManager::SyncInternal
   void OnIPAddressChangedImpl();
 
   // Functions called by ProcessMessage().
-  browser_sync::JsArgList ProcessGetNodeByIdMessage(
+  browser_sync::JsArgList ProcessGetNodesByIdMessage(
+      const browser_sync::JsArgList& args);
+
+  browser_sync::JsArgList ProcessGetChildNodeIdsMessage(
       const browser_sync::JsArgList& args);
 
   browser_sync::JsArgList ProcessFindNodesContainingString(
@@ -2719,13 +2722,20 @@ void SyncManager::SyncInternal::ProcessMessage(
     return_args.Append(root.ToValue());
     parent_router_->RouteJsMessageReply(
         name, browser_sync::JsArgList(&return_args), sender);
-  } else if (name == "getNodeById") {
+  } else if (name == "getNodesById") {
+    if (!parent_router_) {
+      LogNoRouter(name, args);
+      return;
+    }
+    parent_router_->RouteJsMessageReply(
+        name, ProcessGetNodesByIdMessage(args), sender);
+  } else if (name == "getChildNodeIds") {
     if (!parent_router_) {
       LogNoRouter(name, args);
       return;
     }
     parent_router_->RouteJsMessageReply(
-        name, ProcessGetNodeByIdMessage(args), sender);
+        name, ProcessGetChildNodeIdsMessage(args), sender);
   } else if (name == "findNodesContainingString") {
     if (!parent_router_) {
       LogNoRouter(name, args);
@@ -2758,29 +2768,70 @@ void SyncManager::SyncInternal::RouteJsMessageReply(
   parent_router_->RouteJsMessageReply(name, args, target);
 }
 
-browser_sync::JsArgList SyncManager::SyncInternal::ProcessGetNodeByIdMessage(
-    const browser_sync::JsArgList& args) {
-  ListValue null_return_args_list;
-  null_return_args_list.Append(Value::CreateNullValue());
-  browser_sync::JsArgList null_return_args(&null_return_args_list);
+namespace {
+
+bool GetId(const ListValue& ids, int i, int64* id) {
   std::string id_str;
-  if (!args.Get().GetString(0, &id_str)) {
-    return null_return_args;
+  if (!ids.GetString(i, &id_str)) {
+    return false;
   }
-  int64 id;
-  if (!base::StringToInt64(id_str, &id)) {
-    return null_return_args;
+  if (!base::StringToInt64(id_str, id)) {
+    return false;
   }
-  if (id == kInvalidId) {
-    return null_return_args;
+  if (*id == kInvalidId) {
+    return false;
   }
+  return true;
+}
+
+}  // namespace
+
+browser_sync::JsArgList SyncManager::SyncInternal::ProcessGetNodesByIdMessage(
+    const browser_sync::JsArgList& args) {
+  ListValue return_args;
+  ListValue* nodes = new ListValue();
+  return_args.Append(nodes);
+  ListValue* id_list = NULL;
   ReadTransaction trans(GetUserShare());
-  ReadNode node(&trans);
-  if (!node.InitByIdLookup(id)) {
-    return null_return_args;
+  if (args.Get().GetList(0, &id_list)) {
+    for (size_t i = 0; i < id_list->GetSize(); ++i) {
+      int64 id = kInvalidId;
+      if (!GetId(*id_list, i, &id)) {
+        continue;
+      }
+      ReadNode node(&trans);
+      if (!node.InitByIdLookup(id)) {
+        continue;
+      }
+      nodes->Append(node.ToValue());
+    }
   }
+  return browser_sync::JsArgList(&return_args);
+}
+
+browser_sync::JsArgList SyncManager::SyncInternal::
+    ProcessGetChildNodeIdsMessage(
+        const browser_sync::JsArgList& args) {
   ListValue return_args;
-  return_args.Append(node.ToValue());
+  ListValue* child_ids = new ListValue();
+  return_args.Append(child_ids);
+  int64 id = kInvalidId;
+  if (GetId(args.Get(), 0, &id)) {
+    ReadTransaction trans(GetUserShare());
+    ReadNode node(&trans);
+    if (node.InitByIdLookup(id)) {
+      int64 child_id = node.GetFirstChildId();
+      while (child_id != kInvalidId) {
+        ReadNode child_node(&trans);
+        if (!child_node.InitByIdLookup(child_id)) {
+          break;
+        }
+        child_ids->Append(Value::CreateStringValue(
+            base::Int64ToString(child_id)));
+        child_id = child_node.GetSuccessorId();
+      }
+    }
+  }
   return browser_sync::JsArgList(&return_args);
 }
 
diff --git a/chrome/browser/sync/engine/syncapi.h b/chrome/browser/sync/engine/syncapi.h
index f51c574..c454c40 100644
--- a/chrome/browser/sync/engine/syncapi.h
+++ b/chrome/browser/sync/engine/syncapi.h
@@ -979,22 +979,24 @@ class SyncManager {
   // The following messages are processed by the returned backend:
   //
   // getNotificationState():
-  //   If there is a parent router, sends the
-  //   onGetNotificationStateFinished(boolean notificationsEnabled)
-  //   event to |sender| via the parent router with whether or not
-  //   notifications are enabled.
+  //   callback(boolean notificationsEnabled):
+  //     notificationsEnabled: whether or not notifications are
+  //     enabled.
   //
   // getRootNode():
-  //   If there is a parent router, sends the
-  //   onGetRootNodeFinished(dictionary nodeInfo) event to |sender|
-  //   via the parent router with information on the root node.
+  //   callback(dictionary nodeInfo):
+  //     nodeInfo: Information on the root node.
   //
-  // getNodeById(string id):
-  //   If there is a parent router, sends the
-  //   onGetNodeByIdFinished(dictionary nodeInfo) event to |sender|
-  //   via the parent router with information on the node with the
-  //   given id (metahandle), if the id is valid and a node with that
-  //   id exists.  Otherwise, calls onGetNodeByIdFinished(null).
+  // getNodesById(array idList):
+  //   idList: A list of IDs as strings.
+  //   callback(array nodeList):
+  //     nodeList: Information on each node for each valid id in
+  //     idList.  Not guaranteed to be in any order.
+  //
+  // getChildNodeIds(string id):
+  //   id: The id of the node for which to return the child node ids.
+  //   callback(array idList):
+  //     idList: The child node IDs of the node with the given id.
   //
   // All other messages are dropped.
   browser_sync::JsBackend* GetJsBackend();
diff --git a/chrome/browser/sync/engine/syncapi_unittest.cc b/chrome/browser/sync/engine/syncapi_unittest.cc
index ba18eb6..229947b 100644
--- a/chrome/browser/sync/engine/syncapi_unittest.cc
+++ b/chrome/browser/sync/engine/syncapi_unittest.cc
@@ -871,23 +871,24 @@ TEST_F(SyncManagerTest, ProcessMessageGetRootNode) {
   js_backend->RemoveParentJsEventRouter();
 }
 
-void CheckGetNodeByIdReturnArgs(const SyncManager& sync_manager,
-                                const JsArgList& return_args,
-                                int64 id) {
+void CheckGetNodesByIdReturnArgs(const SyncManager& sync_manager,
+                                 const JsArgList& return_args,
+                                 int64 id) {
   EXPECT_EQ(1u, return_args.Get().GetSize());
+  ListValue* nodes = NULL;
+  ASSERT_TRUE(return_args.Get().GetList(0, &nodes));
+  ASSERT_TRUE(nodes);
+  EXPECT_EQ(1u, nodes->GetSize());
   DictionaryValue* node_info = NULL;
-  EXPECT_TRUE(return_args.Get().GetDictionary(0, &node_info));
-  if (node_info) {
-    ReadTransaction trans(sync_manager.GetUserShare());
-    ReadNode node(&trans);
-    node.InitByIdLookup(id);
-    CheckNodeValue(node, *node_info);
-  } else {
-    ADD_FAILURE();
-  }
+  EXPECT_TRUE(nodes->GetDictionary(0, &node_info));
+  ASSERT_TRUE(node_info);
+  ReadTransaction trans(sync_manager.GetUserShare());
+  ReadNode node(&trans);
+  node.InitByIdLookup(id);
+  CheckNodeValue(node, *node_info);
 }
 
-TEST_F(SyncManagerTest, ProcessMessageGetNodeById) {
+TEST_F(SyncManagerTest, ProcessMessageGetNodesById) {
   int64 child_id =
       MakeNode(sync_manager_.GetUserShare(), syncable::BOOKMARKS, "testtag");
 
@@ -899,7 +900,7 @@ TEST_F(SyncManagerTest, ProcessMessageGetNodeById) {
   JsArgList return_args;
 
   EXPECT_CALL(event_router,
-              RouteJsMessageReply("getNodeById", _, &event_handler))
+              RouteJsMessageReply("getNodesById", _, &event_handler))
       .Times(2).WillRepeatedly(SaveArg<1>(&return_args));
 
   js_backend->SetParentJsEventRouter(&event_router);
@@ -907,70 +908,180 @@ TEST_F(SyncManagerTest, ProcessMessageGetNodeById) {
   // Should trigger the reply.
   {
     ListValue args;
-    args.Append(Value::CreateStringValue("1"));
-    js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler);
+    ListValue* ids = new ListValue();
+    args.Append(ids);
+    ids->Append(Value::CreateStringValue("1"));
+    js_backend->ProcessMessage("getNodesById",
+                               JsArgList(&args), &event_handler);
   }
 
-  CheckGetNodeByIdReturnArgs(sync_manager_, return_args, 1);
+  CheckGetNodesByIdReturnArgs(sync_manager_, return_args, 1);
 
   // Should trigger another reply.
   {
     ListValue args;
-    args.Append(Value::CreateStringValue(base::Int64ToString(child_id)));
-    js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler);
+    ListValue* ids = new ListValue();
+    args.Append(ids);
+    ids->Append(Value::CreateStringValue(base::Int64ToString(child_id)));
+    js_backend->ProcessMessage("getNodesById",
+                               JsArgList(&args), &event_handler);
   }
 
-  CheckGetNodeByIdReturnArgs(sync_manager_, return_args, child_id);
+  CheckGetNodesByIdReturnArgs(sync_manager_, return_args, child_id);
 
   js_backend->RemoveParentJsEventRouter();
 }
 
-TEST_F(SyncManagerTest, ProcessMessageGetNodeByIdFailure) {
+TEST_F(SyncManagerTest, ProcessMessageGetNodesByIdFailure) {
   browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
 
   StrictMock<MockJsEventHandler> event_handler;
   StrictMock<MockJsEventRouter> event_router;
 
-  ListValue null_args;
-  null_args.Append(Value::CreateNullValue());
+  ListValue empty_list_args;
+  empty_list_args.Append(new ListValue());
 
   EXPECT_CALL(event_router,
-              RouteJsMessageReply("getNodeById", HasArgsAsList(null_args),
+              RouteJsMessageReply("getNodesById",
+                                  HasArgsAsList(empty_list_args),
                                   &event_handler))
-      .Times(5);
+      .Times(6);
 
   js_backend->SetParentJsEventRouter(&event_router);
 
   {
     ListValue args;
-    js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler);
+    js_backend->ProcessMessage("getNodesById",
+                               JsArgList(&args), &event_handler);
   }
 
   {
     ListValue args;
-    args.Append(Value::CreateStringValue(""));
-    js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler);
+    args.Append(new ListValue());
+    js_backend->ProcessMessage("getNodesById",
+                               JsArgList(&args), &event_handler);
   }
 
   {
     ListValue args;
-    args.Append(Value::CreateStringValue("nonsense"));
-    js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler);
+    ListValue* ids = new ListValue();
+    args.Append(ids);
+    ids->Append(Value::CreateStringValue(""));
+    js_backend->ProcessMessage("getNodesById",
+                               JsArgList(&args), &event_handler);
+  }
+
+  {
+    ListValue args;
+    ListValue* ids = new ListValue();
+    args.Append(ids);
+    ids->Append(Value::CreateStringValue("nonsense"));
+    js_backend->ProcessMessage("getNodesById",
+                               JsArgList(&args), &event_handler);
+  }
+
+  {
+    ListValue args;
+    ListValue* ids = new ListValue();
+    args.Append(ids);
+    ids->Append(Value::CreateStringValue("0"));
+    js_backend->ProcessMessage("getNodesById",
+                               JsArgList(&args), &event_handler);
+  }
+
+  {
+    ListValue args;
+    ListValue* ids = new ListValue();
+    args.Append(ids);
+    ids->Append(Value::CreateStringValue("9999"));
+    js_backend->ProcessMessage("getNodesById",
+                               JsArgList(&args), &event_handler);
+  }
+
+  js_backend->RemoveParentJsEventRouter();
+}
+
+TEST_F(SyncManagerTest, ProcessMessageGetChildNodeIds) {
+  browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
+
+  StrictMock<MockJsEventHandler> event_handler;
+  StrictMock<MockJsEventRouter> event_router;
+
+  JsArgList return_args;
+
+  EXPECT_CALL(event_router,
+              RouteJsMessageReply("getChildNodeIds", _, &event_handler))
+      .Times(1).WillRepeatedly(SaveArg<1>(&return_args));
+
+  js_backend->SetParentJsEventRouter(&event_router);
+
+  // Should trigger the reply.
+  {
+    ListValue args;
+    args.Append(Value::CreateStringValue("1"));
+    js_backend->ProcessMessage("getChildNodeIds",
+                               JsArgList(&args), &event_handler);
+  }
+
+  EXPECT_EQ(1u, return_args.Get().GetSize());
+  ListValue* nodes = NULL;
+  ASSERT_TRUE(return_args.Get().GetList(0, &nodes));
+  ASSERT_TRUE(nodes);
+  EXPECT_EQ(5u, nodes->GetSize());
+
+  js_backend->RemoveParentJsEventRouter();
+}
+
+TEST_F(SyncManagerTest, ProcessMessageGetChildNodeIdsFailure) {
+  browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
+
+  StrictMock<MockJsEventHandler> event_handler;
+  StrictMock<MockJsEventRouter> event_router;
+
+  ListValue empty_list_args;
+  empty_list_args.Append(new ListValue());
+
+  EXPECT_CALL(event_router,
+              RouteJsMessageReply("getChildNodeIds",
+                                  HasArgsAsList(empty_list_args),
+                                  &event_handler))
+      .Times(5);
+
+  js_backend->SetParentJsEventRouter(&event_router);
+
+  {
+    ListValue args;
+    js_backend->ProcessMessage("getChildNodeIds",
+                               JsArgList(&args), &event_handler);
+  }
+
+  {
+    ListValue args;
+    args.Append(Value::CreateStringValue(""));
+    js_backend->ProcessMessage("getChildNodeIds",
+                               JsArgList(&args), &event_handler);
   }
 
   {
     ListValue args;
     args.Append(Value::CreateStringValue("nonsense"));
-    js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler);
+    js_backend->ProcessMessage("getChildNodeIds",
+                               JsArgList(&args), &event_handler);
   }
 
   {
     ListValue args;
     args.Append(Value::CreateStringValue("0"));
-    js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler);
+    js_backend->ProcessMessage("getChildNodeIds",
+                               JsArgList(&args), &event_handler);
   }
 
-  // TODO(akalin): Figure out how to test InitByIdLookup() failure.
+  {
+    ListValue args;
+    args.Append(Value::CreateStringValue("9999"));
+    js_backend->ProcessMessage("getChildNodeIds",
+                               JsArgList(&args), &event_handler);
+  }
 
   js_backend->RemoveParentJsEventRouter();
 }