1839455 - Investigate triggering GC less aggressively in CCGCScheduler::RunNextCollectorTimer

Status: RESOLVED DUPLICATE of bug 1856574

(Core :: DOM: Core & HTML, task, P3)

Description

[Jon Coppeard (:jonco)]

We GC very frequently when running Speedometer3 as can be seen in the profile:

https://share.firefox.dev/3NDWxWU

Most of these GCs have the HTML_PARSER reason which means they are triggered via CCGCScheduler::RunNextCollectorTimer. The idea is that this runs some GC or CC work if there is any waiting. Unfortunately this is almost always true since we starts a GC timer at the end of CC (and vice versa). This means we end up GCing very often with little benefit.

I'd like to try changing this to only GC if there is an ongoing incremental GC, or if we are getting close to the heap limit at which we will start one.

Comment 1

[Jon Coppeard (:jonco)]

Benchmark results for this look promising. Speedometer2 showed improvement with Speedometer3 results being more mixed:

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=a1c0b6a6e774767ee191f9e4769f686f5cae7756&newProject=try&newRevision=ff7813f248fbf12e622075cfc1810ed2e9c799ce&framework=13&page=1

Speedometer2:

• macOS 1.4% overall improvement at high confidence
• Windows 1.25 overall improvement at med confidence

Speedometer3:

• LIttle overall change
• Windows subtests had ~20 improvements at high confidence and 4 regressions
• macOS subtests had ~20 improvements at high confidence and 3 regressions

AWSY results don't show any regressions so far but that's a concern with a change like this.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1839455 - Trigger GC less agressively in CCGCScheduler::RunNextCollectorTimer r?smaug

This changes the behaviour to only run a GC slice here if we are currently in
an incremental GC or are appraoching heap limits that would trigger a
collection.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a69c0deca190
Trigger GC less agressively in CCGCScheduler::RunNextCollectorTimer r=smaug

Comment 4

[Narcis Beleuzu [:NarcisB]]

Backed out for assertion failures on nsCellMap.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/22b91ec6d7394372ad4268ccaa8f8c557c4ba63c
Log link: https://treeherder.mozilla.org/logviewer?job_id=420110844&repo=autoland&lineNumber=2683

Please check also this one https://treeherder.mozilla.org/logviewer?job_id=420117038&repo=autoland&lineNumber=1946

Comment 5

[Jon Coppeard (:jonco)]

This is crashing in accessibility code which isn't obviously related to anything changed in this patch (it's not accessing any GCed data for one).

The test that's crashing is a crash test for bug 1472024 and the crash stack is very similar to the one in that bug. This patch does affect timing so it may be possible that this change is triggering a latent intermittent problem here.

dholbert, you fixed bug 1472024 - do you know what's going on here? https://treeherder.mozilla.org/logviewer?job_id=420110844&repo=autoland&lineNumber=2683

Comment 6

[Alex Finder]

(In reply to Narcis Beleuzu [:NarcisB] from comment #4)

Backed out for assertion failures on nsCellMap.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/22b91ec6d7394372ad4268ccaa8f8c557c4ba63c
Log link: https://treeherder.mozilla.org/logviewer?job_id=420110844&repo=autoland&lineNumber=2683

Please check also this one https://treeherder.mozilla.org/logviewer?job_id=420117038&repo=autoland&lineNumber=1946

== Change summary for alert #38908 (as of Fri, 30 Jun 2023 05:35:45 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
3%	build times	osx	buildbot-unknown plain	3,457.93 -> 3,345.24

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Daniel is on PTO, I can look.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Jonco, I'd land the patch annotating the crashtest with skip-if(isDebugBuild), pointing to bug 1843389. The underlying accessibility bug is not fixed, afaict, see the analysis there.

Comment 9

[Jon Coppeard (:jonco)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)
Good idea, thanks.

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)
Unfortunately there's now another accessibility test failure. I filed bug 1843540 for this.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

That's a non-fatal assertion. Tempted to just do asserts(0-1) in that one and point to the bug, but probably someone more familiar with a11y should stamp that change.

Comment 12

[Jon Coppeard (:jonco)]

Attachment: Bug 1839455 - Disable some accessibility crashtests for known failures r?Jamie

I found these two tests started failing with the other patch in this bug
applied. This patch changes the timing of GCs but should have no effect on
accessibility code, which doesn't touch GCed objects.

Depends on D181539

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/851416c5b619
Trigger GC less agressively in CCGCScheduler::RunNextCollectorTimer r=smaug
https://hg.mozilla.org/integration/autoland/rev/953e77c13acb
Disable some accessibility crashtests for known failures r=Jamie

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/851416c5b619
https://hg.mozilla.org/mozilla-central/rev/953e77c13acb

Comment 15

[Jon Coppeard (:jonco)]

Note to sheriffs: this is probably going to make a whole bunch of performance changes.

Comment 16

[Cristina Horotan [:chorotan]]

Backed out on request changeset 851416c5b619 (bug 1839455) for causing bug 1844365

Backout: https://hg.mozilla.org/integration/autoland/rev/aa209b57371913438965655db017777cbbd86dbd

Comment 17

[Sandor Molnar[:smolnar]]

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

Comment 18

[Jon Coppeard (:jonco)]

Bug 1845433 shows that this change did improve several benchmark results while it was in the codeline:

• speedometer EmberJS-Debug-TodoMVC
• speedometer Inferno-TodoMVC
• speedometer Preact-TodoMVC/Adding100Items
• jetstream2 richards-Geometric
• ares6 Babylon_firstIteration

It also regressed speedometer VanillaJS-TodoMVC/Adding100Items/Sync.

This suggests that the change could be worthwhile if we can mitigate the effects on Speedometer3 on Linux.

Comment 19

[Jon Coppeard (:jonco)]

The main idea here was implemented in bug 1856574.