Closed Bug 1336822 Opened 3 years ago Closed 3 years ago

Optimize ResetTimersForThrottleReduction

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(1 file)

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 bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9071b29a86
Optimize ResetTimesForTHrottleReduction() for common case. r=ehsan
https://hg.mozilla.org/mozilla-central/rev/ce9071b29a86
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Attachment #8833784 - Flags: approval-mozilla-beta?
Attachment #8833784 - Flags: approval-mozilla-aurora?
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.
Attachment #8833784 - Flags: approval-mozilla-beta?
Attachment #8833784 - Flags: approval-mozilla-beta+
Attachment #8833784 - Flags: approval-mozilla-aurora?
Attachment #8833784 - Flags: approval-mozilla-aurora+
Setting qe-verify- based on Comment 9.
Flags: qe-verify-
Iteration: --- → 54.2 - Feb 20
Whiteboard: [e10s-multi:+]
Iteration: 54.2 - Feb 20 → ---
Whiteboard: [e10s-multi:+]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.