713462 - Investigate if black content lists don't need to be traversed

Status: RESOLVED FIXED

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

Description

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

Attachment: WIP

https://tbpl.mozilla.org/?tree=Try&rev=52a864233ed5

Comment 1

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

IIRC, Andrew saw content list keeping reasonable huge part of CC graph alive in some cases.

Comment 2

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I didn't see it that much.  If I am remembering correctly, it was basically a problem where a list was holding a ton of DOM nodes in a live document, so the DOM nodes were being added to the graph for no particular reason.

Comment 3

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

Ok. Though, tbpl does seem to create lots of small black content lists.

Comment 4

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I was just looking at a few simple-ish webpages.

Comment 5

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Comment on attachment 584282 [details] [diff] [review]
WIP

Review of attachment 584282 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=me with comment added somewhere about why it is okay to skip wrapper traversal.

::: content/base/src/nsContentList.cpp
@@ +79,5 @@
>    NS_IMPL_CYCLE_COLLECTION_UNLINK_NSTARRAY(mElements)
>    NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsBaseContentList)
> +  if (nsCCUncollectableMarker::sGeneration && tmp->IsBlack()) {

This violates the usual rule of always traverse any held script objects.  It is okay here because we are explicitly checking that the wrapper is black, and the only held script object is black, and we don't traverse black objects.  If TRACE traversed some other object, we'd have to change TRAVERSE.  This is a subtle enough thing I think there should be a comment in here somewhere about it.

This will also change the behavior of WANT_ALL_TRACES, because in that mode we traverse through black objects, but I don't think that matters much.  We don't normally disabled uncollectablemarker optimizations in that mode, but this one is also a little odd because it affects traversal of JS, whereas mostly I think that just affects C++.

Comment 6

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

Actually, I'll change the code so that we traverse script objects always, but not mElements.
That way the code is consistent with DOM nodes.

Comment 7

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

Attachment: patch

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/1d0a814ebf12

Comment 9

[Andrew McCreight [:mccr8] (PTO until 12-2)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Actually, I'll change the code so that we traverse script objects always,
> but not mElements.
> That way the code is consistent with DOM nodes.

Sounds good to me.