Setting up benchmarks and perf utilities

This change introduces:

 * Benchmark and AsyncBenchmark base classes: the former is practically BenchmarkBase from benchmark_harness with some modifications. Eventually we should unify the two.

 * perf utilities: a perf test runner that runs our tests in DRT (dart + js) and writes results in a json file, an update utility script to update results, and a comparison script that shows the difference between two sets of results.

 * 25 benchmarks

Committed: https://github.com/dart-lang/web-ui/commit/98ef8ec

[Siggi Cherem (dart-lang), 2013-01-03 23:33:49]

FYI - I updated the CL to use the BenchmarkBase from the benchmark_harness pub
package.

[Jennifer Messerly, 2013-01-07 21:07:46]

This is so cool. Real benchmarks. Love it!

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/compare....
File test/perf/compare.dart (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/compare....
test/perf/compare.dart:9: */
library test.perf.compare; ?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/compare....
test/perf/compare.dart:57: if (a.name.startsWith('dart') &&
b.name.startsWith('js  ')) return 1;
since ResultPair is its own class, perhaps make it implemented Comparable and
move this logic into the compareTo method?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/compare....
test/perf/compare.dart:99: class _ResultPair {
general note: I don't think most of these classes really needed to be private,
since this is an entry point it's not likely to be imported. (Actually since it
doesn't have a library tag, it can't be imported)

Not a big deal either way though.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/compare....
test/perf/compare.dart:135: _ensure(StringBuffer buff, int ensure) {
I wonder if something like _padSpaces or _ensureColumn would be a better name?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/compare....
test/perf/compare.dart:146: str = (value > 100) ? value.toStringAsFixed(0) :
value.toStringAsFixed(2);
If value is between 10 and 100 should it use toStringAsFixed(1)?

Should it be value >= 100, since 100 is already 3 digits?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/as...
File test/perf/input/async_benchmark.dart (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/as...
test/perf/input/async_benchmark.dart:29: if (count-- <= 0) return new
Future.immediate(val);
FYI: "val" should already be the completed Future here, if using .chain, so you
probably just want to return it?

...or you could instead use .transform, which would pass the value instead of
the Future?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/as...
test/perf/input/async_benchmark.dart:45: static Future<double>
measureFor(Function f, int timeMinimum) {
would be nice to make this Function type annotation more precise either using an
inline annotation or a typedef

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/em...
File test/perf/input/empty_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/em...
test/perf/input/empty_test.html:10: This performance test does nothing. The
score can be thought as a baseline.
I hope the result is pretty close to 0?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/lo...
File test/perf/input/loop_100_10_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/lo...
test/perf/input/loop_100_10_test.html:10: This performance test mutates a list
of 100 elements used in 100 loops.
the DEFAULT_LIST does not seem to be 100 elements?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/lo...
File test/perf/input/loop_1_100_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/lo...
test/perf/input/loop_1_100_test.html:10: This performance test mutates a list of
100 elements used in a single loop.
what's perf like for N=1000?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/lo...
File test/perf/input/loop_1_2_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/lo...
test/perf/input/loop_1_2_test.html:10: This performance test updates a list of 2
elements used in a single loop.
this seems really small, I guess it's intended to measure startup overhead, if
any?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
File test/perf/input/todomvc2_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
test/perf/input/todomvc2_test.html:13: * add 10 todos
what happens if we crank up the number of todos?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
test/perf/input/todomvc2_test.html:27: <section id="todoapp">
If it doesn't affect performance, could you pull the entire body into a
component so it can be reused?

I'm thinking we should do the same thing for our example code and other tests
... it seems like having a nearly empty main page is best for testability, so it
might be worth recommending that application structure for now.

Of course, either part or both could be done in a different CL :)

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
File test/perf/input/todomvc3_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
test/perf/input/todomvc3_test.html:90: class TodoMvcBenchmark extends
BenchmarkBase {
Minor comment about benchmark library:
generally we don't use "Base" as a suffix for a base class.
any idea where I can file that bit of feedback?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
File test/perf/input/todomvc_async_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
test/perf/input/todomvc_async_test.html:100: .chain((_) => step(() =>
addNote("one")))
It would be nice to combine step and chain somehow, to make this cleaner

perhaps:

return asyncSteps([
    app.todos.clear,
    () => addNote("one"),
    () => addNote("two")])

where "asyncSteps" is defined as:

Future asyncSteps(List<Function> steps) {
  Future result = step(steps[0]);
  for (int i = 1; i < steps.length; i++) {
    result = result.chain((_) => step(steps[i]));
  }
  return result;
}


There might even be a better way to implement it by inlining "step". Perhaps:

Future asyncSteps(List<Function> steps) {
  var completer = new Completer();
  window.requestLayoutFrame(() {
    var result = null;
    for (var step in steps) {
      result = step();
      dispatch();
    }
    completer.complete(result);
  });
  return completer.future;
}

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/run.dart
File test/perf/run.dart (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/run.dart...
test/perf/run.dart:6: * This is a helper for run.sh. We try to run all of the
Dart code in one
helper for perf.sh?
should this file be called "perf.dart"?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/run.dart...
test/perf/run.dart:70: expect(Process.run('sed', [
How bad would it be to rewrite this in Dart instead of using "sed"? If it isn't
easy, we should make sure appropriate bug has been filed and add TODO here.

I think with File.readAsStringSync, File.writeAsStringSync, and
String.replaceAll, it shouldn't be too bad? We might be missing support for
using the matched groups in the replace expression, though (e.g. "\1"). I'm not
sure.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/update.dart
File test/perf/update.dart (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/update.d...
test/perf/update.dart:1: #!/usr/bin/env dart
rename this script to update_json or copy_json?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/update.d...
test/perf/update.dart:7: * A simple script that updates a target json file with
the new values from
I might've missed where this is called from?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/update.d...
test/perf/update.dart:9: */
library tag, so this comment has something to be attached to?

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/update.d...
test/perf/update.dart:18: print('usage: update.dart from.json to.json');
Would be nice to print the nice library comment here.

[Siggi Cherem (dart-lang), 2013-01-08 02:26:18]

ptal

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/compare....
File test/perf/compare.dart (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/compare....
test/perf/compare.dart:9: */
On 2013/01/07 21:07:46, John Messerly wrote:
> library test.perf.compare; ?

Done.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/compare....
test/perf/compare.dart:57: if (a.name.startsWith('dart') &&
b.name.startsWith('js  ')) return 1;
On 2013/01/07 21:07:46, John Messerly wrote:
> since ResultPair is its own class, perhaps make it implemented Comparable and
> move this logic into the compareTo method?

Done.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/compare....
test/perf/compare.dart:99: class _ResultPair {
On 2013/01/07 21:07:46, John Messerly wrote:
> general note: I don't think most of these classes really needed to be private,
> since this is an entry point it's not likely to be imported. (Actually since
it
> doesn't have a library tag, it can't be imported)
> 
> Not a big deal either way though.

Done.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/compare....
test/perf/compare.dart:135: _ensure(StringBuffer buff, int ensure) {
On 2013/01/07 21:07:46, John Messerly wrote:
> I wonder if something like _padSpaces or _ensureColumn would be a better name?

Done.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/compare....
test/perf/compare.dart:146: str = (value > 100) ? value.toStringAsFixed(0) :
value.toStringAsFixed(2);
On 2013/01/07 21:07:46, John Messerly wrote:
> If value is between 10 and 100 should it use toStringAsFixed(1)?
> 
> Should it be value >= 100, since 100 is already 3 digits?

Done.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/as...
File test/perf/input/async_benchmark.dart (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/as...
test/perf/input/async_benchmark.dart:29: if (count-- <= 0) return new
Future.immediate(val);
On 2013/01/07 21:07:46, John Messerly wrote:
> FYI: "val" should already be the completed Future here, if using .chain, so
you
> probably just want to return it?
> 
> ...or you could instead use .transform, which would pass the value instead of
> the Future?

it's subtle, but both chain and transform pass the actual value as the argument,
they differ in what they return. I need to use chain because run returns a
Future.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/as...
test/perf/input/async_benchmark.dart:45: static Future<double>
measureFor(Function f, int timeMinimum) {
On 2013/01/07 21:07:46, John Messerly wrote:
> would be nice to make this Function type annotation more precise either using
an
> inline annotation or a typedef

Done.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/em...
File test/perf/input/empty_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/em...
test/perf/input/empty_test.html:10: This performance test does nothing. The
score can be thought as a baseline.
On 2013/01/07 21:07:46, John Messerly wrote:
> I hope the result is pretty close to 0?

yeah - for some reason it's 0.11 usec in dart, and 0.36 usec in js.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/lo...
File test/perf/input/loop_100_10_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/lo...
test/perf/input/loop_100_10_test.html:10: This performance test mutates a list
of 100 elements used in 100 loops.
On 2013/01/07 21:07:46, John Messerly wrote:
> the DEFAULT_LIST does not seem to be 100 elements?

thanks - this comment was wrong, updated :)

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/lo...
File test/perf/input/loop_1_100_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/lo...
test/perf/input/loop_1_100_test.html:10: This performance test mutates a list of
100 elements used in a single loop.
On 2013/01/07 21:07:46, John Messerly wrote:
> what's perf like for N=1000?

It's about 10x slower.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/lo...
File test/perf/input/loop_1_2_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/lo...
test/perf/input/loop_1_2_test.html:10: This performance test updates a list of 2
elements used in a single loop.
On 2013/01/07 21:07:46, John Messerly wrote:
> this seems really small, I guess it's intended to measure startup overhead, if
> any?

yeah - each test is run repeatedly, so this is measuring mostly the cost of
setting up and tearing down the loop.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
File test/perf/input/todomvc2_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
test/perf/input/todomvc2_test.html:13: * add 10 todos
On 2013/01/07 21:07:46, John Messerly wrote:
> what happens if we crank up the number of todos?

it gets really slow, just running 10 iterations is more than a minute (the
default DumpRenderTree timeout)

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
test/perf/input/todomvc2_test.html:27: <section id="todoapp">
On 2013/01/07 21:07:46, John Messerly wrote:
> If it doesn't affect performance, could you pull the entire body into a
> component so it can be reused?
> 
> I'm thinking we should do the same thing for our example code and other tests
> ... it seems like having a nearly empty main page is best for testability, so
it
> might be worth recommending that application structure for now.
> 
> Of course, either part or both could be done in a different CL :)

yeah, it might be a good idea to switch to that to make our lives easier. I
agree. Let's follow up on this on another CL :)

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
File test/perf/input/todomvc3_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
test/perf/input/todomvc3_test.html:90: class TodoMvcBenchmark extends
BenchmarkBase {
On 2013/01/07 21:07:46, John Messerly wrote:
> Minor comment about benchmark library:
> generally we don't use "Base" as a suffix for a base class.
> any idea where I can file that bit of feedback?

Yeah, when I had cloned that code here (patch #5 in this same CL) I had already
done some of that cleanup. But I reverted it when I added the dependency on the
official benchmark harness.

I'll follow up on it :)

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
File test/perf/input/todomvc_async_test.html (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/input/to...
test/perf/input/todomvc_async_test.html:100: .chain((_) => step(() =>
addNote("one")))
On 2013/01/07 21:07:46, John Messerly wrote:
> It would be nice to combine step and chain somehow, to make this cleaner
> 
> perhaps:
> 
> return asyncSteps([
>     app.todos.clear,
>     () => addNote("one"),
>     () => addNote("two")])
> 
> where "asyncSteps" is defined as:
> 
> Future asyncSteps(List<Function> steps) {
>   Future result = step(steps[0]);
>   for (int i = 1; i < steps.length; i++) {
>     result = result.chain((_) => step(steps[i]));
>   }
>   return result;
> }
> 
> 
> There might even be a better way to implement it by inlining "step". Perhaps:
> 
> Future asyncSteps(List<Function> steps) {
>   var completer = new Completer();
>   window.requestLayoutFrame(() {
>     var result = null;
>     for (var step in steps) {
>       result = step();
>       dispatch();
>     }
>     completer.complete(result);
>   });
>   return completer.future;
> }

Good idea - I went with your first proposal (I wanted to have an layout frame on
each step).

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/run.dart
File test/perf/run.dart (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/run.dart...
test/perf/run.dart:6: * This is a helper for run.sh. We try to run all of the
Dart code in one
On 2013/01/07 21:07:46, John Messerly wrote:
> helper for perf.sh?
> should this file be called "perf.dart"?

Done.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/run.dart...
test/perf/run.dart:70: expect(Process.run('sed', [
On 2013/01/07 21:07:46, John Messerly wrote:
> How bad would it be to rewrite this in Dart instead of using "sed"? If it
isn't
> easy, we should make sure appropriate bug has been filed and add TODO here.
> 
> I think with File.readAsStringSync, File.writeAsStringSync, and
> String.replaceAll, it shouldn't be too bad? We might be missing support for
> using the matched groups in the replace expression, though (e.g. "\1"). I'm
not
> sure.

good point - done. I believe the \1 is coming with the new libraries (see
replaceAllMapped)

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/update.dart
File test/perf/update.dart (right):

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/update.d...
test/perf/update.dart:1: #!/usr/bin/env dart
On 2013/01/07 21:07:46, John Messerly wrote:
> rename this script to update_json or copy_json?

Done.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/update.d...
test/perf/update.dart:7: * A simple script that updates a target json file with
the new values from
On 2013/01/07 21:07:46, John Messerly wrote:
> I might've missed where this is called from?

nowhere - it's an utility script used from the command line to help with our
workflow.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/update.d...
test/perf/update.dart:9: */
On 2013/01/07 21:07:46, John Messerly wrote:
> library tag, so this comment has something to be attached to?

Done.

https://chromiumcodereview.appspot.com/11683002/diff/13001/test/perf/update.d...
test/perf/update.dart:18: print('usage: update.dart from.json to.json');
On 2013/01/07 21:07:46, John Messerly wrote:
> Would be nice to print the nice library comment here.

Done.

[Jennifer Messerly, 2013-01-08 02:42:41]

lgtm