Sync: Optimize FavIcon callbacks from bookmark model.

chrome/browser/sync/glue/bookmark_change_processor.cc

 // Copyright 2012 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/bookmark_change_processor.h"
 
 #include <map>
 #include <stack>
 #include <vector>
 
@@ skipping 391 matching lines @@
     }
   }
 
   UpdateTransactionVersion(new_version, model,
                            std::vector<const BookmarkNode*>(1, child));
 }
 
 void BookmarkChangeProcessor::BookmarkNodeFaviconChanged(
     BookmarkModel* model,
     const BookmarkNode* node) {
-  BookmarkNodeChanged(model, node);
+  if (!CanSyncNode(node)) {
+    return;
+  }
+
+  // We shouldn't see changes to the top-level nodes.
+  if (model->is_permanent_node(node)) {
+    NOTREACHED() << "Saw Favicon update to permanent node!";
+    return;
+  }
+
+  // Ignore changes with empty images. This can happen if the favicon is
+  // still being loaded.
+  const gfx::Image& favicon = model->GetFavicon(node);

[Nicolas Zea, 2014/10/20 18:04:46]

it seems like all of this logic, with the exception of the empty favicon check,
is the same as previously? Does it make sense to just do the favicon check
before calling BookmarkNodeChanged?

[stanisc, 2014/10/20 18:39:21]

Not exactly. BookmarkNodeChanges updates everything and that has some extra
cost, including multiple copies of BookmarkSpecifics. A copy of
BookmarkSpecifics internally uses memcpy which where most of the cost is. So
yes, doing just the trivial check will definitely help but it would have a
smaller effect. 
Also please note that this function is called twice - first time to tell the
sync code that the FavIcon has changed (typically the FavIcon doesn't exist at
that point yet), and second time to tell the sync that the FavIcon has loaded.
It appears the loading of FavIcons is done on demand. I know it for sure that it
is triggered by the UI. I suspect it was also triggered for all icons by the
sync code trying to read the icon bytes as a part of BookmarkNodeChanged. So
yes, perhaps doing just the check would be sufficient for practical reasons
because it would eliminate the second callback *for all bookmarks*. I'll double
check that.

+  if (favicon.IsEmpty()) {
+    return;
+  }
+
+  int64 new_version = syncer::syncable::kInvalidTransactionVersion;
+  int64 sync_id = syncer::kInvalidId;
+  {
+    // Acquire a scoped write lock via a transaction.
+    syncer::WriteTransaction trans(FROM_HERE, share_handle(), &new_version);
+    sync_id = model_associator_->GetSyncIdFromChromeId(node->id());
+    if (sync_id != syncer::kInvalidId) {
+      // Lookup the sync node that's associated with |node|.
+      syncer::WriteNode sync_node(&trans);
+      if (!model_associator_->InitSyncNodeFromChromeId(node->id(),
+                                                       &sync_node)) {
+        syncer::SyncError error(FROM_HERE,
+                                syncer::SyncError::DATATYPE_ERROR,
+                                "Failed to init sync node from chrome node",
+                                syncer::BOOKMARKS);
+        error_handler()->OnSingleDataTypeUnrecoverableError(error);
+      }
+      SetSyncNodeFavicon(node, model, &sync_node);
+    } else {
+      NOTREACHED() << "Saw Favicon update to a node unknown to Sync!";
+      return;
+    }
+  }
+
+  UpdateTransactionVersion(
+      new_version, model, std::vector<const BookmarkNode*>(1, node));
 }
 
 void BookmarkChangeProcessor::BookmarkNodeChildrenReordered(
     BookmarkModel* model, const BookmarkNode* node) {
   if (!CanSyncNode(node))
     return;
   int64 new_version = syncer::syncable::kInvalidTransactionVersion;
   std::vector<const BookmarkNode*> children;
   {
     // Acquire a scoped write lock via a transaction.
@@ skipping 450 matching lines @@
 }
 
 // static
 void BookmarkChangeProcessor::SetSyncNodeFavicon(
     const BookmarkNode* bookmark_node,
     BookmarkModel* model,
     syncer::WriteNode* sync_node) {
   scoped_refptr<base::RefCountedMemory> favicon_bytes(NULL);
   EncodeFavicon(bookmark_node, model, &favicon_bytes);
   if (favicon_bytes.get() && favicon_bytes->size()) {
-    sync_pb::BookmarkSpecifics updated_specifics(
-        sync_node->GetBookmarkSpecifics());
-    updated_specifics.set_favicon(favicon_bytes->front(),
-                                  favicon_bytes->size());
-    updated_specifics.set_icon_url(bookmark_node->icon_url().spec());
-    sync_node->SetBookmarkSpecifics(updated_specifics);
+    const sync_pb::BookmarkSpecifics& specifics =
+        sync_node->GetBookmarkSpecifics();
+    // Check for changes before updating. When an favicon is initially set by

[Nicolas Zea, 2014/10/20 18:04:46]

nit: "When an favicon" -> "When a favicon"

[stanisc, 2014/11/03 23:35:35]

Removed this code.

+    // sync for a newly synced bookmark, its favicon comes back to Sync via
+    // BookmarkNodeFaviconChanged notification. This allows to avoid unnecessary

[Nicolas Zea, 2014/10/20 18:04:46]

nit: "This allows to" -> "This allows us to" and "syncer cycle" -> "sync cycle".

Although that said, there shouldn't be a sync cycle if the data hasn't changed
(SetBookmarkSpecifics checks for idempotency)

[stanisc, 2014/11/03 23:35:35]

Removed this code for now.

+    // writing to the directory and the syncer cycle.
+    if (!specifics.has_favicon() || !specifics.has_icon_url() ||
+        specifics.icon_url() != bookmark_node->icon_url().spec() ||
+        specifics.favicon().size() != favicon_bytes->size() ||
+        memcmp(specifics.favicon().c_str(),
+               favicon_bytes->front(),
+               favicon_bytes->size()) != 0) {

[Nicolas Zea, 2014/10/20 18:04:46]

I find it strange that this would result in savings. Take a look at
UpdateEntryWithEncryption in nigori_util.cc. It's basically serializing the
before/after protobufs and checking for idempotency before writing to the
directory. This seems to basically be doing the same (although before creating
the protobuf itself). Is that the only benefit?

[stanisc, 2014/10/20 18:39:21]

It results in savings because it avoids getting a mutable copy of Specifics
which involves memory allocation/copying. That is where the hidden cost is.

+      sync_pb::BookmarkSpecifics updated_specifics(specifics);
+      updated_specifics.set_favicon(favicon_bytes->front(),
+                                    favicon_bytes->size());
+      updated_specifics.set_icon_url(bookmark_node->icon_url().spec());
+      sync_node->SetBookmarkSpecifics(updated_specifics);
+    }
   }
 }
 
 bool BookmarkChangeProcessor::CanSyncNode(const BookmarkNode* node) {
   return bookmark_model_->client()->CanSyncNode(node);
 }
 
 }  // namespace browser_sync