Closed Bug 1336822 Opened 3 years ago Closed 3 years ago
Timers For Throttle Reduction
Currently ResetTimersForThrottleReduction() re-inserts every timer it modifies. This makes the algorithm O(n^2). In most cases, though, we will be shifting a lot of timers by the same amount and the order won't change. We can optimize for this case to avoid large pauses due to re-sorting.
Ehsan, while testing my time-flood case here: https://people-mozilla.org/~bkelly/timer-flood/index.html I am seeing massive main thread jank due to ResetTimersForThrottleReduction(). This algorithm is O(n^2) due to its re-insertion of every modified timer. I think we can avoid a lot of this work, though, because in the common case we will be shifting large blocks of timers in the same direction. This patch only does the re-insertion if we detect that the order actually changed. This completely removes the jank from this method in that test case for me.
Attachment #8833784 - Flags: review?(ehsan)
I would like to uplift this to FF52 if possible.
Comment on attachment 8833784 [details] [diff] [review] Optimize ResetTimesForTHrottleReduction() for common case. r=ehsan Review of attachment 8833784 [details] [diff] [review]: ----------------------------------------------------------------- Nice!
Attachment #8833784 - Flags: review?(ehsan) → review+
Also note that we run this code on tab switch when a window is brought back into the foreground. So it could benefit some of our top line metrics.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9071b29a86 Optimize ResetTimesForTHrottleReduction() for common case. r=ehsan
Comment on attachment 8833784 [details] [diff] [review] Optimize ResetTimesForTHrottleReduction() for common case. r=ehsan Approval Request Comment [Feature/Bug causing the regression]: This is an additional change to realize the benefits of bugs 1300659. I'd like to write a blog post about our timer improvements in FF52 and I need this patch for the demonstration to work well. [User impact if declined]: Reduced jank when there are large numbers of timers and the user switches tabs, etc. [Is this code covered by automated tests?]: We have extensive tests that trigger this code. [Has the fix been verified in Nightly?]: This patch has been in nightly for a week without problem. [Needs manual test from QE? If yes, steps to reproduce]: None [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Low risk [Why is the change risky/not risky?]: The patch has been proven on nightly and is quite small. It simply avoids re-inserting data into a list if we can determine that the order did not actually change. [String changes made/needed]: None
Note, the patch will need a rebase since the code moved from nsGlobalWindow.cpp to TimeoutManager.cpp in FF53 or FF54. I'll handle this if the uplift is approved.
Comment on attachment 8833784 [details] [diff] [review] Optimize ResetTimesForTHrottleReduction() for common case. r=ehsan Another small fix for timer issues, Probably this will miss the build for beta 6 but can still make it to beta 7 later this week.
Setting qe-verify- based on Comment 9.
Iteration: --- → 54.2 - Feb 20
Iteration: 54.2 - Feb 20 → ---
You need to log in before you can comment on or make changes to this bug.