1857618 - Ensure normal tasks get enough time to run

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

Currently RefreshDriver tick runs in certain cases a bit too often

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1857618 - Ensure normal tasks get enough time to run, so try to avoid triggering RefreshDriver too often, r=bas

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4953e7db7d08
Ensure normal tasks get enough time to run, so try to avoid triggering RefreshDriver too often, r=bas

Comment 3

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/4953e7db7d08

Comment 4

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Pulsebot from comment #2)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4953e7db7d08
Ensure normal tasks get enough time to run, so try to avoid triggering
RefreshDriver too often, r=bas

== Change summary for alert #39954 (as of Fri, 13 Oct 2023 11:14:09 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
3%	netflix ContentfulSpeedIndex	windows10-64-shippable-qr	fission warm webrender	697.38 -> 677.00
3%	netflix SpeedIndex	windows10-64-shippable-qr	fission warm webrender	754.27 -> 734.17	Before/After

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=39954

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I think I've found a bit better approach for this, and that doesn't seem to cause the same regressions.

Comment 6

[Dianna Smith [:diannaS]]

Backed out of beta for 120.0b6
Backout link: https://treeherder.mozilla.org/jobs?repo=mozilla-beta&revision=c5cefa7cf7968cb679cd9e608c644853e8bf3c5c

Comment 7

[Cristian Tuns]

Backed out of autoland because of some performance regressions
Backout link: https://hg.mozilla.org/integration/autoland/rev/352bc91163706c64c154a76e5f8cef749b955a20

Comment 8

[Alex Finder]

(In reply to Dianna Smith [:diannaS] from comment #6)

Backed out of beta for 120.0b6
Backout link: https://treeherder.mozilla.org/jobs?repo=mozilla-beta&revision=c5cefa7cf7968cb679cd9e608c644853e8bf3c5c

== Change summary for alert #40138 (as of Mon, 06 Nov 2023 17:42:15 GMT) ==

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
8%	amazon ContentfulSpeedIndex	windows10-64-shippable-qr	fission warm webrender	183.33 -> 198.74	Before/After
7%	amazon SpeedIndex	windows10-64-shippable-qr	fission warm webrender	219.36 -> 234.86	Before/After
5%	google-slides fcp	linux1804-64-shippable-qr	fission warm webrender	261.37 -> 273.52	Before/After

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
49%	motionmark-htmlsuite-ramp	linux1804-64-shippable-qr	fission webrender	305.97 -> 456.82	Before/After
43%	motionmark-htmlsuite-ramp	windows10-64-shippable-qr	fission webrender	406.02 -> 582.41	Before/After
41%	twitch FirstVisualChange	macosx1015-64-shippable-qr	cold fission webrender	221.23 -> 129.51	Before/After
35%	motionmark-animometer-ramp	linux1804-64-shippable-qr	fission webrender	368.60 -> 497.34	Before/After
22%	motionmark-animometer-ramp	windows10-64-shippable-qr	fission webrender	579.24 -> 706.78	Before/After
...	...	...	...	...	...
6%	motionmark-htmlsuite	linux1804-64-shippable-qr	fission webrender	56.23 -> 59.52

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=40138

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1857618 - Ensure normal tasks get enough time to run, r=mstange

The basic idea is to check if there are pending tasks also right after RefreshDriver tick has been handled, not only before tick.
But in this new case we can't predict how much time is needed for other tasks, so just a bit of time is given.

The patch tweaks also other things: CanDoCatchUpTick (which is used by catch-up ticks and FinishedWaitingForTransaction) should
return false if the active timer is blocked because of mSuspendVsyncPriorityTicksUntil.
And to make it easier to check whether driver is blocked, mSuspendVsyncPriorityTicksUntil is changed to rely on local time, not the time stamp
from vsync.

The low priority notify is changed so that it uses member variables from the driver and in case it does trigger a tick, it uses the most recent skipped
values.

Comment 10

[Alex Finder]

(In reply to Cristian Tuns from comment #7)

Backed out of autoland because of some performance regressions
Backout link: https://hg.mozilla.org/integration/autoland/rev/352bc91163706c64c154a76e5f8cef749b955a20

== Change summary for alert #40175 (as of Wed, 08 Nov 2023 06:55:53 GMT) ==

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
18%	netflix LastVisualChange	windows10-64-shippable-qr	fission warm webrender	1,171.03 -> 1,385.24	Before/After
18%	netflix PerceptualSpeedIndex	windows10-64-shippable-qr	fission warm webrender	673.38 -> 794.67	Before/After
9%	amazon ContentfulSpeedIndex	windows10-64-shippable-qr	fission warm webrender	188.22 -> 204.67	Before/After
7%	amazon SpeedIndex	windows10-64-shippable-qr	fission warm webrender	223.97 -> 240.57	Before/After
7%	reddit-billgates-post-1.posts LastVisualChange	linux1804-64-shippable-qr	cold fission webrender	625.28 -> 670.38
6%	amazon SpeedIndex	windows10-64-shippable-qr	bytecode-cached fission warm webrender	216.14 -> 230.10	Before/After
4%	netflix ContentfulSpeedIndex	windows10-64-shippable-qr	fission warm webrender	676.98 -> 704.53	Before/After

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
44%	motionmark-htmlsuite-ramp	linux1804-64-shippable-qr	fission webrender	298.47 -> 431.08	Before/After
41%	motionmark-htmlsuite-ramp	windows10-64-shippable-qr	fission webrender	394.54 -> 556.40	Before/After
34%	motionmark-animometer-ramp	linux1804-64-shippable-qr	fission webrender	355.97 -> 476.72	Before/After
20%	motionmark-animometer-ramp	windows10-64-shippable-qr	fission webrender	538.67 -> 644.92	Before/After
11%	outlook ContentfulSpeedIndex	windows10-64-shippable-qr	cold fission webrender	926.32 -> 820.08	Before/After
...	...	...	...	...	...
3%	wikipedia loadtime	linux1804-64-shippable-qr	fission warm webrender	487.37 -> 471.62	Before/After

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=40175

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4683ed11f2cd
Ensure normal tasks get enough time to run, r=mstange

Comment 12

[Serban Stanca [:SerbanS]]

Backed out for causing multiple assertion failures in nsRefreshDriver.cpp.

• Backout link
• Push with failures
• Failure Log
• Failure line: Assertion failure: pendingTaskCount >= (pendingIdleTaskCount + pendingVsyncTaskCount), at /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:663

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Ah, right, we can't assert that. It is race condition.

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/30972f1e5f79
Backed out changeset 4683ed11f2cd for causing multiple assertion failures in nsRefreshDriver.cpp. CLOSED TREE

Comment 15

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #14)

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/30972f1e5f79
Backed out changeset 4683ed11f2cd for causing multiple assertion failures in
nsRefreshDriver.cpp. CLOSED TREE

== Change summary for alert #40245 (as of Wed, 15 Nov 2023 14:16:59 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
12%	youtube ContentfulSpeedIndex	android-hw-a51-11-0-aarch64-shippable-qr	warm webrender	607.01 -> 536.34
11%	youtube ContentfulSpeedIndex	android-hw-a51-11-0-aarch64-shippable-qr	warm webrender	598.13 -> 534.92

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=40245

Comment 16

[Cristian Tuns]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/09acc6030420

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4124706aa8eb
Ensure normal tasks get enough time to run, r=mstange

Comment 18

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/4124706aa8eb

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.