[webkit-reviews] review granted: [Bug 86021] performance tests should be able to measure runs/sec rather than time : [Attachment 141553] Adds PerfTestRunner.runPerSecond and converts flexbox tests

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 86021: performance tests should be able to measure runs/sec rather than
time
https://bugs.webkit.org/show_bug.cgi?id=86021

Attachment 141553: Adds PerfTestRunner.runPerSecond and converts flexbox tests
https://bugs.webkit.org/attachment.cgi?id=141553&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=141553&action=review


> PerformanceTests/ChangeLog:17
> +	   By default, runPerSecond runs the test for at least 750 milliseconds
in each run, and executes
> +	   21 runs, yielding the total run time of roughly 18 seconds. This is
significantly faster than

Do we really need to run 21 times to get the variance down? Seems excessive to
me. Have you looked at what V8 does? They've have lots of time to tweak their
test runner to reduce variance, but still be fast. With every test taking 18
seconds, the cycle time for the bots will be huge as people add tests and it
will be harder to tell which patch caused a regression.

> PerformanceTests/resources/runner.js:79
> +    result.unit = unit || "ms";

Looks like you always set unit, so do you need this ||?

> PerformanceTests/resources/runner.js:167
> +PerfTestRunner.runPerSecond = function (test) {

It's up to you, but I would call this method run and call the other method
runTime or something like that. Whichever method is called run is the one naive
test writers will use.

> PerformanceTests/resources/runner.js:179
> +    this._runCount = test.runCount || 21;
> +    this._doneFunction = function () { if (test.done) test.done(); };
> +    this._completedRuns = 0;
> +    this._callsPerIteration = 1;
> +    this.customRunFunction = null;
> +    this.unit = 'runs/s';
> +    this._results = [];
> +
> +    this._test = test;
> +    this._runner = this._perSecondRunner;
> +    this.log("Running " + this._runCount + " times");
> +    this._runLoop();

Looks like this duplicates a lot of code in PerfTestRunner.run. Could they both
call a shared helper function? PerfTestRunner._resetState or something?

> PerformanceTests/resources/runner.js:195
> +	   totalTime += this._perSecondRunnerIterator(callsPerIteration);
> +	   i += callsPerIteration;
> +	   if (this._completedRuns <= 0 && totalTime < 100)
> +	       callsPerIteration = Math.max(10, 2 * callsPerIteration);

Again, it would be good to see what the V8 test harness does for this loop.

> PerformanceTests/resources/runner.js:212
> +    var startTime = new Date();
> +    for (var i = 0; i < callsPerIteration; i++)
> +	   this._test.run();
> +    return new Date() - startTime;

Nit: s/new Date()/Date.now()/. Saves the cost of instantiating a Date object,
which is most of the cost.