1179894 - Optimize ghost windows out of the CC graph

Status: NEW

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

Description

[Andrew McCreight [:mccr8]]

Ghost windows cause horrible performance problems due to the CC, and probably are not going to go away, so we should just optimize them out of the CC graph. We may want to add them back to the graph occasionally, say once every 10 minutes, in case the window really did go away.

Comment 1

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

This is a dup of some old bug, which never got fixed since we in general have had ghost windows somewhat rarely. Lately couple of regressions though.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Optimize ghost windows out of the CC graph. WIP

Also, remove a redundant null check of |window| and move a comment.

MozReview-Commit-ID: 6E8J4f5l7Q5

Comment 4

[Andrew McCreight [:mccr8]]

Here's an untested patch. It adds a flag to inner windows that indicate if the ghost window detector thought it was a ghost window the last time it ran. Then it adds code to nsCCUncollectableMarker::Observe() that marks any windows that have the ghost window flag set as uncollectable. I don't know if that's enough to clear out the CC graph to a reasonable extent.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1179894, part 1 - Split out MarkDocument for easier reuse.

Review commit: https://reviewboard.mozilla.org/r/211536/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211536/

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Bug 1179894, part 2 - Optimize ghost windows out of the CC graph.

Ghost windows cause the cycle collector to get very slow. This patch
fixes that by marking ghost windows as uncollectable. This means we
will leak them forever. Every 30 or so CCs, we skip that optimization,
in the hope that sometimes that will let us free the window.

Also, remove a redundant null check of |window| and move a comment.

Review commit: https://reviewboard.mozilla.org/r/211534/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211534/

Comment 7

[Andrew McCreight [:mccr8]]

I tested this by making all nsGlobalWindowInners leak forever, opening the HTML5 spec, then closing the tab. My earlier version had a minor bug, plus I changed it to mark the document in the ghost window, rather than the window. I think that's better, though I guess we could leak an inner window without a document.

You can see here what happens when the ghost window detector notices that the spec page is leaking:
CC(T+97.5)[content-21748] max pause: 49ms, total time: 241ms, slices: 6, suspected: 3, visited: 352061 RCed and 11631 GCed, collected: 0 RCed and 0 GCed (0|0|0 waiting for GC)
CC(T+157.3)[content-21748] max pause: 4ms, total time: 4ms, slices: 1, suspected: 11, visited: 196 RCed and 4703 GCed, collected: 0 RCed and 0 GCed (0|0|0 waiting for GC)

The total time goes from 241ms to 4ms.

Comment 8

[Andrew McCreight [:mccr8]]

My patches here cause intermittent leaks in the bc mochitests:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7af94973ff5c5bd322b0dc00043421c906c49ced

The bc leak detector forces GCs and CCs, which I think makes things get marked as a ghost faster than they would be normally, so maybe that interferes with things. It is also possible that there are similar problems in other test suites, but they don't show up as leaks because we don't run these optimizations on shutdown, so they won't show up in the normal leak detector.

Comment 9

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

perhaps numGhostSkips should be based on timestamp and not counter?