866681 - Make ContentUnbinder use DeferredFinalize

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Some classes, like DOM trees and nsCSSStyleSheetInner, can take a long time to destroy.  Each class like this could just implement its own lazy content blaster, but there is some overlap in how high level scheduling gets implemented, and we don't really want every instance of this firing off its own random runnables, causing extra jankiness while the CC and other of these delayed destroyers are running.

Instead, we can define a DelayedDestroyer class with two methods:
  DestroySome(uint32_t aSliceBudget);
  DestroyAll();

Classes would register a DelayedDestroyer with the CC, which would keep a list of them, and start processing them in its work slices, once unrooting was complete.

Classes like the Lazy Content Blaster that want to put a lot of stuff into each destroyer could maintain a weak pointer to their most recent destroyer (cleared in the destructor).

One question would be how to schedule work when destroyers are registered while the CC isn't running.

Comment 1

[Andrew McCreight [:mccr8]]

This is only useful once unlinking is incremental.

Comment 2

[Andrew McCreight [:mccr8]]

The lazy rule destroyer looks to be useful even without incremental unlinking.

IncrementalFinalizeRunnable is a similar existing class, as is ContentUnbinder.  I'm not sure if we have any others.  They should all at least share the same implementation, even if they can't share the exact same instance (for instance, IFR may want to key off the start and end of the GC, rather than CC).

Comment 3

[Andrew McCreight [:mccr8]]

I think the right thing to do here is to make IncrementalFinalizeRunnable the one true way to do incremental destruction of stuff.  I have a patch in progress to make ContentUnbinder use it.

Comment 4

[Andrew McCreight [:mccr8]]

Part of this will involve things like flushing the DeferredFinalize stuff at the start of a CC.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Make ContentUnbinder use a SegmentedVector. WIP

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Delete trailing whitespace from FragmentOrElement.

try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cd1f9777841e

I'll attach the real meat of things for review once the SegmentedVector API stuff is reviewed.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8540735 [details] [diff] [review]
part 1 - Delete trailing whitespace from FragmentOrElement.

Well, I want to think a little more about my whole approach here, so I'll just cancel the review for now.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: Make ContentUnbinder use DeferredFinalize. WIP

Here's a new version of this that does not use a segmented vector.  I did some local testing with techcrunch page close, and it didn't have more than about 150 items in the array, so maybe segmentation isn't needed?

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Remove trailing whitespace from FragmentOrElement.

This uses a SegmentedVector.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebb37ca861ca

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Track if any slice of CC was manually triggered.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Make ContentUnbinder use DeferredFinalize.

This also makes DeferredFinalize be synchronously finished off at the start of a CC, and started up at the end of the CC.

Comment 12

[Andrew McCreight [:mccr8]]

I'm hitting some weird reentrance assertion in the GC:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3135008e064

I could reproduce locally by closing the browser with a page open.

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 8593633 [details] [diff] [review]
part 1 - Remove trailing whitespace from FragmentOrElement.

This landed separately.

Comment 14

[Andrew McCreight [:mccr8]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=235585197a3b

dt1 is failed 100% of the time on this push, but it is almost permafail on inbound, so I choose to believe it is not my fault.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Track if any slice of CC was manually triggered.

We need to know this so we can decide at the end of the CC if we should dispatch deferred things immediately, or let them run incrementally.

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make ContentUnbinder use DeferredFinalize.

This patch makes ContentUnbinder use the standard DeferredFinalize mechanism. The actual content things are stored in a segmented vector.

This does slightly change the behavior of the content unbinder. We no longer clear it in xpcom-shutdown, or in cycle-collector-begin. Instead, we will only clear it at the end of GC, which is where it is cleared right now.  I tried to preserve that behavior, but it was causing some weird reentrance assertion inside the GC that I couldn't figure out.  I think it should be okay because we GC so much.

At the end of a CC we start up incremental deferred finalize destruction, incrementally if all slices of the CC were incremental, or synchronously otherwise, so that at shutdown and for forced CCs we destroy things immediately. This part unfortunately requires making the FinalizeDeferredThings stuff public. Alternatively, that could be moved inside EndCycleCollectionCallback if you think that is better, though that requires a little more code shuffling.

Comment 17

[Andrew McCreight [:mccr8]]

Oh, another minor change is that the content unbinder starts at the end of the CC rather than during unlinking.

Comment 18

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

Comment on attachment 8604813 [details] [diff] [review]
part 2 - Make ContentUnbinder use DeferredFinalize.


>+  static bool
>+  DeferredFinalize(uint32_t aSlice, void* aData)
Could you rename aSlice to aSliceBudget or some such

>+    if (contentArray->Length() == 0) {
>+      delete contentArray;
Urm. Somewhat ugly to see manual delete, but I guess we have no other options here.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e75b3eb23f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fcf6bf43eda

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8e75b3eb23f4
https://hg.mozilla.org/mozilla-central/rev/7fcf6bf43eda

Comment 21

[Andrew McCreight [:mccr8]]

This was mostly backed out in bug 1177627 (but not entirely which is why I'm not reopening...).