Summary: | Use requestAnimationFrame() to schedule new frames on the performance test Animation/balls.html | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Alberto Lopez Perez <clopez> | ||||
Component: | Tools / Tests | Assignee: | Carlos Alberto Lopez Perez <clopez> | ||||
Status: | NEW --- | ||||||
Severity: | Normal | CC: | benjamin, bugs-noreply, cdumez, cgarcia, commit-queue, dpranke, jdapena, lforschler, ossy, rniwa, simon.fraser, tomz, tonikitoo, tony | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Carlos Alberto Lopez Perez
2016-08-03 06:46:06 PDT
Created attachment 285225 [details]
Patch
Wow, awesome work! Comment on attachment 285225 [details]
Patch
I don't think you should change this test. If it was written using setInterval, then it should stay that way. Feel free to add a new rad-based test alongside this one.
^rAF-based (In reply to comment #0) > The test is measuring much more FPS than it is actually drawing, because it > uses "setInterval(animate,1)" to schedule new frames. > And the issues with this are: > > - Issue #1: There is no guarantee that a given call to animate() would > have finished when the next one arrives. So this is calling animate() each 1 > millisecond without caring about if the previous call to animate() has > finished. > I'm wrong in this point... setInterval() will not fire a new call to the function if the previous has not returned. It will wait until the previous call returns. (In reply to comment #3) > Comment on attachment 285225 [details] > Patch > > I don't think you should change this test. If it was written using > setInterval, then it should stay that way. Feel free to add a new rad-based > test alongside this one. Well, I think the the test is broken. It tries to measure the FPS of the animation, but it not measuring that. It is just measuring how many animate() function calls are executed per second, but is completing ignoring if the new position of the balls is displayed or not on the screen (i.e. the frame has been drawn) Why you think we should not fix a broken test? Or on the other hand do you have some reason to think the test is not broken? (In reply to comment #6) > (In reply to comment #3) > > Comment on attachment 285225 [details] > > Patch > > > > I don't think you should change this test. If it was written using > > setInterval, then it should stay that way. Feel free to add a new rad-based > > test alongside this one. > > Well, I think the the test is broken. > > It tries to measure the FPS of the animation, but it not measuring that. > > It is just measuring how many animate() function calls are executed per > second, but is completing ignoring if the new position of the balls is > displayed or not on the screen (i.e. the frame has been drawn) > > Why you think we should not fix a broken test? > Or on the other hand do you have some reason to think the test is not broken? Because this test represents the way that many old web pages do animation. Even though it doesn't detect real framerate, it will detect changes in setInterval and layout behavior. We have plenty of other perf tests using requestAnimationFrame (in content-animation, and Animometer). (In reply to comment #7) > > Because this test represents the way that many old web pages do animation. > Even though it doesn't detect real framerate, it will detect changes in > setInterval and layout behavior. > So, then I'm a bit confused. How should I interpret the results we get with the new threaded compositor of WebKitGTK+? Without the threaded compositor, the test reports more completed calls to animate() in the first seconds (more FPS according to the the test), but the frames draw are much lower. Is this telling us something useful? (In reply to comment #8) > (In reply to comment #7) > > > > Because this test represents the way that many old web pages do animation. > > Even though it doesn't detect real framerate, it will detect changes in > > setInterval and layout behavior. > > > > So, then I'm a bit confused. > > How should I interpret the results we get with the new threaded compositor > of WebKitGTK+? > > Without the threaded compositor, the test reports more completed calls to > animate() in the first seconds (more FPS according to the the test), but the > frames draw are much lower. > > Is this telling us something useful? It's tell you the round-trip time of setInterval(1) with some style/layout changes in between. We know this isn't a good way to measure actual user-visible framerate. That's why we added Animometer. (In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > > > > Because this test represents the way that many old web pages do animation. > > > Even though it doesn't detect real framerate, it will detect changes in > > > setInterval and layout behavior. > > > > > > > So, then I'm a bit confused. > > > > How should I interpret the results we get with the new threaded compositor > > of WebKitGTK+? > > > > Without the threaded compositor, the test reports more completed calls to > > animate() in the first seconds (more FPS according to the the test), but the > > frames draw are much lower. > > > > Is this telling us something useful? > > It's tell you the round-trip time of setInterval(1) with some style/layout > changes in between. > the round-trip time of setInterval(1) with some style/layout changes _that were not displayed_. Is this really the useful information that we should benchmark and try to optimize for? The purpose of some benchmark test is to have something you want to optimize for, and not measure some internal round-trip that goes nowhere. What if somebody reports a bug with a regression in the FPS rate of Animation/balls? What we should do? Ignore the bug and close it saying that the test is just broken but that we don't want to fix it? I think I will ignore this test from now on. But I also think it does a disservice to all developers having a performance test that reports something that nobody should be optimizing for. > We know this isn't a good way to measure actual user-visible framerate. > That's why we added Animometer. Its the first time I hear about it. At first I tried running it with the run-perf-tests because its under the directory PerformanceTests, but it don't works if run it directly or with run-perf-tests. However it works with the script run-benchmark, so I guess that is how is meant to be executed. Unfortunately the data from the run-benchmark script that the GTK+ bot produces goes nowhere, because (AFAIK) https://perf.webkit.org only supports the data from the script run-benchmark. (In reply to comment #10) > Unfortunately the data from the run-benchmark script that the GTK+ bot > produces goes nowhere, because (AFAIK) https://perf.webkit.org only supports > the data from the script run-benchmark. I mean: https://perf.webkit.org only supports the data from the script run-perf-tests. (In reply to comment #10) > > We know this isn't a good way to measure actual user-visible framerate. > > That's why we added Animometer. > > Its the first time I hear about it. > > At first I tried running it with the run-perf-tests because its under the > directory PerformanceTests, but it don't works if run it directly or with > run-perf-tests. > > However it works with the script run-benchmark, so I guess that is how is > meant to be executed. > This are some numbers I just got for this test: - Without the threaded compositor: Animometer:Score:Geometric: 41.8pt stdev=0.0% Animometer:Score:Geometric: 41.8pt stdev=0.0% Canvas Arcs:Score: 163pt stdev=0.0% Canvas Lines:Score: 1.00pt stdev=0.0% Design:Score: 14.9pt stdev=0.0% Focus:Score: 80.2pt stdev=0.0% Images:Score: 6.00pt stdev=0.0% Leaves:Score: 29.2pt stdev=0.0% Multiply:Score: 271pt stdev=0.0% Paths:Score: 451pt stdev=0.0% Suits:Score: 94.5pt stdev=0.0% - With the threaded compositor: Animometer:Score:Geometric: 74.1pt stdev=0.0% Animometer:Score:Geometric: 74.1pt stdev=0.0% Canvas Arcs:Score: 166pt stdev=0.0% Canvas Lines:Score: 1.00pt stdev=0.0% Design:Score: 15.1pt stdev=0.0% Focus:Score: 328pt stdev=0.0% Images:Score: 196pt stdev=0.0% Leaves:Score: 34.4pt stdev=0.0% Multiply:Score: 258pt stdev=0.0% Paths:Score: 499pt stdev=0.0% Suits:Score: 94.2pt stdev=0.0% |