Closed
Bug 977940
Opened 11 years ago
Closed 11 years ago
Don't run the ghost window detector so much
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(2 files)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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.)
Assignee | ||
Comment 2•11 years ago
|
||
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.
Assignee | ||
Comment 3•11 years ago
|
||
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.
Attachment #8387750 -
Flags: review?(bugs)
Assignee | ||
Comment 4•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Attachment #8387752 -
Flags: review?(bugs)
Comment 5•11 years ago
|
||
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.
Attachment #8387750 -
Flags: review?(bugs) → review+
Comment 6•11 years ago
|
||
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.
Attachment #8387752 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 7•11 years ago
|
||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f1106ddac0b5
https://hg.mozilla.org/mozilla-central/rev/32504c2466e3
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•