1197902 - Reduce the number of CCTimerFired wakeups

Status: NEW

(Core :: Cycle Collector, task, P3)

Description

[Andrew McCreight [:mccr8]]

njn found that around 40% of the time CCTimerFired doesn't do anything. Another 50% of the time, the purple buffer threshold is exceeded and it runs FireForgetSkippable(). We might be able to get rid of most of the ones that don't do anything by monitoring the purple buffer size in the purple buffer itself, and triggering some kind of callback when the threshold is exceeded.

Comment 1

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

Monitoring purple buffer size in purple buffer 'synchronously' _might_ show up in AddRef/Release performance (that happens to be extra-hyper-super hot code).

Comment 2

[Andrew McCreight [:mccr8]]

The fast path would just be a branch which doesn't seem too bad.

Comment 3

[Nicholas Nethercote [inactive]]

Yeah, ~4 wakeups/s continually for every process isn't great.

Comment 4

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

It shouldn't be continuous if there isn't any new possible garbage. But sure, need to optimize this.

Comment 5

[Andrew McCreight [:mccr8]]

We could do something like, run the first 5 slices that do some cleanup as usual. Then, if we meet the CC conditions already maybe continue the timer as usual. But if not, then we just kill the timer, and wait for the inputs to ShouldTriggerCC() to change in a way that would alter its return value from false to true, then set up a timer, based on how long it has been.

Olli, do you have any kind of time line (no pun intended) for your collector timeline thing? If that's going to be able to land in the near term it would make sense to do any inversions like I describe on top of that. (Plus we probably don't want to change too much in a single release.)

Comment 6

[Andrew McCreight [:mccr8]]

There's a number of issues here that can be improved.

- Much of the time, CCTimerFired does nothing. I think we can do this by eliminating the timer fires in between those at the start, and those at the end, and instead add some kind of poke forget skippable in the purple buffer.

- When CCTimerFired does something, mostly it is running ForgetSkippable. When I have loaded BuzzFeed, then the ForgetSkippable calls that aren't in that initial cleanup barely take any time at all, around 0.4ms. We can reduce the number of these calls by increasing the purple buffer change threshold. In local testing, increasing it to 1000 (from 100) seems to eliminate a lot of the timer firings, without increasing the pauses much.

- The early ForgetSkippable calls do some potentially expensive work. When I start up the browser, the first ones take a few milliseconds each, which is good. However, when the page is idle, each of these is only taking about 0.3ms to run. We should run more of the early cleanup phases, until we hit some kind of time budget. When they are fast (like 0.3ms), they could potentially be done in a single timer wakeup. The main difficulty here is that the nsCCUncollectableMarker needs to communicate back to nsJSEnvironment that it is done running the early cleanup phases, but I guess I can just add a callback on nsJSEnvironment.

Comment 7

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

Whatever I'll do with scheduler, increasing threshold from 100 to 1000 sounds good to me.
Need to check the blame why we have 100. ... looking that.


Early FSs can be very expensive, like 5ms, so need to be careful with them. But I guess in cases some
early FS takes < 1ms we could merge the next slice to it.

Comment 8

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

The magical 100 comes from https://bugzilla.mozilla.org/show_bug.cgi?id=721543.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: Don't do incremental CC prep.

This patch should disable all incremental CC preparation, if you want to try it Nick. I loaded BuzzFeed with this patch applied, in an opt build, and it didn't seem to have a noticable impact on the "Energy Impact" reported by OSX, but maybe a finer grained thing would detect some improvement. We wouldn't want to actually land this, but it should represent an upper bound on how much we could save from improving this.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: Don't run as many timers to do CC cleanup. WIP

Here's what I have. It seems to mostly work, though I still need to implement an additional timer to handle lockouts. It is more complex in some ways than what we have, but it is also a little simpler in that it does not mash together a bunch of unrelated conditions.

Comment 11

[Andrew McCreight [:mccr8]]

Running locally with BuzzFeed, I'm getting around 8.6 W output from rapl on trunk, and something like 8.15 W with my patch that totally removes incremental cleanup (attachment 8653694 [details] [diff] [review]). My other actual patch doesn't seem to improve things much over trunk, though it only addresses the first two parts of comment 6.

Comment 12

[Andrew McCreight [:mccr8]]

I was getting about 4 W without the browser running at all, so not doing incremental cleanup is like a 10% improvement, which is pretty decent.

Comment 13

[Nicholas Nethercote [inactive]]

> This patch should disable all incremental CC preparation, if you want to try
> it Nick. I loaded BuzzFeed with this patch applied, in an opt build, and it
> didn't seem to have a noticable impact on the "Energy Impact" reported by
> OSX, but maybe a finer grained thing would detect some improvement. We
> wouldn't want to actually land this, but it should represent an upper bound
> on how much we could save from improving this.

I don't see a noticeable change in power consumption according to rapl. (I get ~22 W which is a lot less than you. Do you have e10s disabled?) But I do see the wake-up frequency drop in the child process by about 8--10 per second (which is probably higher than expected since the timeout it 250 ms?)

BTW, don't use "Energy Impact". Instead look at CPU% or in this case, idle wake-ups. If you ctrl-click on the column headers in the "Energy" tab you can select CPU% and wake-ups, and it can be nicer to watch them in the "Energy" tab rather than the "CPU" tab because the entries don't jump around.

Comment 14

[Nicholas Nethercote [inactive]]

> I don't see a noticeable change in power consumption according to rapl.

But the number of CCTimerFired events drops from 309 to 11 when I start the browser on BuzzFeed and let it sit for 60 seconds. So that's good.

Comment 15

[Andrew McCreight [:mccr8]]

This code is still a giant mess of timers, but I think it wasn't a huge deal relative to layout and graphics stuff.

Comment 16

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

I think we managed to reduce wakeups quite a bit in bug 1548358.