Ben Kelly [:bkelly, not reviewing] Assignee
	Description • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 1 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 2 • 8 years ago

This patch fixes the problem for me, but I should probably turn these constants into preferences.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 3 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 5 • 8 years ago

This fixes an underflow condition I noticed that could become worse with the new constants.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 6 • 8 years ago

This adds preferences so we can tune this more easily in the future.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 7 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=becef4e3e9569b6ec85a8d9ba7d3ca864cc5735e

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 8 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 9 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 10 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 11 • 8 years ago

I would like to uplift this to FF52 if possible.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 12 • 8 years ago

I had a duplicate pref by accident.  The DEBUG try build caught it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a0c701bd03e3c0988a5a12ec31cd787cd8b1660

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 14 • 8 years ago

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

	Pulsebot
	Comment 15 • 8 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

	Carsten Book [:Tomcat]
	Comment 16 • 8 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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 17 • 8 years ago

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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 18 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 19 • 8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/c234f1f1ed00
https://hg.mozilla.org/releases/mozilla-aurora/rev/a07f4b1cef63
https://hg.mozilla.org/releases/mozilla-aurora/rev/49a9d644f1e7

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 20 • 8 years ago

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/81a1e81407be2206b5e00dac1660fc702bed9996
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/8939ba38982b706c6abc62eb2c56aae44c20f0d1
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/3ea27580c85dad05759fbd7ab6aac3ea9a355c2c

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/81a1e81407be
https://hg.mozilla.org/releases/mozilla-esr52/rev/8939ba38982b
https://hg.mozilla.org/releases/mozilla-esr52/rev/3ea27580c85d