Bug 65734 – Timer scheduling leads to poor framerate and responsiveness on timer-heavy pages

• Component: Platform
• Status: NEW

[James Robinson]

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.

[James Robinson]

Created attachment 103007 [details]
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.

[James Robinson]

Created attachment 103008 [details]
Patch

[Dmitry Titov]

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

[James Robinson]

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

[Dmitry Titov]

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?

[WebKit Review Bot]

Comment on attachment 103008 [details]
Patch

Attachment 103008 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9305649

New failing tests:
inspector/timeline/timeline-script-tag-1.html

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

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

[Dmitry Titov]

Comment on attachment 103008 [details]
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?

[Darin Adler]

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?

[Rafael Weinstein]

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.

[Blaze Burg]

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.