721515 - Add Documents, elements and textnodes to BBP

Status: RESOLVED FIXED

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

Description

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

Attachment: patch

- CanSkip and CanSkipCC are similar but not similar enough to have them
  combined.

- UnoptimizableCCNode is used also in nsINode::Traverse where is does few extra checks
  for parent but those are non-virtual and even inline, so I prefer to just keep
  the code simpler.

- MarkUserData and MarkUserData will be used also in another patch

I assume this patch may cause some questions, although it shouldn't be that
difficult to understand.

Comment 1

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

Attachment: patch

oops, I had managed to remove rather important part in nsContentUtils.cpp

Comment 2

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

Attachment: tiny bit cleaner

Comment 3

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

Comment on attachment 592102 [details] [diff] [review]
tiny bit cleaner

This patch should get a review from a DOM peer too.

I'm trying to be conservative when handling anon content.

I've tried to merge CanSkip and CanSkipInCC, but merging them
seems to make the code harder to understand, since those
method do quite different things after all.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 592102 [details] [diff] [review]
tiny bit cleaner

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

::: content/base/src/nsGenericElement.cpp
@@ +1217,5 @@
> +                                    NODE_IS_NATIVE_ANONYMOUS_ROOT |
> +                                    NODE_MAY_BE_IN_BINDING_MNGR |
> +                                    NODE_IS_INSERTION_PARENT);
> +  return aNode->HasFlag(problematicFlags) ||
> +         aNode->NodeType() == nsIDOMNode::ATTRIBUTE_NODE ||

Why do you need this ATTRIBUTE_NODE thing here you didn't have before?

@@ +4265,5 @@
> +  if (aNode->PreservingWrapper()) {
> +    o = aNode->GetWrapperPreserveColor();
> +  } else {
> +    o = aNode->GetExpandoObjectPreserveColor();
> +  }

Could you make a function that takes an nsINode*, then looks for a JSObject* in this way and returns it?  You use this same basic pattern 3 times, in NeedsScriptTraverse, ClearNode and NodeToClear.

@@ +4287,5 @@
> +  xpc_UnmarkGrayObject(wjs);
> +}
> +
> +static void
> +ClearNode(nsINode* aNode)

ClearNode is kind of a generic name.  Maybe MarkNodeChildren?  Or SetNodeChildrenLive?  You aren't really doing anything to the node itself, but to the things it holds onto.

@@ +4316,5 @@
> +
> +nsAutoTArray<nsINode*, 1020>* gCCBlackMarkedNodes = nsnull;
> +
> +void
> +BeforeUnlinkCallback()

A more descriptive name would be useful here.  Maybe something like ClearBlackMarkedNodes or something.

@@ +4336,5 @@
> +bool
> +nsGenericElement::CanSkipInCC(nsINode* aNode)
> +{
> +  // Don't try to optimize anything during shutdown.
> +  if (nsCCUncollectableMarker::sGeneration == 0) {

Should this be |if (!nsCCUncollectableMarker::sGeneration)|?  I wonder if we should do this check in the cycle collector, and just not call any of the skip functions if it holds.  Something to consider for later.

@@ +4353,5 @@
> +    return !NeedsScriptTraverse(aNode);
> +  }
> +
> +  // Find subtree root.
> +  nsINode* root = currentDoc ? static_cast<nsINode*>(currentDoc) : aNode;

Is the currentDoc ever going to have a node parent?

@@ +4365,5 @@
> +
> +  if (UnoptimizableCCNode(root)) {
> +    return false;
> +  }
> +  

I'll keep going from here tomorrow.  I have no more comments for the other files.

::: content/base/src/nsGenericElement.h
@@ +624,5 @@
> +  }
> +
> +  static bool CanSkip(nsINode* aNode);
> +  static bool CanSkipInCC(nsINode* aNode);
> +  static bool CanSkipThis(nsINode* aNode);

It is odd that these are in nsGenericElement, but take nsINode as arguments.  Is there some reason for that?

@@ +626,5 @@
> +  static bool CanSkip(nsINode* aNode);
> +  static bool CanSkipInCC(nsINode* aNode);
> +  static bool CanSkipThis(nsINode* aNode);
> +  static void InitCCCallbacks();
> +  static void MarkUserData(void* aObject, nsIAtom* aKey, void* aChild,

These two Mark functions take arguments they don't use.  I know this is because they are intended to be used with a hash table iterator, but theirs no real indication from the name that this is the case.  It also seems a little dangerous to have cross-file functions like this with so many void* arguments.

Comment 5

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

(In reply to Andrew McCreight [:mccr8] from comment #4)
> @@ +1217,5 @@
> > +                                    NODE_IS_NATIVE_ANONYMOUS_ROOT |
> > +                                    NODE_MAY_BE_IN_BINDING_MNGR |
> > +                                    NODE_IS_INSERTION_PARENT);
> > +  return aNode->HasFlag(problematicFlags) ||
> > +         aNode->NodeType() == nsIDOMNode::ATTRIBUTE_NODE ||
> 
> Why do you need this ATTRIBUTE_NODE thing here you didn't have before?
I made it more generic. The code in CanSkip can't really handle 
Attr nodes (which are going away soon).



> Could you make a function that takes an nsINode*, then looks for a JSObject*
> in this way and returns it?  You use this same basic pattern 3 times, in
> NeedsScriptTraverse, ClearNode and NodeToClear.
Yeah, I was thinking that too.
I'll add it.
JSObject* GetJSObject(nsINode* aNode);


> 
> ClearNode is kind of a generic name.  Maybe MarkNodeChildren?
MarkNodeChildren sounds ok


> > +BeforeUnlinkCallback()
> 
> A more descriptive name would be useful here.  Maybe something like
> ClearBlackMarkedNodes or something.
Ok, can change.



> Should this be |if (!nsCCUncollectableMarker::sGeneration)|?
I don't care whether to check == 0 or !
Just curious, why would !nsCCUncollectableMarker::sGeneration be better?

> I wonder if we
> should do this check in the cycle collector, and just not call any of the
> skip functions if it holds.  Something to consider for later.
CC doesn't know about generations atm.

> > +  // Find subtree root.
> > +  nsINode* root = currentDoc ? static_cast<nsINode*>(currentDoc) : aNode;
> 
> Is the currentDoc ever going to have a node parent?
No. But the code is just checking using GetNodeParent which is inline non-virtual call,
so the next while loop is just skipped in that case.


> It is odd that these are in nsGenericElement, but take nsINode as arguments.
> Is there some reason for that?
Because they are used by nsGenericDOMDataNode and nsDocument too.

nsGenericElement has been just a place where we've collected some generic nsINode* related
static methods. I don't want to add static CanSkip methods to nsINode, since that is a
higher level interface.


> > +  static void MarkUserData(void* aObject, nsIAtom* aKey, void* aChild,
> 
> These two Mark functions take arguments they don't use.  I know this is
> because they are intended to be used with a hash table iterator, but theirs
> no real indication from the name that this is the case.  It also seems a
> little dangerous to have cross-file functions like this with so many void*
> arguments.
Well, I'm just following the way we handle user data elsewhere. It is a bit ugly.
See for example nsNodeUtils. I blame the strange nsPropertyTable data structure ;)

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #5)
> > Could you make a function that takes an nsINode*, then looks for a JSObject*
> > in this way and returns it?  You use this same basic pattern 3 times, in
> > NeedsScriptTraverse, ClearNode and NodeToClear.
> Yeah, I was thinking that too.
> I'll add it.
> JSObject* GetJSObject(nsINode* aNode);
Okay, great!  Maybe call it something like GetJSObjectChild or GetJSChild to indicate what you are geting?

> > Should this be |if (!nsCCUncollectableMarker::sGeneration)|?
> I don't care whether to check == 0 or !
> Just curious, why would !nsCCUncollectableMarker::sGeneration be better?
Wait, ignore me.  I was misremembering the style guide.

> > I wonder if we
> > should do this check in the cycle collector, and just not call any of the
> > skip functions if it holds.  Something to consider for later.
> CC doesn't know about generations atm.
Yeah.  It could also maybe just listen for XPCOM shutdown, and flip a flag on itself to indicate that it should not optimize any more.

> > Is the currentDoc ever going to have a node parent?
> No. But the code is just checking using GetNodeParent which is inline
> non-virtual call,
> so the next while loop is just skipped in that case.
Ah, ok.

> nsGenericElement has been just a place where we've collected some generic
> nsINode* related
> static methods. I don't want to add static CanSkip methods to nsINode, since
> that is a
> higher level interface.
Make sense!

> > > +  static void MarkUserData(void* aObject, nsIAtom* aKey, void* aChild,
> > 
> > These two Mark functions take arguments they don't use.  I know this is
> > because they are intended to be used with a hash table iterator, but theirs
> > no real indication from the name that this is the case.  It also seems a
> > little dangerous to have cross-file functions like this with so many void*
> > arguments.
> Well, I'm just following the way we handle user data elsewhere. It is a bit
> ugly.
> See for example nsNodeUtils. I blame the strange nsPropertyTable data
> structure ;)

Right, but in that case NoteUserData is local to nsNodeUtils.cpp.

Comment 7

[Andrew McCreight [:mccr8]]

So, I think it would be better if you made a function like

MarkAllUserDataHandler (nsDocument *doc) {
  doc->PropertyTable(DOM_USER_DATA_HANDLER)->
		EnumerateAll(MarkUserDataHandler,
                &nsCCUncollectableMarker::sGeneration);
}

and then a similar one MarkAllUserData(nsDocument *doc), in nsGenericElement.cpp.  Put both in the header.  Then remove MarkUserDataHandler and MarkUserData from the header and make them static.  Then the weird hash table functions wouldn't be exposed to the world.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 592102 [details] [diff] [review]
tiny bit cleaner

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

Part 2.  Just some odds and ends.  I'm really starting to dig into CanSkipInCC and CanSkip now.

::: content/base/src/nsGenericElement.cpp
@@ +1227,5 @@
>  /* static */
>  bool
>  nsINode::Traverse(nsINode *tmp, nsCycleCollectionTraversalCallback &cb)
>  {
>    nsIDocument *currentDoc = tmp->GetCurrentDoc();

I feel like that for any class with a real CanSkipThis we should be able to tear this optimization code out of Traverse.  Something to think about for a future patch.

@@ +4258,5 @@
>    nsINode::Trace(tmp, aCallback, aClosure);
>  NS_IMPL_CYCLE_COLLECTION_TRACE_END
>  
> +static bool
> +NeedsScriptTraverse(nsINode* aNode)

Could nsWrapperCache::IsBlack do the same thing?  EG also check if there's a non-gray expando, in addition to checking if there's a non-gray wrapper.  It seems safe to not return true sometimes when we could, but maybe it would help some?  I'm sure what the difference is between the two kinds of wrapper caches, I'm just noticing some inconsistency in the code.

@@ +4613,5 @@
> +  nsIDocument* c = aNode->GetCurrentDoc();
> +  if (((c &&
> +        nsCCUncollectableMarker::InGeneration(c->GetMarkedCCGeneration())) ||
> +        aNode->InCCBlackTree()) &&
> +      !NeedsScriptTraverse(aNode)) {

You could just return the result of this expression instead of returning true when it is true and false when it is false.

Comment 9

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

(In reply to Andrew McCreight [:mccr8] from comment #7)
> So, I think it would be better if you made a function like
> 
> MarkAllUserDataHandler (nsDocument *doc) {
>   doc->PropertyTable(DOM_USER_DATA_HANDLER)->
> 		EnumerateAll(MarkUserDataHandler,
>                 &nsCCUncollectableMarker::sGeneration);
> }
> 
> and then a similar one MarkAllUserData(nsDocument *doc), in
> nsGenericElement.cpp.  Put both in the header.  Then remove
> MarkUserDataHandler and MarkUserData from the header
They need to be in the header since nsCCUncollectableMarker uses them too.
Though, I think I don't quite understand what you mean here.

 and make them static. 
> Then the weird hash table functions wouldn't be exposed to the world.

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #9)
> They need to be in the header since nsCCUncollectableMarker uses them too.
> Though, I think I don't quite understand what you mean here.

I meant that you should replace the uses of MarkUserDataHandler and MarkUserData in nsCCUncollectableMarker with MarkAllUserData and MarkAllUserDataHandlers.  If that makes any more sense?  Then you won't need to put MarkAllUserData{Handlers} in the header.

Comment 11

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

(In reply to Andrew McCreight [:mccr8] from comment #8)
> @@ +4258,5 @@
> >    nsINode::Trace(tmp, aCallback, aClosure);
> >  NS_IMPL_CYCLE_COLLECTION_TRACE_END
> >  
> > +static bool
> > +NeedsScriptTraverse(nsINode* aNode)
> 
> Could nsWrapperCache::IsBlack do the same thing?  EG also check if there's a
> non-gray expando,
Well, expando doesn't own the node, so IsBlack doesn't make sense there.


> > +  nsIDocument* c = aNode->GetCurrentDoc();
> > +  if (((c &&
> > +        nsCCUncollectableMarker::InGeneration(c->GetMarkedCCGeneration())) ||
> > +        aNode->InCCBlackTree()) &&
> > +      !NeedsScriptTraverse(aNode)) {
> 
> You could just return the result of this expression instead of returning
> true when it is true and false when it is false.
ok.

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #11)
> > Could nsWrapperCache::IsBlack do the same thing?  EG also check if there's a
> > non-gray expando,
> Well, expando doesn't own the node, so IsBlack doesn't make sense there.

Ahh, I see.  Thanks.

Comment 13

[Andrew McCreight [:mccr8]]

Here's my psuedocode for what CanSkipInCC:

CanSkipInCC
 false if (gen == 0) or it is a weird node
 true if in a live doc, doesn’t need script traverse

 find root of our tree, via GetNodeParent()
     false if you ever encounter a weird node during this
 if root is marked
     true if root is black and doesn’t need script traverse (otherwise false)

 if no global set of CC marked nodes, make one
 nodesToUnpurple: in tree, purple, don’t need script traverse.
 grayNodes: in tree, need script traverse

 set black to blackness of root
 if we’re an orphan node(?), Visit(static_cast<nsIContent*>(root))
 
 iterate over all children |node| of root
    accumulate node blackness into black
    if we found a black node, and we’re in a document,
       bail out early, because we can just mark the doc
    Visit(node)

 mark that we’ve explored root
 mark on root if we found a black node
 add root to global set of marked nodes

 if we didn’t find a black node, return false.
 if we’re in a doc

mark the doc in generation.
 else
    mark gray nodes as being in a black tree
    add gray nodes to the global set of marked nodes
 unpurple all nodes to unpurple
 in a bit of an anticlimax, return if the current node doesn’t need script traversal

Hopefully that is in the ballpark. I'll post comments once I read CanSkip and I make sure that doesn't change my interpretation of things.

Comment 14

[Andrew McCreight [:mccr8]]

I'll leave you with one high-level comment for now, so you can mull it over.  I notice that you don't unpurple NeedsScriptTraverse nodes, but that's actually totally okay!  They are going to be added to the graph anyway by BeginCycleCollection, because they are JS holders.

In fact, it is probably best to ALWAYS remove them, whether or not they are in live nodes, because that will slow purple buffer growth and delay CC further.

But I think it is 100% okay to leave them around, and it is a bit of a radical change, so I think we can just leave it like it is, and deal with it in a followup.  I just thought it was fun.

Comment 15

[Andrew McCreight [:mccr8]]

Here's my pseudocode for CanSkip:

during shutdown or if the node is weird, return false
If we’re in a live doc, ClearNode() then return true

Find the root as before, bailing if you hit a weird node.

If root was previously visited during a CanSkip, we tried and failed
 to remove this node from the purple buffer, so return false.

nodesToClear:

Check if root is black.
If this tree is an orphan(?):
    if node has children to clear, add it to nodesToClear.

iterate over all nodes in the subtree:
    accumulate blackness
    if we’ve found a black node so far:
        if there’s a doc, bail on loop because we can handle this quickly
        otherwise, unpurple and clear the node
    else:
        // may find out this subtree is black later
        if node has children to clear, add it to nodesToClear

if we didn’t find a black node:
    mark that we tried to clear the current node and failed
    remember that we marked the node, for clearing later
    return false
If there’s a real doc involved, mark it as being in current gen, clear it.
Clear and unpurple any interesting nodes we squirreled away during the loop.
return true


pseudocode for NodeToClear:
// basically, return true if node may contain a clearable child
if node is purple, return true
if node contains a pointer to a gray JSObject, return true
if node has a listener manager or properties, return true

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 592102 [details] [diff] [review]
tiny bit cleaner

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

Looks good.  Just some minor comments.  It was easier to understand CanSkip once I'd figured out the similar CanSkipInCC.

I guess contrary to what I'd assumed, it looks like you don't actually check for NeedsScriptTraverse in CanSkip, only in CanSkipInCC and CanSkipThis, so the improvements from removing them in CanSkipInCC make it even less critical.

::: content/base/public/nsINode.h
@@ +1222,5 @@
>      // that is in a Selection.
>      NodeIsCommonAncestorForRangeInSelection,
>      // Set if the node is a descendant of a node with the above bit set.
>      NodeIsDescendantOfCommonAncestorForRangeInSelection,
> +    // Set if BeforeTraverse check has been done for this subtree root.

Maybe this should be "the CanSkipInCC check" instead of "BeforeTraverse check"?

@@ +1227,5 @@
> +    NodeIsCCMarkedRoot,
> +    // Maybe set if this node is in black subtree.
> +    NodeIsCCBlackTree,
> +    // Maybe set if the node is a root of such subtree
> +    // which need to be kept in purple buffer.

Maybe something like "Maybe set if the node is a root of a subtree which needs to be kept in the purple buffer."  Or you could be more direct "Set if the CanSkip check has been done and failed."

::: content/base/src/nsGenericElement.cpp
@@ +4375,5 @@
> +  if (!gCCBlackMarkedNodes) {
> +    gCCBlackMarkedNodes = new nsAutoTArray<nsINode*, 1020>;
> +  }
> +
> +  // nodesToUnPurple contains nodes which will be removed

"which will be removed" -> "that will be removed", I think.

@@ +4376,5 @@
> +    gCCBlackMarkedNodes = new nsAutoTArray<nsINode*, 1020>;
> +  }
> +
> +  // nodesToUnPurple contains nodes which will be removed
> +  // from purple buffer if the DOM tree is black.

"from the purple buffer"

@@ +4377,5 @@
> +  }
> +
> +  // nodesToUnPurple contains nodes which will be removed
> +  // from purple buffer if the DOM tree is black.
> +  nsAutoTArray<nsIContent*, 1020> nodesToUnPurple;

Maybe name this nodesToUnpurple instead (lower case p)

@@ +4379,5 @@
> +  // nodesToUnPurple contains nodes which will be removed
> +  // from purple buffer if the DOM tree is black.
> +  nsAutoTArray<nsIContent*, 1020> nodesToUnPurple;
> +  // grayNodes need script traverse, so they aren't removed from
> +  // purple buffer, but are just marked to be in black subtree so that

"the purple buffer", maybe you can drop the "just".

@@ +4383,5 @@
> +  // purple buffer, but are just marked to be in black subtree so that
> +  // traverse is faster.
> +  nsAutoTArray<nsINode*, 1020> grayNodes;
> +
> +  bool black = root->IsBlack();

Something like |foundBlack| would be a more descriptive name than |black|.

@@ +4384,5 @@
> +  // traverse is faster.
> +  nsAutoTArray<nsINode*, 1020> grayNodes;
> +
> +  bool black = root->IsBlack();
> +  if (root != currentDoc) {

If currentDoc never has a parent, doesn't root != currentDoc imply that currentDoc is null?

@@ +4388,5 @@
> +  if (root != currentDoc) {
> +    currentDoc = nsnull;
> +    if (NeedsScriptTraverse(root)) {
> +      grayNodes.AppendElement(root);
> +    } if (static_cast<nsIContent*>(root)->IsPurple()) {

This should be |else if|, right?  It would probably be good to make a new function for the two places you use this code pattern, and call it here.  Maybe something like VisitNodeInCC or something?

@@ +4406,5 @@
> +      // so much, since when the next purple node in the document will be
> +      // handled, it is fast to check that currentDoc is in CCGeneration.
> +      break;
> +    }
> +    if (NeedsScriptTraverse(node)) {

here's the other chunk for VisitNodeInCC or whatever.

@@ +4418,5 @@
> +  root->SetCCMarkedRoot(true);
> +  root->SetInCCBlackTree(black);
> +  gCCBlackMarkedNodes->AppendElement(root);
> +
> +  if (black) {

An early return here if !black would be nicer.

@@ +4427,5 @@
> +        MarkUncollectableForCCGeneration(nsCCUncollectableMarker::sGeneration);
> +    } else {
> +      for (PRUint32 i = 0; i < grayNodes.Length(); ++i) {
> +        nsINode* node = grayNodes[i];
> +        node->SetInCCBlackTree(true);

I think the idea here is that the only way into a DOM is through the document at the top (which we handle separately, and block off quickly) or through a gray node, so if we mark a gray node black, then we won't traverse into the rest of the DOM, so we don't need to mark any of the other nodes in the DOM black?  Is that right?  If so maybe add a comment about that somewhere.

@@ +4436,5 @@
> +    // Subtree is black, we can remove non-gray purple nodes from
> +    // purple buffer.
> +    for (PRUint32 i = 0; i < nodesToUnPurple.Length(); ++i) {
> +      nsIContent* purple = nodesToUnPurple[i];
> +      // Can't remove currently handled purple buffer.

purple node, not purple buffer.

@@ +4448,5 @@
> +}
> +
> +nsAutoTArray<nsINode*, 1020>* gPurpleRoots = nsnull;
> +
> +void ForgetSkippableCallback()

A more descriptive name like ClearPurpleRoots would be good.

@@ +4513,5 @@
> +    ClearNode(aNode);
> +    return true;
> +  }
> +
> +  // Find subtree root.

This whole "find subtree root" could be factored out into a function that takes a node and returns the root, or NULL if you hit a weird node, or something like that, then used here and in CanSkipInCC.

@@ +4537,5 @@
> +  // nodesToClear contains nodes which are either purple or
> +  // gray.
> +  nsAutoTArray<nsIContent*, 1020> nodesToClear;
> +
> +  bool black = root->IsBlack();

As before, something like |foundBlack| would be a more descriptive name.

@@ +4565,5 @@
> +      if (node->IsPurple() && node != aNode) {
> +        node->RemovePurple();
> +      }
> +      ClearNode(node);
> +    } else if (NodeToClear(node)) {

Maybe add a comment to the effect that we may discover later that this subtree is black, so we'll want to clear any interesting nodes we skipped.

@@ +4570,5 @@
> +      nodesToClear.AppendElement(node);
> +    }
> +  }
> +
> +  if (black) {

As before, an early return on !black is probably better here.  Well, you have to do the SetIsPurpleStuff, but maybe even given that.

Comment 17

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

(In reply to Andrew McCreight [:mccr8] from comment #10)
> (In reply to Olli Pettay [:smaug] from comment #9)
> I meant that you should replace the uses of MarkUserDataHandler and
> MarkUserData in nsCCUncollectableMarker with MarkAllUserData and
> MarkAllUserDataHandlers.  If that makes any more sense?  Then you won't need
> to put MarkAllUserData{Handlers} in the header.
I still don't understand. Note that nsCCUncollectableMarker uses EnumerateAll and
nsGenericElement Enumerate

Comment 18

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

(In reply to Andrew McCreight [:mccr8] from comment #14)
> I'll leave you with one high-level comment for now, so you can mull it over.
> I notice that you don't unpurple NeedsScriptTraverse nodes, but that's
> actually totally okay!  They are going to be added to the graph anyway by
> BeginCycleCollection, because they are JS holders.
> 
> In fact, it is probably best to ALWAYS remove them, whether or not they are
> in live nodes, because that will slow purple buffer growth and delay CC
> further.
> 
> But I think it is 100% okay to leave them around, and it is a bit of a
> radical change, so I think we can just leave it like it is, and deal with it
> in a followup.  I just thought it was fun.

This is a case where I wanted to be conservative and not change the current behavior.

Comment 19

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

(In reply to Andrew McCreight [:mccr8] from comment #13)
>  iterate over all children |node| of root
iterate all descendants...

Comment 20

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

(In reply to Andrew McCreight [:mccr8] from comment #16)
> Maybe this should be "the CanSkipInCC check" instead of "BeforeTraverse
> check"?
Ah, leftover from an earlier version of the patch.

> > +    // Maybe set if the node is a root of such subtree
> > +    // which need to be kept in purple buffer.
> 
> Maybe something like "Maybe set if the node is a root of a subtree which
> needs to be kept in the purple buffer."
This is ok

> "which will be removed" -> "that will be removed", I think.
ok

> "from the purple buffer"
ok

> 
> @@ +4377,5 @@
> > +  }
> > +
> > +  // nodesToUnPurple contains nodes which will be removed
> > +  // from purple buffer if the DOM tree is black.
> > +  nsAutoTArray<nsIContent*, 1020> nodesToUnPurple;
> 
> Maybe name this nodesToUnpurple instead (lower case p)
ok

> 
> "the purple buffer", maybe you can drop the "just".
ok

> Something like |foundBlack| would be a more descriptive name than |black|.
ok
> > +  if (root != currentDoc) {
> 
> If currentDoc never has a parent, doesn't root != currentDoc imply that
> currentDoc is null?
In fact yes

> > +  if (root != currentDoc) {
> > +    currentDoc = nsnull;
> > +    if (NeedsScriptTraverse(root)) {
> > +      grayNodes.AppendElement(root);
> > +    } if (static_cast<nsIContent*>(root)->IsPurple()) {
> 
> This should be |else if|, right?
Uh, right. I change that while cleaning up the code...


>  It would probably be good to make a new
> function for the two places you use this code pattern, and call it here. 
> Maybe something like VisitNodeInCC or something?
I'm not sure that is really needed

> @@ +4427,5 @@
> > +        MarkUncollectableForCCGeneration(nsCCUncollectableMarker::sGeneration);
> > +    } else {
> > +      for (PRUint32 i = 0; i < grayNodes.Length(); ++i) {
> > +        nsINode* node = grayNodes[i];
> > +        node->SetInCCBlackTree(true);
> 
> I think the idea here is that the only way into a DOM is through the
> document at the top (which we handle separately, and block off quickly) or
> through a gray node, so if we mark a gray node black, then we won't traverse
> into the rest of the DOM, so we don't need to mark any of the other nodes in
> the DOM black?  Is that right?
Yes

>  If so maybe add a comment about that
> somewhere.
Well, there is already comment about that in the
  for (nsIContent* node = root->GetFirstChild(); node;
       node = node->GetNextNode(root)) 


> > +    // Subtree is black, we can remove non-gray purple nodes from
> > +    // purple buffer.
> > +    for (PRUint32 i = 0; i < nodesToUnPurple.Length(); ++i) {
> > +      nsIContent* purple = nodesToUnPurple[i];
> > +      // Can't remove currently handled purple buffer.
> 
> purple node, not purple buffer.
indeed

 
> A more descriptive name like ClearPurpleRoots would be good.
ok

> > +      if (node->IsPurple() && node != aNode) {
> > +        node->RemovePurple();
> > +      }
> > +      ClearNode(node);
> > +    } else if (NodeToClear(node)) {
> 
> Maybe add a comment to the effect that we may discover later that this
> subtree is black, so we'll want to clear any interesting nodes we skipped.
ok

Comment 21

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

Attachment: patch

Comment 22

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

Jonas, this is btw something I'd really like to get to FF12.
This patch blocks effectively rest of the cycle collection optimizations.

(Andrew, feel free to read through the updated patch.)

Comment 23

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

Or Jonas, if you don't have time to review this, could you please tell it asap, so that
I can beg some other DOM peer to look at this.
(Fortunately Andrew has been very careful with reviewing)

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There's a ton new terms here, do we have it documented anywhere what they all mean? That'd be useful for other people that want to integrate with this system I'm sure, but it'd also be very useful for reviewing.

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 592446 [details] [diff] [review]
patch

Stealing Jonas' review here...

+static bool
+NodeToClear(nsIContent* aContent)

This is called to check whether we should clear the purple bit on the node, so maybe rename this ShouldClearNode(), or ShouldClearPurple() or somesuch?

r=jst with that.

Comment 26

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

Attachment: patch

Comment 27

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/mozilla-central/rev/89bb79343f73