Session/tab sync performance tests.

Session/tab sync performance tests.

This patch adds tab sync performance tests, similar to the tests for the existing datatypes in live_sync/performance.  Changes to LiveSessionsSyncTest were required - most significantly the addition of AwaitSessionPropagation, which waits until a vector of urls can all be found in the model associator.  This type of wait loop previously existed as part of OpenTab, and worked with a single url.  AwaitSessionPropagation supports multiple urls, and is called from OpenTab, OpenMultipleTabs, and SessionsSyncPerfTest::UpdateTabs.

BUG=none
TEST=sync_performance_tests, sync_integration_tests

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

[braffert, 2011-08-04 23:47:34]

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sessions_sync_test.cc (right):

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sessions_sync_test.cc:118: found = false;
Using a flag here to avoid calling ModelAssociatorHasTabWithUrl twice.

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/perfor...
File chrome/test/live_sync/performance/sessions_sync_perf_test.cc (right):

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance/sessions_sync_perf_test.cc:89: VLOG(1) <<
"GetSessionData returned false";
Intentionally not returning -1 here.  GetSessionData returns false if there is
no foreign session data, in which case we should continue with counting the tabs
in the local session.

[Nicolas Zea, 2011-08-05 00:40:02]

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sessions_sync_test.cc (right):

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sessions_sync_test.cc:125: found =
ModelAssociatorHasTabWithUrl(index, *it);
May as well put this before the timeout check.

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sessions_sync_test.h (right):

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sessions_sync_test.h:120: bool
AwaitSessionPropagation(int index, const std::vector<GURL>& urls);
prefer WaitForTabsToLoad

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/perfor...
File chrome/test/live_sync/performance/sessions_sync_perf_test.cc (right):

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance/sessions_sync_perf_test.cc:108: ASSERT_EQ(0,
GetTabCount(0));
Doesn't this wind up being 1 (due to the NTP you mentioned)?

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance/sessions_sync_perf_test.cc:137: // New tab
page opens after closing all tabs.
"New tab page remains on profile 0 after..."

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance/sessions_sync_perf_test.cc:143:
IN_PROC_BROWSER_TEST_F(SessionsSyncPerfTest, DISABLED_Benchmark) {
Why disabled?

[braffert, 2011-08-05 00:47:38]

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sessions_sync_test.cc (right):

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sessions_sync_test.cc:125: found =
ModelAssociatorHasTabWithUrl(index, *it);
On 2011/08/05 00:40:02, nzea wrote:
> May as well put this before the timeout check.

I'm not seeing why one way is better than the other, but done.

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sessions_sync_test.h (right):

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sessions_sync_test.h:120: bool
AwaitSessionPropagation(int index, const std::vector<GURL>& urls);
On 2011/08/05 00:40:02, nzea wrote:
> prefer WaitForTabsToLoad

Done.

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/perfor...
File chrome/test/live_sync/performance/sessions_sync_perf_test.cc (right):

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance/sessions_sync_perf_test.cc:108: ASSERT_EQ(0,
GetTabCount(0));
On 2011/08/05 00:40:02, nzea wrote:
> Doesn't this wind up being 1 (due to the NTP you mentioned)?

Yes, nice catch.  This method is only called from the disabled benchmark test,
which is not automated but used for generating graphs.

[braffert, 2011-08-05 17:43:55]

Missed a couple comments from your last review.  I reworded the comment you
pointed out, and I ended up removing the disabled test all together along with
Cleanup.

Also modified the wait loop in WaitForTabsToLoad to use a simple while loop. 
The do-while loop is not needed because the found flag is initially false.

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/perfor...
File chrome/test/live_sync/performance/sessions_sync_perf_test.cc (right):

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance/sessions_sync_perf_test.cc:137: // New tab
page opens after closing all tabs.
On 2011/08/05 00:40:02, nzea wrote:
> "New tab page remains on profile 0 after..."

Done.

http://codereview.chromium.org/7575026/diff/1001/chrome/test/live_sync/perfor...
chrome/test/live_sync/performance/sessions_sync_perf_test.cc:143:
IN_PROC_BROWSER_TEST_F(SessionsSyncPerfTest, DISABLED_Benchmark) {
On 2011/08/05 00:40:02, nzea wrote:
> Why disabled?

We have one of these tests for each datatype - they aren't intended to be
automated.  They can be run offline to generate a graph showing how the
measurement times scale when increasing the number of synced entries.

I'm removing this test for tabs, because CleanUp is causing problems.  I am
considering removing it for the other datatypes as well.

[Raghu Simha, 2011-08-05 21:26:07]

I have a few minor comments. LGTM pending green trybots after that.

http://codereview.chromium.org/7575026/diff/13001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_sessions_sync_test.cc (right):

http://codereview.chromium.org/7575026/diff/13001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sessions_sync_test.cc:102: VLOG(1) << "Opening tab: "
<< it->spec();
How about: "Opening tab: " << it->spec() << " using profile " << index << ".";

http://codereview.chromium.org/7575026/diff/13001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sessions_sync_test.cc:120: found =
ModelAssociatorHasTabWithUrl(index, *it);
Curious: What if found becomes true at the last possible instant? If the line
above this comment returns true, but we just went past the deadline, this
function will end up returning false when it could technically return true.

If you think this is an edge case that we probably don't care about, I'm fine
leaving this as it is.

http://codereview.chromium.org/7575026/diff/13001/chrome/test/live_sync/perfo...
File chrome/test/live_sync/performance/sessions_sync_perf_test.cc (right):

http://codereview.chromium.org/7575026/diff/13001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance/sessions_sync_perf_test.cc:37: int url_number;
s/url_number/url_number_/

[braffert, 2011-08-05 21:37:21]

http://codereview.chromium.org/7575026/diff/13001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_sessions_sync_test.cc (right):

http://codereview.chromium.org/7575026/diff/13001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sessions_sync_test.cc:102: VLOG(1) << "Opening tab: "
<< it->spec();
On 2011/08/05 21:26:07, rsimha wrote:
> How about: "Opening tab: " << it->spec() << " using profile " << index << ".";

Done.

http://codereview.chromium.org/7575026/diff/13001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sessions_sync_test.cc:120: found =
ModelAssociatorHasTabWithUrl(index, *it);
On 2011/08/05 21:26:07, rsimha wrote:
> Curious: What if found becomes true at the last possible instant? If the line
> above this comment returns true, but we just went past the deadline, this
> function will end up returning false when it could technically return true.
> 
> If you think this is an edge case that we probably don't care about, I'm fine
> leaving this as it is.

I don't think it's anything to be concerned with.  I originally had the timeout
check come before the call to ModelAssociatorHasTabWithUrl, which would slightly
change the meaning of the corner case.  

Nicolas, what do you think?

http://codereview.chromium.org/7575026/diff/13001/chrome/test/live_sync/perfo...
File chrome/test/live_sync/performance/sessions_sync_perf_test.cc (right):

http://codereview.chromium.org/7575026/diff/13001/chrome/test/live_sync/perfo...
chrome/test/live_sync/performance/sessions_sync_perf_test.cc:37: int url_number;
On 2011/08/05 21:26:07, rsimha wrote:
> s/url_number/url_number_/

Done.

[Nicolas Zea, 2011-08-05 21:40:41]

LGTM

http://codereview.chromium.org/7575026/diff/13001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_sessions_sync_test.cc (right):

http://codereview.chromium.org/7575026/diff/13001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sessions_sync_test.cc:120: found =
ModelAssociatorHasTabWithUrl(index, *it);
On 2011/08/05 21:37:21, braffert wrote:
> On 2011/08/05 21:26:07, rsimha wrote:
> > Curious: What if found becomes true at the last possible instant? If the
line
> > above this comment returns true, but we just went past the deadline, this
> > function will end up returning false when it could technically return true.
> > 
> > If you think this is an edge case that we probably don't care about, I'm
fine
> > leaving this as it is.
> 
> I don't think it's anything to be concerned with.  I originally had the
timeout
> check come before the call to ModelAssociatorHasTabWithUrl, which would
slightly
> change the meaning of the corner case.  
> 
> Nicolas, what do you think? 

There's nothing we can do about it turning true after the call and before the
timeout. The timeout is sufficiently large that this should never happen, and if
it does it's likely a bug anyways, so I think this is fine.