sync: Test and improve bookmark performance

sync: Test and improve bookmark performance

This commit adds a test for bookmark model association.  It's not a
great test in terms of code coverage, but it's possible to turn it into
a useful benchmark by changing the kNumBookmarksPerFolder and
kNumFolders constants.

Also included in this commit is a small change to the WriteTransaction's
SaveOriginal method.  The old code created a temporary
EntryKernelMutation, which had to be cleaned up as it went out of scope.
It turns out that destroying the four EntitySpecifics objects it
contains can be pretty expensive.

The performance impact isn't huge, but it is visible in the profiling
results.  Since the fix is so trivial, the cost-benefit tradeoff is
pretty clearly in favor of fixing this.

BUG=241813, 238621
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201363

[rlarocque, 2013-05-17 22:37:54]

Please review.

The tests and the fix probably could be split out into separate commits, but I
decided it would be easier to merge them rather than have to commit a trivial,
three-line change later.  

I've got some other small performance tweaks on the way, but those are big
enough to deserve their own code review.

[tim (not reviewing), 2013-05-19 20:56:44]

On 2013/05/17 22:37:54, rlarocque wrote:
> Please review.
> 
> The tests and the fix probably could be split out into separate commits, but I
> decided it would be easier to merge them rather than have to commit a trivial,
> three-line change later.  
> 
> I've got some other small performance tweaks on the way, but those are big
> enough to deserve their own code review.

This looks good, but can you explain what you mean by "comes with some easy to
tweak parameters  ..."?  Would it make sense to document in the test?

[rlarocque, 2013-05-20 17:20:57]

On 2013/05/19 20:56:44, timsteele wrote:
> On 2013/05/17 22:37:54, rlarocque wrote:
> > Please review.
> > 
> > The tests and the fix probably could be split out into separate commits, but
I
> > decided it would be easier to merge them rather than have to commit a
trivial,
> > three-line change later.  
> > 
> > I've got some other small performance tweaks on the way, but those are big
> > enough to deserve their own code review.
> 
> This looks good, but can you explain what you mean by "comes with some easy to
> tweak parameters  ..."?  Would it make sense to document in the test?

I've updated the commit message to make it clear I was referring to kNumFolders
and kNUmBookmarksPerFolder.  By increasing those numbers by a few orders of
magnitude, we can start to see the performance effects of changes that would
otherwise be too small to measure.  

I don't see much point in documenting this in the test.  The chances that it
will be found by someone looking to profile model association will probably be
the same with or without comment.

[tim (not reviewing), 2013-05-20 17:39:03]

On 2013/05/20 17:20:57, rlarocque wrote:
> On 2013/05/19 20:56:44, timsteele wrote:
> > On 2013/05/17 22:37:54, rlarocque wrote:
> > > Please review.
> > > 
> > > The tests and the fix probably could be split out into separate commits,
but
> I
> > > decided it would be easier to merge them rather than have to commit a
> trivial,
> > > three-line change later.  
> > > 
> > > I've got some other small performance tweaks on the way, but those are big
> > > enough to deserve their own code review.
> > 
> > This looks good, but can you explain what you mean by "comes with some easy
to
> > tweak parameters  ..."?  Would it make sense to document in the test?
> 
> I've updated the commit message to make it clear I was referring to
kNumFolders
> and kNUmBookmarksPerFolder.  By increasing those numbers by a few orders of
> magnitude, we can start to see the performance effects of changes that would
> otherwise be too small to measure.  
> 
> I don't see much point in documenting this in the test.  The chances that it
> will be found by someone looking to profile model association will probably be
> the same with or without comment.

My point is that your commit message makes it sound like this new test is
useless except for the hidden fact that if you time it (? Not sure how you are
using it as a "benchmark") it's useful as a perf tool, but there is no mention
of this in the code.  Without reading your commit message, the test and setup
code you add here looks mostly redundant beside MergeWithEmptyBookmarkModel.

[rlarocque, 2013-05-20 18:22:48]

On 2013/05/20 17:39:03, timsteele wrote:
> On 2013/05/20 17:20:57, rlarocque wrote:
> > On 2013/05/19 20:56:44, timsteele wrote:
> > > On 2013/05/17 22:37:54, rlarocque wrote:
> > > > Please review.
> > > > 
> > > > The tests and the fix probably could be split out into separate commits,
> but
> > I
> > > > decided it would be easier to merge them rather than have to commit a
> > trivial,
> > > > three-line change later.  
> > > > 
> > > > I've got some other small performance tweaks on the way, but those are
big
> > > > enough to deserve their own code review.
> > > 
> > > This looks good, but can you explain what you mean by "comes with some
easy
> to
> > > tweak parameters  ..."?  Would it make sense to document in the test?
> > 
> > I've updated the commit message to make it clear I was referring to
> kNumFolders
> > and kNUmBookmarksPerFolder.  By increasing those numbers by a few orders of
> > magnitude, we can start to see the performance effects of changes that would
> > otherwise be too small to measure.  
> > 
> > I don't see much point in documenting this in the test.  The chances that it
> > will be found by someone looking to profile model association will probably
be
> > the same with or without comment.
> 
> My point is that your commit message makes it sound like this new test is
> useless except for the hidden fact that if you time it (? Not sure how you are
> using it as a "benchmark") it's useful as a perf tool, but there is no mention
> of this in the code.  Without reading your commit message, the test and setup
> code you add here looks mostly redundant beside MergeWithEmptyBookmarkModel.

Maybe the commit message is a bit too strongly worded.  It's not *entirely*
useless.  If I understand the other tests correctly, then it looks like we never
had any tests for the case where the sync model has data and the bookmark model
is empty.  It does make some sense to cover this case explicitly since this is
what the model association after configure scenario looks like.  I think that
the "empty" model associate test will end up hitting mostly the same code paths
anyway, making this test redundant, but this does make some sense as a black box
test.

That being said, I guess it doesn't hurt to add a comment about the alternative
use of this test.

[tim (not reviewing), 2013-05-20 18:26:20]

Oops, I had left behind an unpublished comment.

Have you seen ProfileSyncServiceBookmarkTestWithData.MergeWithEmptyBookmarkModel
?

https://codereview.chromium.org/15308003/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):

https://codereview.chromium.org/15308003/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:293: int64
AddFolderToShare(syncer::WriteTransaction* trans, std::string title) {
It seems this is actually hard-wired to add a folder directly to the
bookmark_bar.

Can't this be made to be a special case of FakeServerChange?  Why do we need
multiple ways of adding bookmarks in these tests?

[rlarocque, 2013-05-20 20:09:54]

https://codereview.chromium.org/15308003/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):

https://codereview.chromium.org/15308003/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:293: int64
AddFolderToShare(syncer::WriteTransaction* trans, std::string title) {
On 2013/05/20 18:26:20, timsteele wrote:
> It seems this is actually hard-wired to add a folder directly to the
> bookmark_bar.
> 
> Can't this be made to be a special case of FakeServerChange?  Why do we need
> multiple ways of adding bookmarks in these tests?  

It is hard-coded to add folders to the bookmark bar.  I don't see that as a
problem.  

It can't use the FakeServerChange infrastructure because FakeServerChanges are
interpreted by the ChangeProcessor.  The intent of the test I've added is to see
what happens when a pre-populated sync share is associated with an empty native
model.  This requires that the directory be populated before the change
processor exists.

[rlarocque, 2013-05-20 20:18:03]

On 2013/05/20 18:26:20, timsteele wrote:
> Oops, I had left behind an unpublished comment.
> 
> Have you seen
ProfileSyncServiceBookmarkTestWithData.MergeWithEmptyBookmarkModel
> ?
> 

MergeWithEmptyBookmarkModel is the most similar to the test that I've added. 
However, it only only associates 2-3 special nodes.  This makes it a poor
simulation of the initial sync with non-empty account scenario.  It's also hard
to measure the performance impact of changes when only three nodes are being
associated.

I could have modified the MergeWithEmptyBookmarkModel to fit my needs rather
than creating a separate test.  However, I think it makes some sense to keep
both.  MergeWithEmptyBookmarkModel is a fairly accurate simulation of the
first-time sync with empty account scenario.  

Currently, there's not a lot of difference in the code paths exercised by either
test.  They do simulate different user-visible scenarios, though, so one could
argue that they both have reason to exist.

[tim (not reviewing), 2013-05-20 20:41:56]

On 2013/05/20 20:18:03, rlarocque wrote:
> On 2013/05/20 18:26:20, timsteele wrote:
> > Oops, I had left behind an unpublished comment.
> > 
> > Have you seen
> ProfileSyncServiceBookmarkTestWithData.MergeWithEmptyBookmarkModel
> > ?
> > 
> 
> MergeWithEmptyBookmarkModel is the most similar to the test that I've added. 
> However, it only only associates 2-3 special nodes.  This makes it a poor
> simulation of the initial sync with non-empty account scenario.  It's also
hard
> to measure the performance impact of changes when only three nodes are being
> associated.
> 
> I could have modified the MergeWithEmptyBookmarkModel to fit my needs rather
> than creating a separate test.  However, I think it makes some sense to keep
> both.  MergeWithEmptyBookmarkModel is a fairly accurate simulation of the
> first-time sync with empty account scenario.  
> 
> Currently, there's not a lot of difference in the code paths exercised by
either
> test.  They do simulate different user-visible scenarios, though, so one could
> argue that they both have reason to exist.

So, I agree with everything you've said here, but all of this is useful to point
out in comments :)  E.g Why your Add* routines are different from
FakeServerChange, and why your test is different from
MergeWithEmptyBookmarkModel.  Provided you add some comments to address those
things, this patch LGTM!

[rlarocque, 2013-05-21 00:18:46]

On 2013/05/20 20:41:56, timsteele wrote:
> On 2013/05/20 20:18:03, rlarocque wrote:
> > On 2013/05/20 18:26:20, timsteele wrote:
> > > Oops, I had left behind an unpublished comment.
> > > 
> > > Have you seen
> > ProfileSyncServiceBookmarkTestWithData.MergeWithEmptyBookmarkModel
> > > ?
> > > 
> > 
> > MergeWithEmptyBookmarkModel is the most similar to the test that I've added.

> > However, it only only associates 2-3 special nodes.  This makes it a poor
> > simulation of the initial sync with non-empty account scenario.  It's also
> hard
> > to measure the performance impact of changes when only three nodes are being
> > associated.
> > 
> > I could have modified the MergeWithEmptyBookmarkModel to fit my needs rather
> > than creating a separate test.  However, I think it makes some sense to keep
> > both.  MergeWithEmptyBookmarkModel is a fairly accurate simulation of the
> > first-time sync with empty account scenario.  
> > 
> > Currently, there's not a lot of difference in the code paths exercised by
> either
> > test.  They do simulate different user-visible scenarios, though, so one
could
> > argue that they both have reason to exist.
> 
> So, I agree with everything you've said here, but all of this is useful to
point
> out in comments :)  E.g Why your Add* routines are different from
> FakeServerChange, and why your test is different from
> MergeWithEmptyBookmarkModel.  Provided you add some comments to address those
> things, this patch LGTM!

Comments updated.  I'll send this at the CQ soon.

[commit-bot: I haz the power, 2013-05-21 17:59:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/15308003/12001

[commit-bot: I haz the power, 2013-05-21 21:32:01]

Change committed as 201363