I was testing how well the new back pressure mechanism worked with timer bombs on some pages today and noticed a minor problem. While the timer bomb is ramping up it can enter and leave back pressure a few times before its solidly stays in back pressure. This triggers a few ResetTimersForThrottleReduction() calls which can be quite expensive. I think we should perhaps add some hysteresis to the back pressure mechanism. We would increase our back pressure delay whenever necessary, but only reduce the delay if the reduction would be significant. For example: * Allow a 990ms delay to increase to 1000ms delay * Do not allow a 1000ms delay to decrease to 990ms * Allow a 1000ms delay to decrease to 750ms Again, this is to reduce how often we call ResetTimersForThrottleReduction(). I noticed this while viewing the animated GIF on this site: http://nolanlawson.github.io/database-comparison/ And running this code in the console: function boom() { setTimeout(boom, 0); setTimeout(boom, 0); } boom() I took a mac os process sample during some animation jank and observed multiple ResetTimersForThrottleReduction() calls.

Comment on attachment 8816444 [details] [diff] [review] Introduce hysteresis into window timer back pressure calculations. r=smaug When testing a timer flood on a page with an animated gif I noticed the animation would jank a few times after starting the flood. The profiler showed this was due to thrashing in and out of back pressure calling ResetTimersForThrottleReduction() a lot. This patch avoids the issue by introducing some hysteresis. Once in back pressure we will increase our throttle delay immediately, but when the queue is drained we resists reducing the delay. Only if the delay should be reduced by a significant amount do we apply the change and call ResetTimersForThrottleReduction(). With this patch the animated gif does not jank any more.

Comment on attachment 8816444 [details] [diff] [review] Introduce hysteresis into window timer back pressure calculations. r=smaug I realized there is a bug here. With this patch we could allow back pressure delay to drop to say 100ms, but then because of the reduction limit prevent it from ever dropping all the way to zero.

This patch makes the back pressure delay a bit "sticky". We allow it to increase at will, but we require the desired delay to drop significantly before we actually adjust the delay down. This avoids thrashing the back pressure delay up and down as the ThrottledEventQueue backlog fluctuates. This in turn avoids calling ResetTimersForThrottleReduction() too often. In practical terms this avoids some jank I observed when the a page was ramping up its timer usage. As it crossed the back pressure threshold it briefly then dropped back down below the threshold. This caused us to recalculate timers a number of times janking the onscreen animation. This patch allows us to ramp up timers without janking. We only incur the timer recalculation cost when actually coming out of the back pressure. I verified with local debug that we do indeed return to our normal state without back pressure when timers stop getting scheduled. I don't think I write an automated test for this without making something very racy.

Comment on attachment 8818104 [details] [diff] [review] Aurora 52 patch: Introduce hysteresis into window timer back pressure calculations. r=smaug >+ >+ // If the delay has increased, then simply apply it. Increasing the delay >+ // does not risk re-ordering timers. Why not? This could use some better explanation. I guess the ordering isn't really guaranteed anyhow, so this is ok

(In reply to Olli Pettay [:smaug] from comment #7) > >+ // If the delay has increased, then simply apply it. Increasing the delay > >+ // does not risk re-ordering timers. > Why not? This could use some better explanation. I guess the ordering isn't > really guaranteed anyhow, so this is ok Consider two setTimeout() calls: let a = setTimeout(func, 0); let b = setTimeout(func, 0); If we increase the delay between `a` and `b`, we can be sure that `b` will still fire after `a`. I guess it doesn't really help in the case of: let a = setTimeout(func, 100); let b = setTimeout(func, 0); I was more concerned with the first case, however, because I feel like a lot of sites do setTimeout(func, 0) as a "next tick" operation. There is probably a fair bit of code out there which assumes ordering is guaranteed in those cases. I'll clarify in the comment that it helps ensure that setTimeout() with similar delay values are not re-ordered.

I also need to rebase around Ehsan's timeout refactor. Thats somewhat unfortunate since it means I can't as easily uplift the fix.

Comment on attachment 8818104 [details] [diff] [review] Aurora 52 patch: Introduce hysteresis into window timer back pressure calculations. r=smaug I guess I can just use this one for uplift.

Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e6be7f76066e Introduce hysteresis into window timer back pressure calculations. r=smaug

Comment on attachment 8818104 [details] [diff] [review] Aurora 52 patch: Introduce hysteresis into window timer back pressure calculations. r=smaug Approval Request Comment [Feature/Bug causing the regression]: Bug 1300659 and bug 1318084 introduced a back pressure mechanism for sites that abuse setTimeout() calls. There is a small problem in the code that can trigger some main thread jank when the back pressure is triggered. [User impact if declined]: Poor responsiveness under loaded conditions. [Is this code covered by automated tests?]: Timers are extensively tested, but the exact semantics of the back pressure cannot be automated due to races. [Has the fix been verified in Nightly?]: I have verified it locally. [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Minimal [Why is the change risky/not risky?]: Its a change to an algorithm that should be rarely triggered. [String changes made/needed]: None

Comment on attachment 8818104 [details] [diff] [review] Aurora 52 patch: Introduce hysteresis into window timer back pressure calculations. r=smaug tweak new back pressure mechanism in aurora52