Sync: Optimize FavIcon callbacks from bookmark model.

Sync: Perf: Optimize handing FavIcon callbacks in BookmarkChangeProcessor

This change improves responsiveness of chrome UI thread by eliminating wasted work that BookmarkChangeProcessor
does in BookmarkNodeFaviconChanged in a scenario when user initially connects sync to an account with a large
number of bookmarks. This is achieved by ignoring the notification earlier when there is no FavIcon image, before doing any expensive work. Please note that the notification is called twice for each bookmark - first when a change in FavIcon is detected and then when the icon is uploaded to the bookmark model. The change cuts the work associated with the first call on the sync side.

Estimated saving on the UI thread is about 10-12% (as measured by reduction of samples in sampling profile).

BUG=239621
Committed: https://crrev.com/d38ccdc3e21501cc81753431ececd0856df87f7c
Cr-Commit-Position: refs/heads/master@{#302685}

[stanisc, 2014-10-17 17:05:27]

Patchset #1 (id:1) has been deleted

[stanisc, 2014-10-17 18:05:10]

stanisc@chromium.org changed reviewers:
+ zea@chromium.org

[stanisc, 2014-10-17 18:05:10]

Please review this change.

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

https://codereview.chromium.org/661883002/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/bookmark_change_processor.cc (right):

https://codereview.chromium.org/661883002/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/bookmark_change_processor.cc:424: const gfx::Image&
favicon = model->GetFavicon(node);
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?

https://codereview.chromium.org/661883002/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/bookmark_change_processor.cc:927: // Check for changes
before updating. When an favicon is initially set by
nit: "When an favicon" -> "When a favicon"

https://codereview.chromium.org/661883002/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/bookmark_change_processor.cc:929: //
BookmarkNodeFaviconChanged notification. This allows to avoid unnecessary
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)

https://codereview.chromium.org/661883002/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/bookmark_change_processor.cc:936:
favicon_bytes->size()) != 0) {
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]

I'll do some further experiments when I have time to see if I can come up with a
simple fix with nearly the same benefits.

https://codereview.chromium.org/661883002/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/bookmark_change_processor.cc (right):

https://codereview.chromium.org/661883002/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/bookmark_change_processor.cc:424: const gfx::Image&
favicon = model->GetFavicon(node);
On 2014/10/20 18:04:46, Nicolas Zea wrote:
> 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?

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.

https://codereview.chromium.org/661883002/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/bookmark_change_processor.cc:936:
favicon_bytes->size()) != 0) {
On 2014/10/20 18:04:46, Nicolas Zea wrote:
> 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?

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.

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

I went back to a much smaller change for now to keep this simple. I kept the
check whether the icon image exists in
BookmarkChangeProcessor::BookmarkNodeFaviconChanged and reverted all other
changes for now. 
This limited change gets about 50% of the performance gain of the initial
change. Let's start with this simple change, we can always reapply more
optimizations later.

https://codereview.chromium.org/661883002/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/bookmark_change_processor.cc (right):

https://codereview.chromium.org/661883002/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/bookmark_change_processor.cc:927: // Check for changes
before updating. When an favicon is initially set by
On 2014/10/20 18:04:46, Nicolas Zea wrote:
> nit: "When an favicon" -> "When a favicon"

Removed this code.

https://codereview.chromium.org/661883002/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/bookmark_change_processor.cc:929: //
BookmarkNodeFaviconChanged notification. This allows to avoid unnecessary
On 2014/10/20 18:04:46, Nicolas Zea wrote:
> 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)

Removed this code for now.

[Nicolas Zea, 2014-11-04 18:51:53]

lgtm

[Nicolas Zea, 2014-11-04 18:52:09]

Although probably need to update the commit message.

[stanisc, 2014-11-04 21:29:58]

The CQ bit was checked by stanisc@chromium.org

[commit-bot: I haz the power, 2014-11-04 21:31:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661883002/40001

[commit-bot: I haz the power, 2014-11-04 22:29:19]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2014-11-04 22:30:31]

Patchset 2 (id:??) landed as
https://crrev.com/d38ccdc3e21501cc81753431ececd0856df87f7c
Cr-Commit-Position: refs/heads/master@{#302685}