977940 - Don't run the ghost window detector so much

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

Whenever nsWindowMemoryReporter observes DOM_WINDOW_DESTROYED_TOPIC, it schedules a runnable to call CheckForGhostWindows() (if none has been scheduled yet).  This is probably okay for normal usage, but in mochitest browser-chrome this gets triggered constantly. In local measurement, it can easily run 6 times a second, sometimes less than 0.1 seconds apart.  That seems like a waste of time, so I was thinking I could add some kind of timer delay of 10 or so seconds before actually scheduling the runnable.

This causes huge problems for ICC, because when CheckForGhostWindows() runs, it touches every window (or something like that), and when an otherwise-garbage window is touched while ICC is running it will be added to the graph, but not freed, so you end up with tons of useless stuff in the graph, making it much slower.

I'll probably want to set it up so the CheckForGhostWindows() runnable never triggers during ICC, but I think spacing them out is a good idea in any event.

Comment 1

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

It is not ok to run such checker all the time.
(I'm very surprised that we seem to do it so often, even outside testing.)

Comment 2

[Andrew McCreight [:mccr8]]

For the record, it looks like it takes a few milliseconds to run the ghost window detector, which, at 5 times a second, isn't the end of the world, but it isn't that great either.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Don't run the ghost window detector more than every 45 seconds.

I'm not sure who a good reviewer for this, but the style of this is like the GC/CC timers, so I'll ask smaug. ;)

The idea here is that when a DOM window closes, we want to run the detector almost right away, if we haven't run it in the last 45 seconds.  If we have run it recently, then we want to run it again, around 45 seconds after the last time we ran it.

For this patch in isolation, I could just make it so that the time takes 45 seconds or something, but in the next patch I kill off the detector timer any time we start a CC, so we'd end up starving the ghost window detector if we didn't measure the time since we last ran it, as the CC runs much more frequently.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Don't automatically trigger the ghost window detector during ICC.

This patch adds a new observer event thing for the end of the CC.  The ghost window detector listens for the start and end of a CC, and locally tracks the CC's state. (Technically, we only care about the graph building part of CC, but I just track the whole of a CC for simplicity.)

When a CC starts, we kill the watch timer, if it there is one. If we had a timer going when we started the CC, or we tried to start a watch timer during the CC, then we set a flag indicating that. When the CC is over, if that flag is set, then we spawn a new watch timer.

try run: https://tbpl.mozilla.org/?tree=Try&rev=de836775384d

Comment 5

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

Comment on attachment 8387750 [details] [diff] [review]
part 1 - Don't run the ghost window detector more than every 45 seconds.

>+nsWindowMemoryReporter::CheckTimerFired(nsITimer *aTimer, void *aClosure)
* goes with the type

>+  int32_t timeSinceLastCheck = (TimeStamp::NowLoRes() - mLastCheckForGhostWindows).ToSeconds();
>+  int32_t timerDelay = (kTimeBetweenChecks - std::min(timeSinceLastCheck, kTimeBetweenChecks)) * PR_MSEC_PER_SEC;
A comment would be nice here explaining what kind of behavior is expected.

Comment 6

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

Comment on attachment 8387752 [details] [diff] [review]
part 2 - Don't automatically trigger the ghost window detector during ICC.

I guess cycle-collector-end can be useful for some tools too.

Comment 7

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f1106ddac0b5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/32504c2466e3

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f1106ddac0b5
https://hg.mozilla.org/mozilla-central/rev/32504c2466e3