Bug 78789

Summary: Import themaninblue.com/experiment/AnimationBenchmark/ as performance tests
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Tools / TestsAssignee: Tom Zakrajsek <tomz>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cmarcelo, dpranke, menard, morrita, ojan, simon.fraser, thorton, tomz, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 77037    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Ryosuke Niwa 2012-02-16 01:57:59 PST
Per my request, the author of http://themaninblue.com/experiment/AnimationBenchmark/html/ have LGPL-licensed his animation benchmarks: http://themaninblue.com/writing/perspective/2010/03/22/
We should import them as performance tests.
Comment 1 Tom Zakrajsek 2012-06-04 16:14:12 PDT
Created attachment 145649 [details]
Patch
Comment 2 Ryosuke Niwa 2012-06-04 16:24:33 PDT
Comment on attachment 145649 [details]
Patch

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

> Tools/Scripts/webkitpy/performance_tests/perftest.py:95
> +        re.compile(r'^\d+(.\d+)?(\s*(runs\/s|ms|FPS|fps))?$'),

Why do we need to support both FPS and fps? To be consistent, I'd prefer just supporting the lowercase fps.

> PerformanceTests/Animation/balls.html:272
> +        // Get the balls and FPS out of our way

I don't think this comment is helpful.

> PerformanceTests/Animation/balls.html:278
> +        for (var particle in particles)
> +        {
> +            var p = particles[particle];
> +            particles[particle] = 0;
> +            p.destroy();
> +        }

Please use WebKit style for { and } (e.g. for (...) {).

> PerformanceTests/Animation/balls.html:289
> +        for (var rate in frameRates)
> +        {
> +            PerfTestRunner.log(frameRates[rate].toFixed(2) + " FPS");
> +        }

No curly brackets around a single line statement.

> PerformanceTests/Animation/balls.html:305
> +        if (++run < maxRuns)
> +        {
> +            init();
> +        }
> +        else
> +        {
> +            if (window.layoutTestController) {
> +                layoutTestController.notifyDone();
> +            }
> +        }

Ditto.
Comment 3 Tom Zakrajsek 2012-06-04 16:35:09 PDT
Thanks for the feedback.  I thought minimizing change was more important than fitting our style. I'll fix it.  And, i'll restrict to fps.  I also realized that the stdev goes down *significantly* if I widen the resolution of the timing to every ten screen updates.  Average and Median frame rates stay about the same.  Any moral problems with that?
Comment 4 Ryosuke Niwa 2012-06-04 16:38:40 PDT
(In reply to comment #3)
> Thanks for the feedback.  I thought minimizing change was more important than fitting our style. I'll fix it.  And, i'll restrict to fps.  I also realized that the stdev goes down *significantly* if I widen the resolution of the timing to every ten screen updates.  Average and Median frame rates stay about the same.  Any moral problems with that?

Whatever works for you. Could you also report post the sample results (4-5 runs) somewhere and tell us how long it takes to run the test?
Comment 5 Tom Zakrajsek 2012-06-18 13:29:24 PDT
Created attachment 148161 [details]
Patch
Comment 6 Tom Zakrajsek 2012-06-18 13:34:47 PDT
[tomz-mac-pro] WebKit> time run-perf-tests PerformanceTests/Animation/balls.html
Running 1 tests
Running Animation/balls.html (1 of 1)
RESULT Animation: balls= 169.223297408 fps
median= 168.918918919 fps, stdev= 7.89349356211 fps, min= 160.0 fps, max= 192.307692308 fps
real 2.749	user 2.464	sys 0.485	pcpu 100.00

[tomz-mac-pro] WebKit> time run-perf-tests PerformanceTests/Animation/balls.html
Running 1 tests
Running Animation/balls.html (1 of 1)
RESULT Animation: balls= 167.750742674 fps
median= 166.270783848 fps, stdev= 8.07382328862 fps, min= 160.0 fps, max= 192.307692308 fps
real 2.759	user 2.485	sys 0.486	pcpu 100.00

[tomz-mac-pro] WebKit> time run-perf-tests PerformanceTests/Animation/balls.html
Running 1 tests
Running Animation/balls.html (1 of 1)
RESULT Animation: balls= 168.481646673 fps
median= 167.714884696 fps, stdev= 8.06403523694 fps, min= 158.73015873 fps, max= 192.307692308 fps
real 2.739	user 2.469	sys 0.486	pcpu 100.00

[tomz-mac-pro] WebKit> time run-perf-tests PerformanceTests/Animation/balls.html
Running 1 tests
Running Animation/balls.html (1 of 1)
RESULT Animation: balls= 165.902753749 fps
median= 164.705882353 fps, stdev= 7.51791081831 fps, min= 158.73015873 fps, max= 188.679245283 fps
real 2.760	user 2.476	sys 0.489	pcpu 100.00

[tomz-mac-pro] WebKit> time run-perf-tests PerformanceTests/Animation/balls.html
Running 1 tests
Running Animation/balls.html (1 of 1)
RESULT Animation: balls= 171.81506867 fps
median= 171.30620985 fps, stdev= 8.34146408445 fps, min= 161.290322581 fps, max= 196.078431373 fps
real 2.728	user 2.455	sys 0.487	pcpu 100.00
Comment 7 Ryosuke Niwa 2012-06-18 13:38:42 PDT
Comment on attachment 148161 [details]
Patch

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

> PerformanceTests/Animation/balls.html:263
> +        else
> +            if (window.layoutTestController)
> +                layoutTestController.notifyDone();

Please put if & else on the same line.
Comment 8 Tim Horton 2012-06-18 13:39:52 PDT
(In reply to comment #6)
> [tomz-mac-pro] WebKit> time run-perf-tests PerformanceTests/Animation/balls.html
> RESULT Animation: balls= 169.223297408 fps
> median= 168.918918919 fps, stdev= 7.89349356211 fps, min= 160.0 fps, max= 192.307692308 fps

What's the WebKit performance test policy on importing tests that are ridiculously fast? 160fps is crazy, and some systems might have optimizations that perhaps make the testing unfair. Do we have a "target" fps, or a mechanism for scaling tests, or anything?
Comment 9 Ryosuke Niwa 2012-06-18 13:42:13 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > [tomz-mac-pro] WebKit> time run-perf-tests PerformanceTests/Animation/balls.html
> > RESULT Animation: balls= 169.223297408 fps
> > median= 168.918918919 fps, stdev= 7.89349356211 fps, min= 160.0 fps, max= 192.307692308 fps
> 
> What's the WebKit performance test policy on importing tests that are ridiculously fast? 160fps is crazy, and some systems might have optimizations that perhaps make the testing unfair. Do we have a "target" fps, or a mechanism for scaling tests, or anything?

We don't have a policy :) If we think that's too fast, maybe we can tweak it to add more load?
Comment 10 Simon Fraser (smfr) 2012-06-18 13:46:31 PDT
Any test that runs > 60fps is useless as a perf test. It's just testing throttling at various places in the whole stack.
Comment 11 Tim Horton 2012-06-18 13:46:43 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > [tomz-mac-pro] WebKit> time run-perf-tests PerformanceTests/Animation/balls.html
> > > RESULT Animation: balls= 169.223297408 fps
> > > median= 168.918918919 fps, stdev= 7.89349356211 fps, min= 160.0 fps, max= 192.307692308 fps
> > 
> > What's the WebKit performance test policy on importing tests that are ridiculously fast? 160fps is crazy, and some systems might have optimizations that perhaps make the testing unfair. Do we have a "target" fps, or a mechanism for scaling tests, or anything?
> 
> We don't have a policy :) If we think that's too fast, maybe we can tweak it to add more load?

I use this test (and its siblings) with MAX_PARTICLES set to 10,000 :P

This (having a target speed, etc.) is probably something we should look into sooner rather than later; as graphics subsystems get smarter, there's no reason they can't throw out or defer work that won't make it to the screen immediately (or ever), and there's potential for system-level fps capping that we should watch out for.

In any case, something to think about.
Comment 12 Tom Zakrajsek 2012-06-18 13:55:24 PDT
One note about the high FPS... this actually runs slower (like 1/3 -1/2 the speed) when you run it in a browser.  But scaling the FPS is trivial.  If you triple MAX_PARTICLES, the rate goes down proportionally.  I'll update with the nit fix.
Comment 13 Ryosuke Niwa 2012-06-19 20:27:45 PDT
So... should we land this patch? Or should we tweak it first?
Comment 14 Tom Zakrajsek 2012-06-20 07:07:16 PDT
Created attachment 148558 [details]
Patch
Comment 15 Tom Zakrajsek 2012-06-20 08:09:25 PDT
Comment on attachment 148558 [details]
Patch

Clearing flags on attachment: 148558

Committed r120823: <http://trac.webkit.org/changeset/120823>
Comment 16 Tom Zakrajsek 2012-06-20 08:09:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Dominik Röttsches (drott) 2012-06-20 08:45:18 PDT
Comment on attachment 148558 [details]
Patch

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

> Tools/Scripts/webkitpy/performance_tests/perftest.py:138
> +                _log.error("PerfTest can't parse line [" + line + "]")

Looks like this causes a python tests regression:
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/1653/steps/webkitpy-test/logs/stdio