Closed Bug 713462 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

IIRC, Andrew saw content list keeping reasonable huge part of CC graph alive in some cases.
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.
Ok. Though, tbpl does seem to create lots of small black content lists.
I was just looking at a few simple-ish webpages.
Attachment #584282 - Flags: review?(continuation)
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++.
Attachment #584282 - Flags: review?(continuation) → review+
OS: Linux → All
Hardware: x86_64 → All
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.
Attached patch patchSplinter Review
Assignee: nobody → bugs
https://hg.mozilla.org/mozilla-central/rev/1d0a814ebf12
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(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.
Blocks: 698919
Target Milestone: --- → mozilla12
Version: 10 Branch → Trunk
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.