Performance tests for bookmark sync.

Performance tests for bookmark sync.

For now, these tests should always pass.  Eventually, the results of the
benchmark runs should be stored somewhere, and the timings from the other
three test cases should be compared against a target value.

PerformanceLiveBookmarksSyncTest.Add
PerformanceLiveBookmarksSyncTest.Update
PerformanceLiveBookmarksSyncTest.Delete
PerformanceLiveBookmarksSyncTest.Benchmark


BUG=
TEST=


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

[Raghu Simha, 2011-06-15 18:38:15]

This is really cool work. I have a few comments below.

http://codereview.chromium.org/7171003/diff/5001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7171003/diff/5001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:3379:
'test/live_sync/performance_live_bookmarks_sync_test.cc',
Entries in this file are alphabetized. Move this to just below
multiple_client_live_typed_urls_sync_test.cc.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/live_b...
File chrome/test/live_sync/live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/live_b...
chrome/test/live_sync/live_bookmarks_sync_test.cc:269: return
StringPrintf("http://www.host.ext:1234/path/filename/%d", i);
indent -= 2.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/live_b...
chrome/test/live_sync/live_bookmarks_sync_test.cc:273: return StringPrintf(L"URL
Title %d", i);
indent -= 2.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/live_b...
chrome/test/live_sync/live_bookmarks_sync_test.cc:277: return
StringPrintf(L"Folder Name %d", i);
indent -= 2.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/live_b...
chrome/test/live_sync/live_bookmarks_sync_test.cc:281: return
StringPrintf(L"Subfolder Name %d", i);
indent -= 2.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/live_b...
chrome/test/live_sync/live_bookmarks_sync_test.cc:285: return
StringPrintf(L"Subsubfolder Name %d", i);
indent -= 2.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/live_b...
chrome/test/live_sync/live_bookmarks_sync_test.cc:287: 
Delete extra blank line.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
File chrome/test/live_sync/performance_live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:2: // Author:
braffert@google.com (Ben Rafferty)
Use the standard chromium header. See any other file in this directory for an
example.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:11: //
TODO(braffert): Consider the range / resolution of these test points.
How long does it take to run through all points on a slow machine? (the mac bots
are pretty slow) Would it make sense to distribute them more sparsely?

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:17: //
TODO(braffert): Move this class into its own .h/.cc files.  Maybe.
s/Maybe/Yes/ :)

It's much cleaner, and will allow for some of these methods to be used in
performance tests for other data types.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:43: void Cleanup()
{
I'm wondering if this method is really necessary, since you only seem to be
calling it at the end of a test. All profiles are torn down after each test, so
this seems moot.

OTOH, if removing URLs is something you explicitly want to test, this code is
better off being a part of the test.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:53: // the
performance test class for all datatypes.
To be able to use these across datatypes, how about putting these in something
like sync_timing_helper.{h/cc}?

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:100:
base::TimeDelta dt_down = TimeUntilQuiescence();
TimeUntilQuiescence(1) will include any sync cycles that Client 0 might go
through when Client 1 comes back online. Do you think it makes sense to
DisableNetwork(0), EnableNetwork(1), and then just TimeSyncCycle(1)?

[braffert, 2011-06-16 00:59:24]

Thanks for the feedback.  Everything you mentioned should be fixed, for the most
part.

I added a new LiveSyncTimingHelper class so that the timing methods can
eventually be generic across all datatypes.  The SetUp of the performance test
class is overridden to pair itself with the timing helper.  I'm not sure if this
is the best way.

I also rearranged the declarations and definitions in performance_live_sync_test
so that things can be easily split into separate files.  I couldn't think of a
good name for the new files though...

http://codereview.chromium.org/7171003/diff/5001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7171003/diff/5001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:3379:
'test/live_sync/performance_live_bookmarks_sync_test.cc',
On 2011/06/15 18:38:16, rsimha wrote:
> Entries in this file are alphabetized. Move this to just below
> multiple_client_live_typed_urls_sync_test.cc.

Done.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/live_b...
File chrome/test/live_sync/live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/live_b...
chrome/test/live_sync/live_bookmarks_sync_test.cc:269: return
StringPrintf("http://www.host.ext:1234/path/filename/%d", i);
On 2011/06/15 18:38:16, rsimha wrote:
> indent -= 2.

All 5 fixed.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/live_b...
chrome/test/live_sync/live_bookmarks_sync_test.cc:287: 
On 2011/06/15 18:38:16, rsimha wrote:
> Delete extra blank line.

Done.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
File chrome/test/live_sync/performance_live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:2: // Author:
braffert@google.com (Ben Rafferty)
On 2011/06/15 18:38:16, rsimha wrote:
> Use the standard chromium header. See any other file in this directory for an
> example.

Done.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:11: //
TODO(braffert): Consider the range / resolution of these test points.
On 2011/06/15 18:38:16, rsimha wrote:
> How long does it take to run through all points on a slow machine? (the mac
bots
> are pretty slow) Would it make sense to distribute them more sparsely?

I can't say for sure - It depends on whether the Python test server or a local
live server is used, and whether or not the server is restarted between
iterations.  I think this works well enough for now, but it should be
reconsidered once we figure out how the benchmarking will be automated.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:17: //
TODO(braffert): Move this class into its own .h/.cc files.  Maybe.
I've split up the declaration and definition here so that it should be simple to
move into new files, but I'm not what to name them (as opposed to the file
containing the tests).

On 2011/06/15 18:38:16, rsimha wrote:
> s/Maybe/Yes/ :)
> 
> It's much cleaner, and will allow for some of these methods to be used in
> performance tests for other data types.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:43: void Cleanup()
{
On 2011/06/15 18:38:16, rsimha wrote:
> I'm wondering if this method is really necessary, since you only seem to be
> calling it at the end of a test. All profiles are torn down after each test,
so
> this seems moot.
> 
> OTOH, if removing URLs is something you explicitly want to test, this code is
> better off being a part of the test.

Cleanup() is called between benchmark iterations to empty both clients before
continuing.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:53: // the
performance test class for all datatypes.
On 2011/06/15 18:38:16, rsimha wrote:
> To be able to use these across datatypes, how about putting these in something
> like sync_timing_helper.{h/cc}?

Done.  This required overriding the SetUp method of the benchmark performance
test class to connect itself to the timer helper.  Now the timing methods are
not specific to the benchmark performance class, however this step will be
needed in every location before the timer helper can be used.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:100:
base::TimeDelta dt_down = TimeUntilQuiescence();
On 2011/06/15 18:38:16, rsimha wrote:
> TimeUntilQuiescence(1) will include any sync cycles that Client 0 might go
> through when Client 1 comes back online. Do you think it makes sense to
> DisableNetwork(0), EnableNetwork(1), and then just TimeSyncCycle(1)?

Calling TimeSyncCycle(1) instead of AwaitQuiescence() caused the next
AllModelsMatch() check to fail.  Using a mutual sync cycle works, although I'm
not sure if that will change anything in this case.

[Raghu Simha, 2011-06-16 22:42:54]

Nice work again -- this stuff is shaping up really well. Sorry about the
multiple iterations, but the first patch is always the hardest :)

I have a bunch of comments below, many of which have to do with the points you
have already identified via TODOs.

I also owe you an answer on where the performance tests should go, since adding
them to the sync_integration_tests target will greatly increase their turnaround
time and this isn't something we want to do.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
File chrome/test/live_sync/performance_live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:11: //
TODO(braffert): Consider the range / resolution of these test points.
On 2011/06/16 00:59:24, braffert wrote:
> On 2011/06/15 18:38:16, rsimha wrote:
> > How long does it take to run through all points on a slow machine? (the mac
> bots
> > are pretty slow) Would it make sense to distribute them more sparsely?
> 
> I can't say for sure - It depends on whether the Python test server or a local
> live server is used, and whether or not the server is restarted between
> iterations.  I think this works well enough for now, but it should be
> reconsidered once we figure out how the benchmarking will be automated.

SGTM.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:43: void Cleanup()
{
On 2011/06/16 00:59:24, braffert wrote:
> On 2011/06/15 18:38:16, rsimha wrote:
> > I'm wondering if this method is really necessary, since you only seem to be
> > calling it at the end of a test. All profiles are torn down after each test,
> so
> > this seems moot.
> > 
> > OTOH, if removing URLs is something you explicitly want to test, this code
is
> > better off being a part of the test.
> 
> Cleanup() is called between benchmark iterations to empty both clients before
> continuing.
> 

SGTM.

http://codereview.chromium.org/7171003/diff/5001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:53: // the
performance test class for all datatypes.
On 2011/06/16 00:59:24, braffert wrote:
> On 2011/06/15 18:38:16, rsimha wrote:
> > To be able to use these across datatypes, how about putting these in
something
> > like sync_timing_helper.{h/cc}?
> 
> Done.  This required overriding the SetUp method of the benchmark performance
> test class to connect itself to the timer helper.  Now the timing methods are
> not specific to the benchmark performance class, however this step will be
> needed in every location before the timer helper can be used.
> 

In other comments, I've suggested some changes to the helper class that might
help you completely detach it from the performance test classes for each
datatype and not have to override SetUp each time. Let me know if they make
sense.

http://codereview.chromium.org/7171003/diff/10001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7171003/diff/10001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:3386:
'test/live_sync/performance_live_bookmarks_sync_test.cc',
I just realised that adding these tests to the sync_integration_tests target
will cause them to be run on the buildbots and trybots for every patch. I don't
think we want to do this. Perhaps we should create a new target called
sync_performance_tests. Let me get back to you on where they should live.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_sync_timing_helper.h (right):

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_timing_helper.h:12: class TimeDelta;
indent -= 2.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_timing_helper.h:24: void Setup(LiveSyncTest*
test);
With the change I've suggested to TimeUntilQuiescence, we won't need this
method.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_timing_helper.h:29: // Returns the time taken
for both |client| and partner to complete a sync
nit: partner -> |partner|.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_timing_helper.h:36: base::TimeDelta
TimeUntilQuiescence();
I suggest making this method take a std::vector<ProfileSyncServiceHarness*>&
called |clients|, and then time the call to
ProfileSyncServiceHarness::AwaitQuiescence(clients) instead of
LiveSyncTest::AwaitQuiescence().

You can then make all the methods in this class static and the performance tests
can use this class as a helper class without having to each maintain a member
variable of type LiveSyncTimingHelper.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_timing_helper.h:39: LiveSyncTest* test_;
With the change I've suggested to TimeUntilQuiescence, we won't need this
member.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
File chrome/test/live_sync/performance_live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:10: */
Remove this block if it's unnecessary.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:25: // class files
be named as opposed to the file containing the tests themselves?
How about naming the class LiveBookmarksSyncPerformanceTest and moving it to
live_bookmarks_sync_performance_test.{h/cc}? (I realise all the class names in
this directory are wordy, but consider it an artifact of chrome history :) )

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:32: void SetUp()
OVERRIDE;
With the change I've suggested to LiveSyncTimingHelper::TimeUntilQuiescence, we
won't need this method.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:35: void
AddURLs(int profile, int nURLs);
Style guide recommends num_urls over nURLs.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:47:
LiveSyncTimingHelper timing_helper;
With the change I've suggested to LiveSyncTimingHelper::TimeUntilQuiescence, we
won't need this member.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:51: GURL
NextIndexedURL();
IndexedURL returns a wstring while NextIndexedURL returns a GURL. Better to be
consistent and make them return the same type.

Easy fix: Make NextIndexedURL return a wstring and do a find-replace in the
performance tests.
Maybe painful fix: Make IndexedURL return a GURL and do a find-replace in all
the bookmarks tests.

Your choice :)

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:56: int urlNumber;
Style guide recommends that variable names are lower_case_with_underscore and
not mixedCase. (Here and everywhere else in this file)

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:176: int
numBookmarks = kBenchmarkPoints[i];
numBookmarks -> num_bookmarks.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/two_c...
File chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/two_c...
chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc:18: }
This method could move to LiveBookmarksSyncTest too, right?

[braffert, 2011-06-17 18:19:46]

I'm still not quite sure what to do about the test class filename issue.  The
only reason I'm hesitant to do what you suggested is that we would end up with
live_bookmarks_sync_performance_test.cc and
performance_live_bookmarks_sync_test.cc in the same directory.

http://codereview.chromium.org/7171003/diff/10001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7171003/diff/10001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:3386:
'test/live_sync/performance_live_bookmarks_sync_test.cc',
On 2011/06/16 22:42:55, rsimha wrote:
> I just realised that adding these tests to the sync_integration_tests target
> will cause them to be run on the buildbots and trybots for every patch. I
don't
> think we want to do this. Perhaps we should create a new target called
> sync_performance_tests. Let me get back to you on where they should live.

I discussed this with Anna a while ago, and we decided that the goal would be to
leave the add/update/delete cases for each datatype as part of the integration
tests.  The time taken for these test cases to finish is slightly longer, but
still comparable to the rest of the integration tests.  Only the benchmark takes
a significant amount of time, so it should eventually be moved somewhere else.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_sync_timing_helper.h (right):

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_timing_helper.h:12: class TimeDelta;
On 2011/06/16 22:42:55, rsimha wrote:
> indent -= 2.

Done.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_timing_helper.h:24: void Setup(LiveSyncTest*
test);
On 2011/06/16 22:42:55, rsimha wrote:
> With the change I've suggested to TimeUntilQuiescence, we won't need this
> method.

Done.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_timing_helper.h:29: // Returns the time taken
for both |client| and partner to complete a sync
On 2011/06/16 22:42:55, rsimha wrote:
> nit: partner -> |partner|.

Done.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_timing_helper.h:36: base::TimeDelta
TimeUntilQuiescence();
On 2011/06/16 22:42:55, rsimha wrote:
> I suggest making this method take a std::vector<ProfileSyncServiceHarness*>&
> called |clients|, and then time the call to
> ProfileSyncServiceHarness::AwaitQuiescence(clients) instead of
> LiveSyncTest::AwaitQuiescence().
> 
> You can then make all the methods in this class static and the performance
tests
> can use this class as a helper class without having to each maintain a member
> variable of type LiveSyncTimingHelper.

Done.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_timing_helper.h:39: LiveSyncTest* test_;
On 2011/06/16 22:42:55, rsimha wrote:
> With the change I've suggested to TimeUntilQuiescence, we won't need this
> member.

Done.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
File chrome/test/live_sync/performance_live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:10: */
On 2011/06/16 22:42:55, rsimha wrote:
> Remove this block if it's unnecessary.

Done.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:32: void SetUp()
OVERRIDE;
On 2011/06/16 22:42:55, rsimha wrote:
> With the change I've suggested to LiveSyncTimingHelper::TimeUntilQuiescence,
we
> won't need this method.

Done.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:35: void
AddURLs(int profile, int nURLs);
On 2011/06/16 22:42:55, rsimha wrote:
> Style guide recommends num_urls over nURLs.

Done.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:47:
LiveSyncTimingHelper timing_helper;
On 2011/06/16 22:42:55, rsimha wrote:
> With the change I've suggested to LiveSyncTimingHelper::TimeUntilQuiescence,
we
> won't need this member.

Done.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:51: GURL
NextIndexedURL();
On 2011/06/16 22:42:55, rsimha wrote:
> IndexedURL returns a wstring while NextIndexedURL returns a GURL. Better to be
> consistent and make them return the same type.
> 
> Easy fix: Make NextIndexedURL return a wstring and do a find-replace in the
> performance tests.
> Maybe painful fix: Make IndexedURL return a GURL and do a find-replace in all
> the bookmarks tests.
> 
> Your choice :)

Taking the easy option here - just to keep this CL more focused.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:56: int urlNumber;
On 2011/06/16 22:42:55, rsimha wrote:
> Style guide recommends that variable names are lower_case_with_underscore and
> not mixedCase. (Here and everywhere else in this file)

Done.

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:176: int
numBookmarks = kBenchmarkPoints[i];
On 2011/06/16 22:42:55, rsimha wrote:
> numBookmarks -> num_bookmarks.

Done.  Curious - why do constants use a different naming convention?  Just so
that they are identifiable?

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/two_c...
File chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/7171003/diff/10001/chrome/test/live_sync/two_c...
chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc:18: }
On 2011/06/16 22:42:55, rsimha wrote:
> This method could move to LiveBookmarksSyncTest too, right?

It could, but since this function simply generates a hard-coded favicon specific
to only these tests, I though it belonged here alongside kGenericURL etc.

[commit-bot: I haz the power, 2011-06-20 17:03:17]

No LGTM from valid reviewers yet.

[Raghu Simha, 2011-06-20 17:42:54]

LGTM.

There's still the issue of moving the benchmark test into a new test target, but
that can be done in another patch.

This is some really good work :)

http://codereview.chromium.org/7171003/diff/13001/chrome/test/live_sync/perfo...
File chrome/test/live_sync/performance_live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/7171003/diff/13001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:154:
IN_PROC_BROWSER_TEST_F(PerformanceLiveBookmarksSyncTest, Benchmark) {
This will greatly increase the amount of time taken by the
sync_integration_tests target. I suggest disabling it until a
sync_performance_tests target is created.

http://codereview.chromium.org/7171003/diff/13001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:175: VLOG(0) <<
std::endl << "Add: " << num_bookmarks << " " <<
nit: The "<<" at the end goes on the next line, aligned with the "<<" above it.
(Here and elsewhere in this file)

[braffert, 2011-06-20 19:34:01]

http://codereview.chromium.org/7171003/diff/13001/chrome/test/live_sync/perfo...
File chrome/test/live_sync/performance_live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/7171003/diff/13001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:154:
IN_PROC_BROWSER_TEST_F(PerformanceLiveBookmarksSyncTest, Benchmark) {
On 2011/06/20 17:42:54, rsimha wrote:
> This will greatly increase the amount of time taken by the
> sync_integration_tests target. I suggest disabling it until a
> sync_performance_tests target is created.

Done.

http://codereview.chromium.org/7171003/diff/13001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:175: VLOG(0) <<
std::endl << "Add: " << num_bookmarks << " " <<
On 2011/06/20 17:42:54, rsimha wrote:
> nit: The "<<" at the end goes on the next line, aligned with the "<<" above
it.
> (Here and elsewhere in this file)

Done.

[anna, 2011-06-21 19:46:58]

Good work, Ben!

I am thinking that to complete the bookmarks data type perf, we need to also
consider folders (complex hierarchies). I need to get these tests in TCM first
and as they are a more general bookmarks cases, they should not be blocking this
check-in.

Thanks,
-Anna

http://codereview.chromium.org/7171003/diff/17002/chrome/test/live_sync/perfo...
File chrome/test/live_sync/performance_live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/7171003/diff/17002/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:88:
IN_PROC_BROWSER_TEST_F(PerformanceLiveBookmarksSyncTest, Add) {
TCM ID - 7556828

http://codereview.chromium.org/7171003/diff/17002/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:107:
IN_PROC_BROWSER_TEST_F(PerformanceLiveBookmarksSyncTest, Update) {
TCM ID - 7564762

http://codereview.chromium.org/7171003/diff/17002/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:131:
IN_PROC_BROWSER_TEST_F(PerformanceLiveBookmarksSyncTest, Delete) {
TCM ID - 7566626

http://codereview.chromium.org/7171003/diff/17002/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance_live_bookmarks_sync_test.cc:161: // Disable
client 1.  Add bookmarks and time commit by client 0.
To ensure that we account for possible perf issues with non-flat bookmarks'
hierarchies, you should consider adding Add / Delete / Update (Reorder) for
such.

[commit-bot: I haz the power, 2011-06-21 21:07:53]

Change committed as 89893