65734 2011-08-04 17:05:22 -0700 Timer scheduling leads to poor framerate and responsiveness on timer-heavy pages 2016-08-03 11:52:25 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified NEW P2 Normal --- 68729 1 jamesr jamesr ap bburg cmarrin darin dglazkov dimich dino fishd levin mjs rafaelw sam simon.fraser syoichi webkit.review.bot oldest_to_newest 446906 0 jamesr 2011-08-04 17:05:22 -0700 The current Timer scheduling logic in http://trac.webkit.org/browser/trunk/Source/WebCore/platform/ThreadTimers.cpp#L96 services all eligible pending timers for 50ms before yielding, which leads to bad behavior when a page has lots of timers running and can cap the effective framerate to <20FPS as nothing else (such as painting) can be scheduled on the main loop until the timer fires exceed the 50ms cap. Originally this code serviced an unlimited number of timers per call to sharedTimerFired() which could lead to an indefinite hang, so a 50ms cap was added in https://bugs.webkit.org/show_bug.cgi?id=23865. However, the only reason to yield at all after dispatching a timer is if the underlying platform implementation of setFireInterval() / sharedTimerFired() is grossly inefficient. In chromium our implementation of this feature is optimized for frequently yielding and scheduling, so ideal behavior for us would be to fire at most one timer per call to sharedTimerFired(). Other platforms may want to choose a different behavior, but I don't think 50ms makes much sense for anyone. 446908 1 103007 jamesr 2011-08-04 17:08:17 -0700 Created attachment 103007 Test case This test case intentionally floods the browser with far too many timers while attempting to animate a blue box. The box animation is driven by requestAnimationFrame, so it only moves in chromium or Firefox. In Safari try interacting with the text area and notice how crappy it is. The framerate on this test page with ToT WebKit is below 20fps (around 17fps on my box, it depends on the exact timer scheduling) even though the animation logic itself is quite cheap. 446910 2 103008 jamesr 2011-08-04 17:12:25 -0700 Created attachment 103008 Patch 446927 3 dimich 2011-08-04 17:41:04 -0700 (In reply to comment #0) > Originally this code serviced an unlimited number of timers per call to sharedTimerFired() which could lead to an indefinite hang, so a 50ms cap was added in https://bugs.webkit.org/show_bug.cgi?id=23865. However, the only reason to yield at all after dispatching a timer is if the underlying platform implementation of setFireInterval() / sharedTimerFired() is grossly inefficient. The 50ms was a semi-random choice indeed. Executing just one timer and then rescheduling the next time when we can service timers was potentially a bigger behavioral change. I remember thinking about it and I was unsure then if I can come up with enough data to explain why it is safe. The issue was that even if setFireInterval is called with the current time, the platform does not necessarily will call us back immediately or even 'very soon'. Depending on unknown amount of factors (like load on other parts of the system, precision of scheduling mechanism, etc) we might get next call after some delay. If there are many timers that run all the time (as in case you are addressing) the work those timers do may in turn become underserviced and some other parts of the page UI that depend on those timers could become less responsive. So reducing servicing of timers may make one set of pages snappier and another set jerky. Also, it looks like if the page has enough timers to saturate timeline completely it's probably going to suffer anyways... I loaded the test file and interacted with text area while blue thingy was moving back and forth. It looks normal. If it's possible to quantify 'crap' here, it'd be helpful. Otherwise we may 'unfix' this next time someone finds a page which looks a bit better with a delay... 446933 4 jamesr 2011-08-04 17:49:44 -0700 (In reply to comment #3) > (In reply to comment #0) > > Originally this code serviced an unlimited number of timers per call to sharedTimerFired() which could lead to an indefinite hang, so a 50ms cap was added in https://bugs.webkit.org/show_bug.cgi?id=23865. However, the only reason to yield at all after dispatching a timer is if the underlying platform implementation of setFireInterval() / sharedTimerFired() is grossly inefficient. > > The 50ms was a semi-random choice indeed. Executing just one timer and then rescheduling the next time when we can service timers was potentially a bigger behavioral change. I remember thinking about it and I was unsure then if I can come up with enough data to explain why it is safe. The issue was that even if setFireInterval is called with the current time, the platform does not necessarily will call us back immediately or even 'very soon'. Depending on unknown amount of factors (like load on other parts of the system, precision of scheduling mechanism, etc) we might get next call after some delay. If there are many timers that run all the time (as in case you are addressing) the work those timers do may in turn become underserviced and some other parts of the page UI that depend on those timers could become less responsive. The platform mechanism underlying the shared timer can make this decision, however, based on whatever factors it likes since it has the next fire interval available to it (which will be zero if there are more timers immediately ready to fire). We've found a balancing mechanism in chromium that produces the best user experience is one that round robins between multiple inputs. If other ports, however, want to give timers a higher effective priority that is definitely possible without a 50ms cap in ThreadTimers.cpp. > > So reducing servicing of timers may make one set of pages snappier and another set jerky. Also, it looks like if the page has enough timers to saturate timeline completely it's probably going to suffer anyways... > > I loaded the test file and interacted with text area while blue thingy was moving back and forth. It looks normal. If it's possible to quantify 'crap' here, it'd be helpful. Otherwise we may 'unfix' this next time someone finds a page which looks a bit better with a delay... The quantification for "crap" is framerate, in this instance. You can observe the framerate by looking at the Timeline panel in the inspector, or in chrome pass the flags "--force-compositing-mode --show-fps-counter" on the command line. Without this patch, the framerate is limited to ~17fps. With the patch, the page can hit 55fps easily. There's an reciprocal relationship on the delay between user input and visual feedback - without this patch, the delay is around 55ms on average, with the patch it averages around 8ms. 446940 5 dimich 2011-08-04 17:57:04 -0700 The test has a bunch of timers that only do a busy loop. The fix impoves fps of the page because it effectively reduces the amount of work the timers are allowed to do. If the patch disabled timers at all or executed them only rarely, the fps would be even better. Is there a real-life example of a page loaded 100% with timers where this patch improves things? 446970 6 103008 webkit.review.bot 2011-08-04 19:06:35 -0700 Comment on attachment 103008 Patch Attachment 103008 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9305649 New failing tests: inspector/timeline/timeline-script-tag-1.html 446996 7 fishd 2011-08-04 21:09:27 -0700 Processing multiple timers off of a single shared timer is really borked. It causes FIFO ordering problems between MessageLoop::PostTask and WebCore::Timer. It also means that we are screwing over the scheduling of MessageLoop, not allowing other work to happen between Timer instances. There's no good reason for the WebCore shared timer driving multiple timers off of a single callback. 446997 8 fishd 2011-08-04 21:09:49 -0700 (In reply to comment #7) > Processing multiple timers off of a single shared timer is really borked. It causes FIFO ordering problems between MessageLoop::PostTask and WebCore::Timer. > > It also means that we are screwing over the scheduling of MessageLoop, not allowing other work to happen between Timer instances. There's no good reason for the WebCore shared timer driving multiple timers off of a single callback. ^^^ At least not in Chromium. I can believe that a naive shared timer implementation would need this. 447099 9 103008 dimich 2011-08-05 00:46:40 -0700 Comment on attachment 103008 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103008&action=review A general observation: This change has potential to cause regressions or at least behavioral chnages (because it indeed can change the ordering of some tasks/timers). Please give it a Chromium try run before landing. If there is a real scenario that is broken and this is the fix for it then it makes sense to mention it here simply so the next person who will tweak this code has the data. Otherwise, if we are doing it to fix potential FIFO ordering between timers and tasks, we should add this specific info into the bug and ChangeLog. r- due to following: > Source/WebCore/platform/ThreadTimers.cpp:48 > +static const double maxDurationOfFiringTimers = 0; If you want to ensure that the loop only fires one timer in Chromium, it's better to explicitly exit it in case of Chromium instead of using 0 time. It would be easier to understand what's going on. Could you move the #ifdef into the firing function itself? 447295 10 darin 2011-08-05 11:08:49 -0700 I’d like to learn more about this. I don’t understand this comment: > However, the only reason to yield at all after dispatching a timer is if the underlying platform implementation of setFireInterval() / sharedTimerFired() is grossly inefficient. As I understand it, the reason to yield would be that there is user input and processing the user input is more urgent than doing more timer-driven work. Nothing to do with efficiency or “gross inefficiency”. Am I missing something? 560982 11 rafaelw 2012-02-21 10:01:48 -0800 I'm adding a block on the Mutation Observers meta bug here because (if I understand correctly), this may result in mutations *not* being delivered at the end of a Task. 1217049 12 bburg 2016-08-03 11:52:25 -0700 This still looks much worse on WebKit than Chrome/FF. However, I don't know whether real sites have bad performance because of this scheduling. If you have that many timers, and you do any actual work, you are going to have a bad time regardless. 103007 2011-08-04 17:08:17 -0700 2011-08-04 17:08:17 -0700 Test case timers_fairness.html text/html 935 jamesr PCFET0NUWVBFIGh0bWw+CjxkaXYgaWQ9InQiIHN0eWxlPSJ3aWR0aDoxMDBweDsgaGVpZ2h0OjEw MHB4OyBwb3NpdGlvbjphYnNvbHV0ZTsgYmFja2dyb3VuZC1jb2xvcjogYmx1ZTsgbGVmdDowcHgi PjwvZGl2Pgo8dGV4dGFyZWEgc3R5bGU9InBvc2l0aW9uOmFic29sdXRlOyB0b3A6MjAwcHgiPjwv dGV4dGFyZWE+CjxzY3JpcHQ+CmZ1bmN0aW9uIGJ1c3lMb29wKG1pbGxpcykgewogICAgdmFyIHN0 YXJ0ID0gRGF0ZS5ub3coKTsKICAgIHdoaWxlIChEYXRlLm5vdygpIC0gc3RhcnQgPCBtaWxsaXMp IHt9Cn0KCnZhciB0ID0gZG9jdW1lbnQuZ2V0RWxlbWVudEJ5SWQoInQiKTsKCmlmICgid2Via2l0 UmVxdWVzdEFuaW1hdGlvbkZyYW1lIiBpbiB3aW5kb3cpCiAgICByYWYgPSB3aW5kb3cud2Via2l0 UmVxdWVzdEFuaW1hdGlvbkZyYW1lOwplbHNlIGlmICgibW96UmVxdWVzdEFuaW1hdGlvbkZyYW1l IiBpbiB3aW5kb3cpCiAgICByYWYgPSB3aW5kb3cubW96UmVxdWVzdEFuaW1hdGlvbkZyYW1lOwoK cmFmKGZ1bmN0aW9uIGxvb3AoKSB7CiAgICB3aW5kb3cuc2V0VGltZW91dChmdW5jdGlvbigpIHti dXN5TG9vcCg5KTt9LCA1KTsKICAgIHdpbmRvdy5zZXRUaW1lb3V0KGZ1bmN0aW9uKCkge2J1c3lM b29wKDkpO30sIDUpOwogICAgd2luZG93LnNldFRpbWVvdXQoZnVuY3Rpb24oKSB7YnVzeUxvb3Ao OSk7fSwgNSk7CiAgICB3aW5kb3cuc2V0VGltZW91dChmdW5jdGlvbigpIHtidXN5TG9vcCg5KTt9 LCA1KTsKICAgIHdpbmRvdy5zZXRUaW1lb3V0KGZ1bmN0aW9uKCkge2J1c3lMb29wKDkpO30sIDUp OwogICAgd2luZG93LnNldFRpbWVvdXQoZnVuY3Rpb24oKSB7YnVzeUxvb3AoOSk7fSwgNSk7CiAg ICB0LnN0eWxlLmxlZnQgPSAoKERhdGUubm93KCkgLyA4KSAlIDUwMCkgKyAicHgiOwogICAgcmFm KGxvb3ApOwp9KTsKCjwvc2NyaXB0Pgo= 103008 2011-08-04 17:12:25 -0700 2011-08-05 00:46:39 -0700 Patch bug-65734-20110804171223.patch text/plain 2001 jamesr U3VidmVyc2lvbiBSZXZpc2lvbjogOTI0MDcKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBlMjA5OWQ5MTYxZjgzYjU0 NWU0ZDgwYzBhZGEyYzU3M2EyN2VjNGJhLi45MGJhOTEwZDRjNjExZTM2ODk4OTBjZjliNTVkMzA4 OTU2MTI0YzU4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEtMDgtMDQgIEphbWVz IFJvYmluc29uICA8amFtZXNyQGNocm9taXVtLm9yZz4KKworICAgICAgICBUaW1lciBzY2hlZHVs aW5nIGxlYWRzIHRvIHBvb3IgZnJhbWVyYXRlIGFuZCByZXNwb25zaXZlbmVzcyBvbiB0aW1lci1o ZWF2eSBwYWdlcworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9NjU3MzQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBTZXQgdGhlIG1heER1cmF0aW9uT2ZGaXJpbmdUaW1lcnMgdG8gMCBmb3IgY2hyb21pdW0gc28g dGhhdCBhdCBtb3N0IG9uZSBUaW1lciBpcyBzZXJ2aWNlZCBwZXIgY2FsbCB0bworICAgICAgICBz aGFyZWRUaW1lckZpcmVkKCkgaW4gb3JkZXIgdG8gY29vcGVyYXRlIG1vcmUgZmFpcmx5IHdpdGgg cmVuZGVyaW5nIGFuZCBpbnB1dCBldmVudCB1cGRhdGVzLgorCisgICAgICAgICogcGxhdGZvcm0v VGhyZWFkVGltZXJzLmNwcDoKKwogMjAxMS0wOC0wNCAgQnJhZHkgRWlkc29uICA8YmVpZHNvbkBh cHBsZS5jb20+CiAKICAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzk4ODI1ODE+LCA8cmRhcjovL3By b2JsZW0vOTg2ODAxNT4sIGFuZCBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9NjU3MTIKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1RocmVhZFRpbWVy cy5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9UaHJlYWRUaW1lcnMuY3BwCmluZGV4IDk5 OGRjMzJhNjUyMjg2YmZkMTkyZjMzYTA2ZmE3OTU1ZjlmZjk4NjEuLjFjZDVhOTM4ZjhjMzdhMjNh MjY4OTFiY2U1YjVhYWMyOTgxYzZlNzkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL1RocmVhZFRpbWVycy5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vVGhyZWFk VGltZXJzLmNwcApAQCAtNDAsNyArNDAsMTMgQEAgbmFtZXNwYWNlIFdlYkNvcmUgewogLy8gRmly ZSB0aW1lcnMgZm9yIHRoaXMgbGVuZ3RoIG9mIHRpbWUsIGFuZCB0aGVuIHF1aXQgdG8gbGV0IHRo ZSBydW4gbG9vcCBwcm9jZXNzIHVzZXIgaW5wdXQgZXZlbnRzLgogLy8gMTAwbXMgaXMgYWJvdXQg YSBwZXJjZXB0YWJsZSBkZWxheSBpbiBVSSwgc28gdXNlIGEgaGFsZiBvZiB0aGF0IGFzIGEgdGhy ZXNob2xkLgogLy8gVGhpcyBpcyB0byBwcmV2ZW50IFVJIGZyZWV6ZSB3aGVuIHRoZXJlIGFyZSB0 b28gbWFueSB0aW1lcnMgb3IgbWFjaGluZSBwZXJmb3JtYW5jZSBpcyBsb3cuCisjaWYgIVBMQVRG T1JNKENIUk9NSVVNKQogc3RhdGljIGNvbnN0IGRvdWJsZSBtYXhEdXJhdGlvbk9mRmlyaW5nVGlt ZXJzID0gMC4wNTA7CisjZWxzZQorLy8gQ2hyb21pdW0gc2VydmljZXMgb25lIHRpbWVyIHBlciBk aXNwYXRjaCwgc2VlIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02NTcz NC4KKy8vIEZJWE1FOiBSZWR1Y2Ugb3IgcmVtb3ZlIHRoaXMgbGltaXQgZm9yIGFsbCBwbGF0Zm9y bXMuCitzdGF0aWMgY29uc3QgZG91YmxlIG1heER1cmF0aW9uT2ZGaXJpbmdUaW1lcnMgPSAwOyAK KyNlbmRpZgogCiAvLyBUaW1lcnMgYXJlIGNyZWF0ZWQsIHN0YXJ0ZWQgYW5kIGZpcmVkIG9uIHRo ZSBzYW1lIHRocmVhZCwgYW5kIGVhY2ggdGhyZWFkIGhhcyBpdHMgb3duIFRocmVhZFRpbWVycwog Ly8gY29weSB0byBrZWVwIHRoZSBoZWFwIGFuZCBhIHNldCBvZiBjdXJyZW50bHkgZmlyaW5nIHRp bWVycy4K