Bug 78789

Created attachment 145649 [details] Patch

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.

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?

(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?

Created attachment 148161 [details] Patch

[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 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.

(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?

(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?

Any test that runs > 60fps is useless as a perf test. It's just testing throttling at various places in the whole stack.

(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.

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.

So... should we land this patch? Or should we tweak it first?

Created attachment 148558 [details] Patch

Comment on attachment 148558 [details] Patch Clearing flags on attachment: 148558 Committed r120823: <http://trac.webkit.org/changeset/120823>

All reviewed patches have been landed. Closing bug.

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