[Sync] Speed up sync node browser/search in about:sync

[Sync] Speed up sync node browser/search in about:sync

Also lay the groundwork for an auto-refreshing sync node browser.

In syncable/, add GetChildHandlesByHandle method and expose
GetAllMetaHandles.

In syncapi.{h,cc}, add getNodeSummariesById message.  Also make
findNodesContainingString message use GetAllMetaHandles.

Finally, in the about:sync Javascript code, use getNodeSummariesById
when possible.  This speeds up large operations (e.g., opening up the
Autofill folder, searching for a single letter) by an order of magnitude
or so.

Cleaned up docs for some Javascript functions.

BUG=76811
TEST=


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

[akalin, 2011-06-02 08:29:12]

+lipalani for review.

Let me know if you need me to walk through the Javascript changes.

[lipalani1, 2011-06-02 18:51:52]

I would like a walk through with you for the js code and the js plumbing in
syncapi.cc. 

I have reviewed the rest of the stuff and here are my comments.

Thanks and a pretty good change by the way :)

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:2789: DictionaryValue*
(BaseNode::*info_getter)() const) {
CHECK(info_getter != NULL)

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:2795: if (args.Get().GetList(0, &id_list))
{
We should check if id_list is null and execute the for only if it is not null.

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:2798: if (id == kInvalidId) {
Should we do a DCHECK here and in the InitByIdLookup?

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:2868:
trans.GetLookup()->GetAllMetaHandles(trans.GetWrappedTrans(), &all_handles);
May be we can have this as a TODO if you think this cannot be done easily.
Instead of getting the handles and then doing a search on the id to get the
entry we can get the list of entries itself(It is just a pointer in the Entry
object). And then initialize the node from it.

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi_unittest.cc (right):

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi_unittest.cc:936:
ids->Append(Value::CreateStringValue("1"));
shouldnt this be child_id?

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/syncable/sy...
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.cc:513: result->clear();
Can we move result->clear to the private function?

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.cc:515: ScopedKernelLock lock(this);
If the result->clear is moved is the block necessary.

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.cc:1965: const Id& parent_id,
Can this function be const? Also the callers? I think the lock could be a
problem...

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.cc:1966: Directory::ChildHandles* result)
{
Nit - CHECK(result != NULL)

[akalin, 2011-06-02 22:13:33]

Addressed all comments, please take another look.

Also, I can walk through the JS code with you whenever you're ready.

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:2789: DictionaryValue*
(BaseNode::*info_getter)() const) {
On 2011/06/02 18:51:52, lipalani1 wrote:
> CHECK(info_getter != NULL)

Done.  Chrome/Google style is CHECK(ptr);

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:2795: if (args.Get().GetList(0, &id_list))
{
On 2011/06/02 18:51:52, lipalani1 wrote:
> We should check if id_list is null and execute the for only if it is not null.

GetList() guarantees that it fills in id_list with a non-NULL value if it
returns true.  Added CHECK.

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:2798: if (id == kInvalidId) {
On 2011/06/02 18:51:52, lipalani1 wrote:
> Should we do a DCHECK here and in the InitByIdLookup?

Nah, we don't want to crash if the javascript code passes an invalid ID down;
that ID may have been valid previously but became invalid.

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:2868:
trans.GetLookup()->GetAllMetaHandles(trans.GetWrappedTrans(), &all_handles);
On 2011/06/02 18:51:52, lipalani1 wrote:
> May be we can have this as a TODO if you think this cannot be done easily.
> Instead of getting the handles and then doing a search on the id to get the
> entry we can get the list of entries itself(It is just a pointer in the Entry
> object). And then initialize the node from it. 

Added GetAllEntryKernels function and use that instead.

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi_unittest.cc (right):

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi_unittest.cc:936:
ids->Append(Value::CreateStringValue("1"));
On 2011/06/02 18:51:52, lipalani1 wrote:
> shouldnt this be child_id?

It should be the id of the root, actually.  Refactored test to make it more
obvious.

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/syncable/sy...
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.cc:513: result->clear();
On 2011/06/02 18:51:52, lipalani1 wrote:
> Can we move result->clear to the private function?

I don't think we should; the other function (GetChildHandlesByHandle needs to
clear it itself because it can return before calling GetChildHandles.

Renamed GetChildHandles to AppendChildHandles to make it more obvious.

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.cc:515: ScopedKernelLock lock(this);
On 2011/06/02 18:51:52, lipalani1 wrote:
> If the result->clear is moved is the block necessary.

The block isn't necessary anyway, so removed it.

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.cc:1965: const Id& parent_id,
On 2011/06/02 18:51:52, lipalani1 wrote:
> Can this function be const? Also the callers? I think the lock could be a
> problem...

In this case, it isn't the lock, but the fact that the function it calls
modifies kernel_->needle, which is used as a scratch variable.  Ugh.

http://codereview.chromium.org/7033043/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.cc:1966: Directory::ChildHandles* result)
{
On 2011/06/02 18:51:52, lipalani1 wrote:
> Nit - CHECK(result != NULL)

Done.

[lipalani1, 2011-06-02 22:37:34]

all changes looks good. let us walk through the JS code. thx

[lipalani1, 2011-06-02 23:06:39]

just one comment. rest makes sense.

http://codereview.chromium.org/7033043/diff/4003/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7033043/diff/4003/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1207: 
This list does not seem to change. So is it possible to just initialize the map
directly and make it a const?

[akalin, 2011-06-02 23:14:58]

http://codereview.chromium.org/7033043/diff/4003/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7033043/diff/4003/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1207: 
On 2011/06/02 23:06:39, lipalani1 wrote:
> This list does not seem to change. So is it possible to just initialize the
map
> directly and make it a const?

As we discussed, C++ isn't expressive enough to do this. :(

[lipalani1, 2011-06-02 23:15:02]

LGTM.
thx

[commit-bot: I haz the power, 2011-06-03 00:26:03]

Change committed as 87736