262764 - [FIXr]ComparePoints() is pretty slow

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

ComparePoints creates a new range, does a bunch of random manipulation, etc.  It
doesn't need to do any of that stuff, really.

Timing setting of innerHTML, 10% of it is spent in ComparePoints().  I'll attach
a patch that reduces that to 1%.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 160978 [details] [diff] [review]
Patch

The changes to GetNodeBracketPoints were just removing unnecessary QIs and
such.

In addition, would it make sense to make IsIncreasing just use
CompareTreePosition?  The locking is pretty expensive; I have to wonder whether
the stack-allocation of arrays that CompareTreePosition does is faster...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 160978 [details] [diff] [review]
Patch

This is wrong.	There are more callers than that.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Fixed so it all works

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 161020 [details] [diff] [review]
Fixed so it all works

>Index: content/base/src/nsRange.cpp
>===================================================================

>-  if (!(*outParent)) // special case for root node
>+  if (!parent) // special case for root node
>   {
>     // can't make a parent/offset pair to represent start or 
>     // end of the root node, becasue it has no parent.
>     // so instead represent it by (node,0) and (node,numChildren)
>     *outParent = do_QueryInterface(aNode);
>-    nsCOMPtr<nsIContent> cN(do_QueryInterface(*outParent));
>-    if (!cN)
>-      return PR_FALSE;

It'd be funny if it wasn't sad.

> NS_IMETHODIMP_(PRBool) 
> nsRangeUtils::IsNodeIntersectsRange(nsIContent* aNode, nsIDOMRange* aRange)
> {
>-  return ::IsNodeIntersectsRange( aNode,  aRange);
>+  return nsRange::IsNodeIntersectsRange( aNode,  aRange);
> }

Do we even need this? Seem unused.

>Index: content/base/src/nsSelection.cpp
>===================================================================

>@@ -5037,22 +5039,22 @@ nsTypedSelection::LookUpSelection(nsICon
>           newDetails->mEnd = PR_MIN(aContentLength, endOffset - aContentOffset);
>           newDetails->mType = aType;
>         }
>       }
>       else { //then we MUST be completely selected! unless someone needs us to check to make sure with slowcheck
> 
>         if (cnt > 1 || aSlowCheck){ //if more than 1 selection or we need to do slow check see if farther than start or less than end.
>           //we only have to look at start offset because anything else would have been in the range
>-          PRInt32 resultnum = ComparePoints(startNode, startOffset 
>-                                  ,passedInNode, aContentOffset);
>+          PRInt32 resultnum = nsRange::ComparePoints(startNode, startOffset,
>+                                                     passedInNode, aContentOffset);

Long line.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Do we even need this? Seem unused.

I'm not sure, but it doesn't cost us much to expose it, so.... ;)

> Long line.

Will fix.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.