timer anti-flood patches are not working anymore

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox51 unaffected, firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

I ran my local test for bug 1300659 again today and unfortunately it doesn't seem to be working in any branch.  Not sure what changed to break this.
It looks like I just mistuned the constants in bug 1319991.  The anti-flood mechanism is working, but its still janking the animations in my test because the hysteresis is not big enough.
Blocks: 1319991
No longer blocks: 1300659
This patch fixes the problem for me, but I should probably turn these constants into preferences.
I wrapped my anti-flood test into a page here.  We should be able to start and stop a flood without janking the animated GIF.
(Assignee)

Updated

2 years ago
Depends on: 1336822
This fixes an underflow condition I noticed that could become worse with the new constants.
This adds preferences so we can tune this more easily in the future.
Comment on attachment 8833785 [details] [diff] [review]
P1 Tune setTimeout anti-flood constants to minimize jank. r=smaug

This patch tunes some of the back pressure heuristic constants to avoid jank.
Attachment #8833785 - Flags: review?(bugs)
Comment on attachment 8833787 [details] [diff] [review]
P2 Avoid underflow in timeout CancelOrUpdateBackpressure(). r=smaug

Try to avoid an underflow condition by reversing the comparison around.  We now add instead of subtracting.  In general these values will be close to zero and not close to UINT32_MAX; so overflow is not much of a concern.
Attachment #8833787 - Flags: review?(bugs)
Comment on attachment 8833789 [details] [diff] [review]
P3 Add preferences to control timeout back pressure algorithm. r=smaug

Add preferences so we can more easily tune these values in the future.
Attachment #8833789 - Flags: review?(bugs)
I would like to uplift this to FF52 if possible.
I had a duplicate pref by accident.  The DEBUG try build caught it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a0c701bd03e3c0988a5a12ec31cd787cd8b1660
Attachment #8833789 - Attachment is obsolete: true
Attachment #8833789 - Flags: review?(bugs)
Attachment #8833797 - Flags: review?(bugs)
Comment on attachment 8833785 [details] [diff] [review]
P1 Tune setTimeout anti-flood constants to minimize jank. r=smaug

yeah, we really need some tests to ensure these values are reasonable and don't regress.
Attachment #8833785 - Flags: review?(bugs) → review+
Attachment #8833787 - Flags: review?(bugs) → review+
Attachment #8833797 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #13)
> yeah, we really need some tests to ensure these values are reasonable and
> don't regress.

So, we do have a timer flood test, but it really just checks that we don't completely lock the main thread.  I'm not sure how to write an automated test that verifies we don't jank an animated GIF.

Also, these tunings are a bit of a bandaid.  If the processor is faster or we start handling timers faster we can actually start triggering this again.  I discovered that while testing my timer optimization patches in bug 1325254.  With those patches we process about 5x as many timers in the same period on this test.  This lets us drain the backlog back to zero when backpressure is first enabled and this triggers the ResetTimersForThrottleReduction() on a large number of timers.  Since ResetTimersForThrottleReduction() is O(n^2) this janks hard.

To help mitigate this further I have some proposed optimizations for ResetTimersForThrottleReduction() in bug 1336822.

With the patches from bug 1336598 (this bug), bug 1336822, and bug 1325254 I can run the test without janking the animation.  And because of the optimizations we handle about 5x as many timers as we do currently.

Comment 15

2 years ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/901008a059b6
P1 Tune setTimeout anti-flood constants to minimize jank. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/79f1f1073258
P2 Avoid underflow in timeout CancelOrUpdateBackpressure(). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca7ec2dd740d
P3 Add preferences to control timeout back pressure algorithm. r=smaug

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/901008a059b6
https://hg.mozilla.org/mozilla-central/rev/79f1f1073258
https://hg.mozilla.org/mozilla-central/rev/ca7ec2dd740d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8833785 [details] [diff] [review]
P1 Tune setTimeout anti-flood constants to minimize jank. r=smaug

Approval Request Comment
[Feature/Bug causing the regression]: These patches are a few tuning changes needed to realize the benefits of bug 1300659.  I'd like to write a blog post about our timer improvements in FF52 and I need these patches for the demonstration to work well.
[User impact if declined]: Janky browser performance during timer floods.
[Is this code covered by automated tests?]: We have many tests cover timers and a test covering timer floods.
[Has the fix been verified in Nightly?]: Yes, the patches have been in nightly for a week.
[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 risk
[Why is the change risky/not risky?]: These patches tune some constants, fix a small underflow condition, and add preferences to make tuning easier in the future.  There is very low risk of regression.
[String changes made/needed]: None

Note, these patches will need some rebasing since timer code moved from nsGlobalWindow TimeoutManager in FF53 or FF54.  I'll handle this if the uplift is approved.
Attachment #8833785 - Flags: approval-mozilla-beta?
Attachment #8833785 - Flags: approval-mozilla-aurora?
Comment on attachment 8833785 [details] [diff] [review]
P1 Tune setTimeout anti-flood constants to minimize jank. r=smaug

Fix for an issue that started in 52, should fix some timeouts.
Attachment #8833785 - Flags: approval-mozilla-beta?
Attachment #8833785 - Flags: approval-mozilla-beta+
Attachment #8833785 - Flags: approval-mozilla-aurora?
Attachment #8833785 - Flags: approval-mozilla-aurora+
Iteration: --- → 54.2 - Feb 20
Whiteboard: [e10s-multi:+]
Iteration: 54.2 - Feb 20 → ---
Whiteboard: [e10s-multi:+]
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.