454280 - [FIX]nsContentList performance improvements

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I did some nsContentList profiling for bug 453929, and think that we can make things a bit faster here.  Patch should be coming up pretty soon.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Make it faster

This speeds up the tree walk a good bit.  The main changes are:

1)  Make the IsNodeOfType() check as early as we can, so we can avoid all the
    function call overhead for the (plentiful) textnodes.
2)  Use NS_FASTCALL on the method that does the actual walking.
3)  Use a faster way to iterate the kids (fewer virtual calls).

Taken together, these make the dromaeo tests that actually test this code (so the getElementsByClassName and getElementsByName tests) almost 1.8x faster.

Comment 2

[Benjamin Smedberg]

FWIW, quite a few of the dromaeo DOM tests spend some significant time in nsContentList, so this is likely to improve the general benchmark by a fair bit.

Comment 3

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

Comment on attachment 337588 [details] [diff] [review]
Make it faster

>@@ -665,20 +670,16 @@ nsContentList::Match(nsIContent *aConten

Could you add an assertion to the top of nsContentList::Match to make sure that aContent is an element? That seems to be the general idea here right?

Also, this means that we can't make content-lists containing things other than elements, right?

>@@ -761,22 +779,25 @@ nsContentList::PopulateWithStartingAfter
>     if (aStartChild) {
>       i = aStartRoot->IndexOf(aStartChild);
>       NS_ASSERTION(i >= 0, "The start child must be a child of the start root!");
>       ++i;  // move to one past
>     }
> 
>     PRUint32 childCount = aStartRoot->GetChildCount();
>     for ( ; ((PRUint32)i) < childCount; ++i) {
>-      PopulateWith(aStartRoot->GetChildAt(i), aElementsToAppend);
>-    
>-      NS_ASSERTION(aElementsToAppend + mElements.Count() == invariant,
>-                   "Something is awry in PopulateWith!");
>-      if (aElementsToAppend == 0)
>-        return;
>+      nsIContent* content = aStartRoot->GetChildAt(i);
>+      if (content->IsNodeOfType(nsINode::eELEMENT)) {
>+        PopulateWith(content, aElementsToAppend);
>+
>+        NS_ASSERTION(aElementsToAppend + mElements.Count() == invariant,
>+                     "Something is awry in PopulateWith!");
>+        if (aElementsToAppend == 0)
>+          return;
>+      }

Why not use the GetChildArray trick here too?

Also, you could make GetChildArray return both a length and an array pointer (one of the two through an out param) in order to avoid one virtual call.

r/sr=me either way

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Could you add an assertion to the top of nsContentList::Match

Done.

> Also, this means that we can't make content-lists containing things other than
> elements, right?

Yes.  We never do right now, and I couldn't think of use cases, so much.

> Why not use the GetChildArray trick here too?

It didn't pop up as a hotspot in this particular profile.  You're right that I should; I've made that change.

> Also, you could make GetChildArray return both a length and an array pointer

Filed bug 454821.  That depends on killing EnsureContentsGenerated.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 6

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

Hmm.. just realized, why not put the IsNodeOfType(eELEMENT) test at the top of Match() instead of at each caller? Or do we have heavy-hitting callers that don't need to check? I guess AttributeChanged?

Comment 7

[Boris Zbarsky [:bzbarsky]]

It's slower.  As currently written, for textnodes we avoid:

1)  Calling PopulateWith
2)  Calling Match
3)  Calling various virtual methods like GetChildCount and GetChildArray

because we do the IsNodeOfType check up front before even calling into PopulateWith.  In fact, avoiding all those function calls is pretty much key to the speedup here; a lot of the time in this code is (still, and it was worse before) taken up by function-call overhead.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Pushed changeset 1bbcf1a4dc5 and changeset 9a15160ea01d to fix ensuing build bustage.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Backed out because it caused orange.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to fix the orange

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Actually, this is the right diff

Comment 12

[Boris Zbarsky [:bzbarsky]]

Pushed changeset 4dfc88a342b2.

Comment 13

[[e-mail]]

Comment on attachment 338342 [details] [diff] [review]
Actually, this is the right diff

>+#ifdef DEBUG
>+  NS_ASSERTION(!debugMutationGuard.Mutated(0),
>+               "Unexpected mutations happened.  Check your match function!");
>+#endif  
Are content lists supposed to force generation of templated content? (This assertion fires because they do.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

1)  Yes, I guess (no other way around it).
2)  See bug 456653.
3)  Lazy generation is going away in bug 450990, hopefully ASAP.