Performance tests for bookmark sync.

chrome/test/live_sync/performance_live_bookmarks_sync_test.cc

Index: chrome/test/live_sync/performance_live_bookmarks_sync_test.cc
diff --git a/chrome/test/live_sync/performance_live_bookmarks_sync_test.cc b/chrome/test/live_sync/performance_live_bookmarks_sync_test.cc
new file mode 100644
index 0000000000000000000000000000000000000000..884cdf456967fd98162135636672c2f26a787db2
--- /dev/null
+++ b/chrome/test/live_sync/performance_live_bookmarks_sync_test.cc
@@ -0,0 +1,214 @@
+// Copyright 2011 Google Inc. All Rights Reserved.
+// Author: braffert@google.com (Ben Rafferty)

[Raghu Simha, 2011/06/15 18:38:16]

Use the standard chromium header. See any other file in this directory for an
example.

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

Done.

+
+#include "base/time.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/profile_sync_service_harness.h"
+#include "chrome/test/live_sync/live_bookmarks_sync_test.h"
+
+static const int kNumBookmarks = 150;
+
+// TODO(braffert): Consider the range / resolution of these test points.

[Raghu Simha, 2011/06/15 18:38:16]

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?

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

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.

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

SGTM.

+static const int kNumBenchmarkPoints = 18;
+static const int kBenchmarkPoints[] = {1, 10, 20, 30, 40, 50, 75, 100, 125,
+                                       150, 175, 200, 225, 250, 300, 350, 400,
+                                       500};
+
+// TODO(braffert): Move this class into its own .h/.cc files.  Maybe.

[Raghu Simha, 2011/06/15 18:38:16]

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.

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

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:

+class PerformanceLiveBookmarksSyncTest
+    : public TwoClientLiveBookmarksSyncTest {
+ public:
+  PerformanceLiveBookmarksSyncTest() : urlNumber(0), urlTitleNumber(0) {}
+
+  void AddURLs(int profile, int nURLs) {
+    for (int i = 0; i < nURLs; ++i) {
+      ASSERT_TRUE(AddURL(
+          profile, 0, NextIndexedURLTitle(), NextIndexedURL()) != NULL);
+    }
+  }
+
+  void UpdateURLs(int profile) {
+    for (int i = 0; i < GetBookmarkBarNode(profile)->child_count(); ++i) {
+      ASSERT_TRUE(SetURL(profile, GetBookmarkBarNode(profile)->GetChild(i),
+                         NextIndexedURL()));
+    }
+  }
+
+  void RemoveURLs(int profile) {
+    while (GetBookmarkBarNode(profile)->child_count()) {
+      Remove(profile, GetBookmarkBarNode(profile), 0);
+    }
+  }
+
+  void Cleanup() {

[Raghu Simha, 2011/06/15 18:38:16]

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.

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

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

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

SGTM.

+    for (int i = 0; i < num_clients(); ++i) {
+      RemoveURLs(i);
+    }
+    ASSERT_TRUE(AwaitQuiescence());
+    ASSERT_EQ(0, GetBookmarkBarNode(0)->child_count());
+    ASSERT_TRUE(AllModelsMatch());
+  }
+
+  // TODO(braffert): Move timing methods somewhere that they can be used by
+  // the performance test class for all datatypes.

[Raghu Simha, 2011/06/15 18:38:16]

To be able to use these across datatypes, how about putting these in something
like sync_timing_helper.{h/cc}?

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

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.

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

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.

+  base::TimeDelta TimeSyncCycle(ProfileSyncServiceHarness* client) {
+    base::Time start = base::Time::Now();
+    EXPECT_TRUE(client->AwaitSyncCycleCompletion("Timing sync cycle."));
+    return base::Time::Now() - start;
+  }
+
+  base::TimeDelta TimeMutualSyncCycle(ProfileSyncServiceHarness* client,
+                                      ProfileSyncServiceHarness* partner) {
+    base::Time start = base::Time::Now();
+    EXPECT_TRUE(client->AwaitMutualSyncCycleCompletion(partner));
+    return base::Time::Now() - start;
+  }
+
+  base::TimeDelta TimeUntilQuiescence() {
+    base::Time start = base::Time::Now();
+    EXPECT_TRUE(AwaitQuiescence());
+    return base::Time::Now() - start;
+  }
+
+ private:
+  GURL NextIndexedURL() {
+    return GURL(IndexedURL(urlNumber++));
+  }
+
+  std::wstring NextIndexedURLTitle() {
+    return IndexedURLTitle(urlTitleNumber++);
+  }
+
+  int urlNumber;
+  int urlTitleNumber;
+  DISALLOW_COPY_AND_ASSIGN(PerformanceLiveBookmarksSyncTest);
+};
+
+// TODO(braffert): Possibly split each of these into separate up / down test
+// cases?
+IN_PROC_BROWSER_TEST_F(PerformanceLiveBookmarksSyncTest, Add) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  DisableVerifier();
+
+  DisableNetwork(GetProfile(1));
+  AddURLs(0, kNumBookmarks);
+  base::TimeDelta dt_up = TimeSyncCycle(GetClient(0));
+  ASSERT_EQ(kNumBookmarks, GetBookmarkBarNode(0)->child_count());
+  ASSERT_EQ(0, GetBookmarkBarNode(1)->child_count());
+
+  EnableNetwork(GetProfile(1));
+  base::TimeDelta dt_down = TimeUntilQuiescence();

[Raghu Simha, 2011/06/15 18:38:16]

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]

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.

+  ASSERT_EQ(kNumBookmarks, GetBookmarkBarNode(0)->child_count());
+  ASSERT_TRUE(AllModelsMatch());
+
+  // TODO(braffert): Compare timings against some target value.
+}
+
+IN_PROC_BROWSER_TEST_F(PerformanceLiveBookmarksSyncTest, Update) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  DisableVerifier();
+
+  AddURLs(0, kNumBookmarks);
+  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
+  ASSERT_TRUE(AllModelsMatch());
+
+  DisableNetwork(GetProfile(1));
+  UpdateURLs(0);
+  base::TimeDelta dt_up = TimeSyncCycle(GetClient(0));
+  ASSERT_EQ(kNumBookmarks, GetBookmarkBarNode(0)->child_count());
+  ASSERT_EQ(kNumBookmarks, GetBookmarkBarNode(1)->child_count());
+  ASSERT_FALSE(AllModelsMatch());
+
+  EnableNetwork(GetProfile(1));
+  base::TimeDelta dt_down = TimeUntilQuiescence();
+  ASSERT_EQ(kNumBookmarks, GetBookmarkBarNode(0)->child_count());
+  ASSERT_EQ(kNumBookmarks, GetBookmarkBarNode(1)->child_count());
+
+  // TODO(braffert): Compare timings against some target value.
+}
+
+IN_PROC_BROWSER_TEST_F(PerformanceLiveBookmarksSyncTest, Delete) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  DisableVerifier();
+
+  AddURLs(0, kNumBookmarks);
+  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
+  ASSERT_TRUE(AllModelsMatch());
+
+  DisableNetwork(GetProfile(1));
+  RemoveURLs(0);
+  base::TimeDelta dt_up = TimeSyncCycle(GetClient(0));
+  ASSERT_EQ(0, GetBookmarkBarNode(0)->child_count());
+  ASSERT_EQ(kNumBookmarks, GetBookmarkBarNode(1)->child_count());
+
+  EnableNetwork(GetProfile(1));
+  base::TimeDelta dt_down = TimeUntilQuiescence();
+  ASSERT_EQ(0, GetBookmarkBarNode(0)->child_count());
+  ASSERT_EQ(0, GetBookmarkBarNode(1)->child_count());
+
+  // TODO(braffert): Compare timings against some target value.
+}
+
+IN_PROC_BROWSER_TEST_F(PerformanceLiveBookmarksSyncTest, Benchmark) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  DisableVerifier();
+
+  for (int i = 0; i < kNumBenchmarkPoints; ++i) {
+    int numBookmarks = kBenchmarkPoints[i];
+
+    // Disable client 1.  Add bookmarks and time commit by client 0.
+    DisableNetwork(GetProfile(1));
+    AddURLs(0, numBookmarks);
+    base::TimeDelta dt_up = TimeSyncCycle(GetClient(0));
+    ASSERT_EQ(numBookmarks, GetBookmarkBarNode(0)->child_count());
+    ASSERT_EQ(0, GetBookmarkBarNode(1)->child_count());
+
+    // Enable client 1 and time update (new bookmarks).
+    EnableNetwork(GetProfile(1));
+    base::TimeDelta dt_down = TimeUntilQuiescence();
+    ASSERT_EQ(numBookmarks, GetBookmarkBarNode(0)->child_count());
+    ASSERT_TRUE(AllModelsMatch());
+
+    VLOG(0) << "Add: " << numBookmarks << " " <<
+            static_cast<double>(numBookmarks) / dt_up.InSecondsF() << " " <<
+            static_cast<double>(numBookmarks) / dt_down.InSecondsF();
+
+    // Disable client 1.  Modify bookmarks and time commit by client 0.
+    DisableNetwork(GetProfile(1));
+    UpdateURLs(0);
+    dt_up = TimeSyncCycle(GetClient(0));
+    ASSERT_EQ(numBookmarks, GetBookmarkBarNode(0)->child_count());
+    ASSERT_EQ(numBookmarks, GetBookmarkBarNode(1)->child_count());
+    ASSERT_FALSE(AllModelsMatch());
+
+    // Enable client 1 and time update (changed bookmarks).
+    EnableNetwork(GetProfile(1));
+    dt_down = TimeUntilQuiescence();
+    ASSERT_EQ(numBookmarks, GetBookmarkBarNode(0)->child_count());
+    ASSERT_TRUE(AllModelsMatch());
+
+    VLOG(0) << "Update: " << numBookmarks << " " <<
+            static_cast<double>(numBookmarks) / dt_up.InSecondsF() << " " <<
+            static_cast<double>(numBookmarks) / dt_down.InSecondsF();
+
+    // Disable client 1.  Delete bookmarks and time commit by client 0.
+    DisableNetwork(GetProfile(1));
+    RemoveURLs(0);
+    dt_up = TimeSyncCycle(GetClient(0));
+    ASSERT_EQ(0, GetBookmarkBarNode(0)->child_count());
+    ASSERT_EQ(numBookmarks, GetBookmarkBarNode(1)->child_count());
+
+    // Enable client 1 and time update (deleted bookmarks).
+    EnableNetwork(GetProfile(1));
+    dt_down = TimeUntilQuiescence();
+    ASSERT_EQ(0, GetBookmarkBarNode(0)->child_count());
+    ASSERT_EQ(0, GetBookmarkBarNode(1)->child_count());
+
+    VLOG(0) << "Delete: " << numBookmarks << " " <<
+            static_cast<double>(numBookmarks) / dt_up.InSecondsF() << " " <<
+            static_cast<double>(numBookmarks) / dt_down.InSecondsF();
+
+    Cleanup();
+  }
+}
+