Sync performance tests: Cleanup TODOs and print in dashboard friendly format

Sync performance tests: Cleanup TODOs and print in dashboard friendly format

This patch adds a new PrintResult method in SyncTimingHelper that should print timings in a format appropriate for use with the chromium perf dashboard.  It is a much simpler replication of the methods found in chrome/test/ui/ui_perf_test.{h,cc}.

Also cleanup TODOs that are no longer needed or relevant.


BUG=90441
TEST=sync_performance_tests

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

[Raghu Simha, 2011-07-26 06:19:44]

Nice work, Ben. I have a few comments below. The comments I've made in
autofill_sync_perf_test.cc apply to the other datatypes too.

http://codereview.chromium.org/7464046/diff/1/chrome/test/live_sync/performan...
File chrome/test/live_sync/performance/autofill_sync_perf_test.cc (left):

http://codereview.chromium.org/7464046/diff/1/chrome/test/live_sync/performan...
chrome/test/live_sync/performance/autofill_sync_perf_test.cc:13: //
TODO(braffert): Consider the range / resolution of these test points.
Perhaps replace this with a TODO for moving kBenchmarkPoints and
kNumBenchmarkPoints into a performance test base class once it's possible to
create one?

http://codereview.chromium.org/7464046/diff/1/chrome/test/live_sync/performan...
chrome/test/live_sync/performance/autofill_sync_perf_test.cc:20: // class files
be named as opposed to the file containing the tests themselves?
Curious: You're deleting the TODO to move the class into its own .h/.cc file,
but the class hasn't actually moved. Do you still intend to do so? (I'm OK with
them being left as they are, instead of tripling the number of files)

http://codereview.chromium.org/7464046/diff/1/chrome/test/live_sync/performan...
chrome/test/live_sync/performance/autofill_sync_perf_test.cc:130: //
TODO(braffert): Compare timings against some target value.
Don't you still have to compare the timings? I don't see PrintResult doing so.

http://codereview.chromium.org/7464046/diff/1/chrome/test/live_sync/performan...
File chrome/test/live_sync/performance/sync_timing_helper.h (right):

http://codereview.chromium.org/7464046/diff/1/chrome/test/live_sync/performan...
chrome/test/live_sync/performance/sync_timing_helper.h:43: const
base::TimeDelta& dt);
nit: Style guide recommends aligning parameters, one in each line.

[braffert, 2011-07-26 17:46:59]

Raghu,

The tracking and comparison is not actually done by the tests themselves.  The
perf dashboard scans the test output for appropriately formatted results, and
does the rest automatically.

http://codereview.chromium.org/7464046/diff/1/chrome/test/live_sync/performan...
File chrome/test/live_sync/performance/autofill_sync_perf_test.cc (left):

http://codereview.chromium.org/7464046/diff/1/chrome/test/live_sync/performan...
chrome/test/live_sync/performance/autofill_sync_perf_test.cc:13: //
TODO(braffert): Consider the range / resolution of these test points.
On 2011/07/26 06:19:45, rsimha wrote:
> Perhaps replace this with a TODO for moving kBenchmarkPoints and
> kNumBenchmarkPoints into a performance test base class once it's possible to
> create one?

Done.

http://codereview.chromium.org/7464046/diff/1/chrome/test/live_sync/performan...
chrome/test/live_sync/performance/autofill_sync_perf_test.cc:20: // class files
be named as opposed to the file containing the tests themselves?
On 2011/07/26 06:19:45, rsimha wrote:
> Curious: You're deleting the TODO to move the class into its own .h/.cc file,
> but the class hasn't actually moved. Do you still intend to do so? (I'm OK
with
> them being left as they are, instead of tripling the number of files)

That's what I was thinking as well - currently there would be no benefit from
separating each datatype into three files.  Though it may be a good idea to move
all the datatype specific convenience methods into a single performance helper
class, but this is best left for a later time.

http://codereview.chromium.org/7464046/diff/1/chrome/test/live_sync/performan...
chrome/test/live_sync/performance/autofill_sync_perf_test.cc:130: //
TODO(braffert): Compare timings against some target value.
On 2011/07/26 06:19:45, rsimha wrote:
> Don't you still have to compare the timings? I don't see PrintResult doing so.

The tracking and comparison is done automatically by the perf dashboard.  This
just prints the timings in a format that matches the dashboard's expectations.

http://codereview.chromium.org/7464046/diff/1/chrome/test/live_sync/performan...
File chrome/test/live_sync/performance/sync_timing_helper.h (right):

http://codereview.chromium.org/7464046/diff/1/chrome/test/live_sync/performan...
chrome/test/live_sync/performance/sync_timing_helper.h:43: const
base::TimeDelta& dt);
On 2011/07/26 06:19:45, rsimha wrote:
> nit: Style guide recommends aligning parameters, one in each line.

Done.  I added static to the first line and forgot to adjust the rest.

[anna, 2011-07-26 22:24:31]

With the above changes, LGTM.