Closed Bug 1661293 Opened 5 years ago Closed 3 years ago

Preparation for eager GC in mainthread event loop

Tracking

()

Status:

RESOLVED FIXED

Milestone:

103 Branch

Tracking Flags:

Tracking

Status

firefox103

---

fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 1 open bug)

Details

(Keywords: perf-alert)

Attachments

(6 files)

Bug 1661293 - Move idle-time nursery GCs to a new nsJSContext::MaybePokeGC r=jonco,smaug 5 years ago Steve Fink [:sfink] [:s:] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1661293 - Make nsJSContext::MaybePokeGC trigger eager minor GCs, and change the naming to support later adding EAGER_ALLOC_TRIGGER major GCs r=jonco,smaug 5 years ago Steve Fink [:sfink] [:s:] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1661293 - Give CCGCScheduler an mEagerMajorGCReason field (set in a later patch) to indicate that we would like to do a major GC when convenient. This is in addition to the usual mMajorGCReason for when we definitely want to do a GC. r=smaug 4 years ago Steve Fink [:sfink] [:s:] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1661293 - Add a CCGCScheduler.mEagerMinorGCReason field to indicate we have reached a threshold where we would like to do a minor GC if convenient. r=smaug 4 years ago Steve Fink [:sfink] [:s:] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1661293 - Rename JS::RunIdleGCTask -> JS::MaybeRunNurseryCollection and pass through the reason, also rename IDLE_TIME_COLLECTION -> EAGER_NURSERY_COLLECTION. r=smaug 4 years ago Steve Fink [:sfink] [:s:] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1661293 - Remove duplicated logic in the snarl of {gcIfRequested,wantMajorGC,maybeGC} r=jonco 4 years ago Steve Fink [:sfink] [:s:] 48 bytes, text/x-phabricator-request		Details \| Review

Steve Fink [:sfink] [:s:]

Assignee

Description

•

5 years ago

JS_MaybeGC uses a lower threshold for triggering a GC than an allocation-triggered GC. We should consider calling this periodically from the main thread's event loop to make it more likely to do a GC when there is less overhead from scanning the stack.

JS_MaybeGC is already called every second from a worker thread. I was under the mistaken impression that we were calling this from the main thread already.

This might reduce some of our GC overhead, as well as reduce the latency from the initial root marking slice. (Or it might not, if most of our GCs are already coming from the idle task, where the JS stack is already empty.)

Jon Coppeard (:jonco)

Comment 1

•

5 years ago

(In reply to Steve Fink [:sfink] [:s:] from comment #0)
This is a good idea. We probably only want to do this if there is idle time though, so this API isn't ideal.

One way to do that would be to change JS::IsIdleGCTaskNeeded / RunIdleTimeGCTask to start a major GC as well as performing a minor GC.

Severity: -- → N/A

Steve Fink [:sfink] [:s:]

Assignee

Comment 2

•

5 years ago

(In reply to Jon Coppeard (:jonco) (PTO until 4th Jan) from comment #1)

One way to do that would be to change JS::IsIdleGCTaskNeeded / RunIdleTimeGCTask to start a major GC as well as performing a minor GC.

The difficulty with just doing that is that normally when the browser starts a major GC, it also sets up the periodic timer to run the slices. The caller of RunIdleTimeGCTask is in XPCOM, while the timer stuff is in DOM.

It also feels a little unnecessary to have two separate idle triggering mechanisms: the DOM GCTimer stuff in addition to the XPCOM (CycleCollectedJSContext) IsIdleGCTaskNeeded stuff (which creates its own idle runnable).

My current plan:

Remove the existing IsIdleGCTaskNeeded stuff from XPCOM
In XPCJSContext::AfterProcessTask, where it calls MaybePokeCC currently, add a new MaybePokeGC
MaybePokeGC will choose whether to do an immediately nursery collection
MaybePokeGC will also choose whether to start a new major GC (this would be the change in behavior)
if it starts a new major GC, also set up the GCTimer

I hope to do those last two by calling the existing nsJSContext::PokeGC, but I'll need to modify it to not insist on doing a full (all zones) GC in this case.

I might also skip the nursery collection if there's a major GC started or ongoing, under the assumption that it'll figure out before each slice whether it wants to do a nursery collection anyway.

This change would mean that not all CycleCollectedJSContexts will do these nursery and new major GCs automatically; it will only happen for xpconnect ones instead. I think that's fine, since I don't think generic CycleCollectedJSContexts exist in practice.

I'll CC mccr8 in the off chance that he can tell me I'm being stupid before I get too far into this.

Andrew McCreight [:mccr8]

Comment 3

•

5 years ago

How much time are we spending doing stack scanning? How many GCs are triggered by allocation? I'm a little concerned about disrupting the current GC too much/GC not enough balance with this, which sounds like a major change in the way we start GCs.

Also, keep in mind that if you start a GC when we're in the middle of a CC we will synchronously finish the CC, which will certainly cause a large pause.

I'm not familiar with how the existing idle stuff works.

Steve Fink [:sfink] [:s:]

Assignee

Comment 4

•

5 years ago

(In reply to Andrew McCreight [:mccr8] from comment #3)

How much time are we spending doing stack scanning?

For this new path, zero -- we'd only be triggering these from an idle task runner, with nothing on the JS stack. Or rather: this new path would reduce stack scanning, since TOO_MUCH_MALLOC GCs that currently do some stack scanning would be replaced with idle-time preemptive GCs.

How many GCs are triggered by allocation? I'm a little concerned about disrupting the current GC too much/GC not enough balance with this, which sounds like a major change in the way we start GCs.

Looking at telemetry: 47% are CC_FINISHED (the new name of CC_WAITING), meaning they're idle-time. 31% are INTER_SLICE_GC, also idle time. 9% are BG_TASK_FINISHED, which if I'm reading the code correctly gets picked up by an interrupt, so happens in non-idle time. Next is 4% PAGE_HIDE, and everything else is change. Oh, wait: 2.6% are TOO_MUCH_MALLOC, which is relevant here.

This would presumably shift things quite a bit, by adding a bunch of new EAGER_ALLOC_TRIGGER GCs during idle time. But I'm really uncertain of how many will be picked up by this; CC_FINISHED might still get the bulk of the GCs done, and these new GCs will be locked out during the time when INTER_SLICE_GC will be operational. So there's a good chance it won't really do much of anything to the overall picture (and given that it'd be replacing some idle time GCs with different idle time GCs, these shifts may not have any effect.)

The reason for trying this out is to shift more non-idle GCs towards idle (EAGER_ALLOC_TRIGGER) GCs. The non-idle ones may not show up that well in telemetry, but they're more likely to cause noticeable pauses. It also shifts more of the scheduling and timing control to Gecko -- Gecko would now hopefully do an EAGER_ALLOC_TRIGGER GC before we are in danger of being forced to GC due to allocation. So the 2.6% non-idle TOO_MUCH_MALLOC ones would hopefully shift to an idle EAGER_ALLOC_TRIGGER bucket.

I don't trust my ability to predict behavior, though. And this would come at a cost of slightly more GCs, since we'll start doing them a little earlier this way.

Also, keep in mind that if you start a GC when we're in the middle of a CC we will synchronously finish the CC, which will certainly cause a large pause.

With my implementation, which I'll post in a moment, I don't do these eager GCs if we're CCIng. One of the benefits of shifting control to Gecko. :-)

I'm not familiar with how the existing idle stuff works.

There's an idle mechanism that's only used for nursery GCs, where after the event loop is drained (in AfterProcessTask) we check whether we'd like to collect the nursery, and if so, create an idle runnable that does a minor GC.

Then there's the main CC/GC scheduling stuff, that normally runs when idle and mostly decides to GC based on a CC having finished. For this, I would be removing the AfterProcessTask and merging it into the CC/GC setup.

Steve Fink [:sfink] [:s:]

Assignee

Comment 5

•

5 years ago

Attached file Bug 1661293 - Move idle-time nursery GCs to a new nsJSContext::MaybePokeGC r=jonco,smaug — Details

Ratio	Test	Platform	Options	Absolute values (old vs new)
20%	ebay loadtime	windows10-64-shippable-qr	cold fission webrender	473.62 -> 568.12
18%	nytimes fnbpaint	windows10-64-shippable-qr	cold fission webrender	326.23 -> 384.25
17%	nytimes fcp	windows10-64-shippable-qr	cold fission webrender	318.04 -> 373.50
7%	ebay FirstVisualChange	windows10-64-shippable-qr	cold fission webrender	339.38 -> 364.08
6%	ebay fcp	windows10-64-shippable-qr	cold fission webrender	310.15 -> 330.08
6%	booking dcf	android-hw-g5-7-0-arm7-shippable-qr	warm webrender	1,074.21 -> 1,140.42
6%	ebay fnbpaint	windows10-64-shippable-qr	cold fission webrender	314.98 -> 333.29
2%	ebay SpeedIndex	windows10-64-shippable-qr	cold fission webrender	1,421.46 -> 1,455.67
2%	ebay PerceptualSpeedIndex	windows10-64-shippable-qr	cold fission webrender	1,155.42 -> 1,181.50

Ratio	Test	Platform	Options	Absolute values (old vs new)
17%	facebook fcp	android-hw-g5-7-0-arm7-shippable-qr	warm webrender	627.83 -> 522.50
13%	instagram fcp	android-hw-g5-7-0-arm7-shippable-qr	warm webrender	443.17 -> 384.17
13%	instagram fnbpaint	android-hw-g5-7-0-arm7-shippable-qr	warm webrender	455.12 -> 396.04
12%	instagram ContentfulSpeedIndex	android-hw-g5-7-0-arm7-shippable-qr	warm webrender	526.25 -> 462.75
12%	facebook fnbpaint	android-hw-g5-7-0-arm7-shippable-qr	warm webrender	631.75 -> 557.83
...	...	...	...	...
4%	booking loadtime	android-hw-g5-7-0-arm7-shippable-qr	warm webrender	1,232.19 -> 1,185.21

Ratio	Test	Platform	Options	Absolute values (old vs new)
8%	Heap Unclassified	windows10-64-2004-shippable-qr	tp6	81,275,135.17 -> 74,449,480.22
8%	Heap Unclassified	macosx1015-64-shippable-qr	tp6	131,240,698.12 -> 120,834,100.22
7%	Resident Memory	macosx1015-64-shippable-qr	tp6	754,350,340.64 -> 705,149,270.45

Ratio	Test	Platform	Options	Absolute values (old vs new)
16%	nytimes FirstVisualChange	windows10-64-shippable-qr	cold fission webrender	413.92 -> 347.42
13%	nytimes loadtime	windows10-64-shippable-qr	cold fission webrender	1,793.02 -> 1,562.71
4%	buzzfeed PerceptualSpeedIndex	windows10-64-shippable-qr	cold fission webrender	799.00 -> 764.92
4%	buzzfeed ContentfulSpeedIndex	windows10-64-shippable-qr	cold fission webrender	871.67 -> 835.42
3%	buzzfeed SpeedIndex	windows10-64-shippable-qr	cold fission webrender	757.54 -> 733.75

Preparation for eager GC in mainthread event loop

Regressions:

Improvements:

Improvements:

Improvements: