Bug 95668 – Performance test should support asynchronous tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Taiju Tsuiki]

Most of storage related APIs are asynchronous. So, we need asynchronous test support to test them.

[Taiju Tsuiki]

Created attachment 161954 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 161954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161954&action=review

> PerformanceTests/ChangeLog:9
> +        Test function for the runAsync and runPerSecondAsync should invoke

Do we really need both runAsync and runPerSecondAsync?

[Taiju Tsuiki]

(In reply to comment #2)
> (From update of attachment 161954 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161954&action=review
> 
> > PerformanceTests/ChangeLog:9
> > +        Test function for the runAsync and runPerSecondAsync should invoke
> 
> Do we really need both runAsync and runPerSecondAsync?

Actually, I don't need runAsync. Adding runAsync corresponding to run looked cleaner to me.
Should I drop runAsync?

[Ryosuke Niwa]

I'd prefer just adding either one as a part of the bug 95704 since we don't normally add unused code.

[Taiju Tsuiki]

Created attachment 164046 [details]
Drop runAsync

[Taiju Tsuiki]

(In reply to comment #4)
> I'd prefer just adding either one as a part of the bug 95704 since we don't normally add unused code.

Done. I dropped runAsync.
Could you take a look again?

[Ryosuke Niwa]

Comment on attachment 164046 [details]
Drop runAsync

View in context: https://bugs.webkit.org/attachment.cgi?id=164046&action=review

> PerformanceTests/resources/runner.js:276
> +    this._doneFunction = function () { if (test.done) test.done(); };
> +    this._description = test.description || "";
> +    this._runCount = test.runCount || 20;
> +    this._callsPerIteration = 1;
> +    this.unit = 'runs/s';
> +
> +    this._test = test;
> +    this._runner = this._perSecondAsyncRunner;
> +    this.initAndStartLoop();

We shouldn't have to duplicate all this code.

> PerformanceTests/resources/runner.js:281
> +    var timeToRun = this._test.timeToRun || 750;

Is 750ms really long enough for async tests?
How many runs per second do we typically get? Anything below 100 runs/s is statistically unsound.
On the other hand, at 100 runs/s, each run is only 10ms at which point we have to worry about the time spent in context switch, etc...

> PerformanceTests/resources/runner.js:305
> +            self._test.run(nextIteration.bind(window, j + 1));

It's not clear to me how this "run" function is used. I would really like to see this patch being merged with the one that adds an actual test.

Also, we need to be careful with calling functions like "bind". It may affect V8 behavior in subtle ways.

> PerformanceTests/resources/runner.js:310
> +            totalTime += Date.now() - startTime;
> +            i += callsPerIteration;
> +            if (self._completedRuns < 0 && totalTime < 100)
> +                callsPerIteration = Math.max(10, 2 * callsPerIteration);

How slow is async function call here? If an async benchmark actually requires a context switch between two threads, then 100ms is probably not long enough.

[Taiju Tsuiki]

Created attachment 164169 [details]
add test, merge runPerSecond to async version

[Taiju Tsuiki]

Comment on attachment 164046 [details]
Drop runAsync

View in context: https://bugs.webkit.org/attachment.cgi?id=164046&action=review

>> PerformanceTests/resources/runner.js:276
>> +    this.initAndStartLoop();
> 
> We shouldn't have to duplicate all this code.

I merged runPerSecond to runPerSecondAsync.

>> PerformanceTests/resources/runner.js:281
>> +    var timeToRun = this._test.timeToRun || 750;
> 
> Is 750ms really long enough for async tests?
> How many runs per second do we typically get? Anything below 100 runs/s is statistically unsound.
> On the other hand, at 100 runs/s, each run is only 10ms at which point we have to worry about the time spent in context switch, etc...

For simple filesystem test like creating file or write small data to a file, it runs around 3500 times/sec on DumpRenderTree and over 1000 times/sec on Chrome. I think it's enough.

>> PerformanceTests/resources/runner.js:305
>> +            self._test.run(nextIteration.bind(window, j + 1));
> 
> It's not clear to me how this "run" function is used. I would really like to see this patch being merged with the one that adds an actual test.
> 
> Also, we need to be careful with calling functions like "bind". It may affect V8 behavior in subtle ways.

I see. I'll merge another change to this.

I don't know how much time bind() need... Can we use anonymous function safely instead?

>> PerformanceTests/resources/runner.js:310
>> +                callsPerIteration = Math.max(10, 2 * callsPerIteration);
> 
> How slow is async function call here? If an async benchmark actually requires a context switch between two threads, then 100ms is probably not long enough.

My tests is enough fast, at least, even when it needs IPC round trip.
If some test needs more time, it might need to be configurable.

[Ryosuke Niwa]

(In reply to comment #9)
> For simple filesystem test like creating file or write small data to a file, it runs around 3500 times/sec on DumpRenderTree and over 1000 times/sec on Chrome. I think it's enough.

Okay.

> I see. I'll merge another change to this.
> 
> I don't know how much time bind() need... Can we use anonymous function safely instead?

Ideally, the code that runs between each run is as small/fast as possible. In general, anything that non-trivial like creating new array, accessing a DOM node, creating an anonymous function, etc... should be avoided as much as possible. On the other hand, things like integer arithmetics should be fast and probably won't affect the js heap.

> My tests is enough fast, at least, even when it needs IPC round trip.
> If some test needs more time, it might need to be configurable.

Okay.

[Ryosuke Niwa]

Comment on attachment 164169 [details]
add test, merge runPerSecond to async version

Ugh... this is very outdated now. Please upload a new patch that applies to ToT.

[Ryosuke Niwa]

Created attachment 169247 [details]
Convert Animation/balls to use PerfTestRunner.measureValueAsync

[Ryosuke Niwa]

Created attachment 169250 [details]
Convert Animation/balls to use PerfTestRunner.measureValueAsync

[Ryosuke Niwa]

Committed r131638: <http://trac.webkit.org/changeset/131638>