Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla12

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 1 bug)

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
Created attachment 584282 [details] [diff] [review]
WIP

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

Comment 1

6 years ago
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.
(Assignee)

Comment 3

6 years ago
Ok. Though, tbpl does seem to create lots of small black content lists.
I was just looking at a few simple-ish webpages.
(Assignee)

Updated

6 years ago
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
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
Created attachment 584839 [details] [diff] [review]
patch
Assignee: nobody → bugs
(Assignee)

Comment 8

6 years ago
https://hg.mozilla.org/mozilla-central/rev/1d0a814ebf12
Status: NEW → RESOLVED
Last Resolved: 6 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

Updated

6 years ago
Target Milestone: --- → mozilla12
Version: 10 Branch → Trunk
Blocks: 716598
No longer blocks: 698919
You need to log in before you can comment on or make changes to this bug.