Last Comment Bug 713462 - Investigate if black content lists don't need to be traversed
: Investigate if black content lists don't need to be traversed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on:
Blocks: 716598
  Show dependency treegraph
 
Reported: 2011-12-25 11:17 PST by Olli Pettay [:smaug]
Modified: 2012-01-09 10:42 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (4.62 KB, patch)
2011-12-25 11:17 PST, Olli Pettay [:smaug]
continuation: review+
Details | Diff | Splinter Review
patch (4.80 KB, patch)
2011-12-29 13:51 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Comment 1 Olli Pettay [:smaug] 2011-12-25 19:38:44 PST
IIRC, Andrew saw content list keeping reasonable huge part of CC graph alive in some cases.
Comment 2 Andrew McCreight [:mccr8] 2011-12-26 19:12:44 PST
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] 2011-12-27 01:27:54 PST
Ok. Though, tbpl does seem to create lots of small black content lists.
Comment 4 Andrew McCreight [:mccr8] 2011-12-27 03:14:29 PST
I was just looking at a few simple-ish webpages.
Comment 5 Andrew McCreight [:mccr8] 2011-12-29 12:27:19 PST
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] 2011-12-29 13:19:17 PST
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] 2011-12-29 13:51:15 PST
Created attachment 584839 [details] [diff] [review]
patch
Comment 8 Olli Pettay [:smaug] 2011-12-29 13:58:29 PST
https://hg.mozilla.org/mozilla-central/rev/1d0a814ebf12
Comment 9 Andrew McCreight [:mccr8] 2011-12-29 16:14:35 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.