103266 - [FIX]Many useless calls to Invalidate() in nsTextFrame

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[rbs]

There are many calls to Invalidate() in nsTextFrame where the damage area is 
empty. 

I tried the following patch and countless of printfs appeared:
(see also bug 45153 for some background on the matter)

Index: html/base/src/nsTextFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/base/src/nsTextFrame.cpp,v
retrieving revision 1.323
diff -u -r1.323 nsTextFrame.cpp
--- nsTextFrame.cpp     2001/10/01 03:43:42     1.323
+++ nsTextFrame.cpp     2001/10/05 05:31:21
@@ -5242,6 +5242,10 @@

     maxFrameWidth  = PR_MAX(maxFrameWidth,  mRect.width) + onePixel;
     maxFrameHeight = PR_MAX(maxFrameHeight, mRect.height);
+if (maxFrameHeight == 0) {
+  printf("Invalidate does nothing: width=%d, height=%d\n",
+          maxFrameWidth,maxFrameHeight);
+}
     Invalidate(aPresContext, nsRect(0,0,maxFrameWidth,maxFrameHeight));
   /*}*/

Comment 1

[Marc Attinasi]

So, it there a performance benefit to removing the empty-invalidates?
nsFrame::Invalidate does a lot of work, even if the ViewManager is smart enough
to check for an empty damage rect...

Comment 2

[rbs]

Yep, it seems so. There was an |if| which was commented out but I didn't lookup
the CVS archives to track the reason. (I was instead initially curious to know
why sometimes words temporarily disappear from textareas.)

Comment 3

[Marc Attinasi]

The if was to limit the invalidates to some types of reflows
(http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsTextFrame.cpp#5232)

I think the nsFrame::Invalidate could easily check for an empty rect, no? That
way we would get most of the occurances of this, not just those from textFrame.
Of course, we would still have the extra virtual-calls, but how
performance-picky do we want to be? ;)

Comment 4

[rbs]

(I meant the reason why the |if| was commented out)

>I think the nsFrame::Invalidate could easily check for an empty rect, no?

If all is fine with the outline stuff, then indeed that will be better than no
check.

Comment 5

[Marc Attinasi]

Ha - our outlines are not even drawn outside of the frame, they are inside now
(hence -moz-outline instead of outline). That outline code should probably be
removed or #ifdef'd anyway. I'll check it out thoroughly when I test this.
Thanks Roger.

Comment 6

[Marc Attinasi]

Attachment: PATCH to check for empty damagerRects and avoid calling Invalidate. Also cleans up Invalidate a bit.

Comment 7

[Marc Attinasi]

Attacehd a patch that checks for empty damage rects before calling Invalidate.
Invalidate also checks this to catch errant callers (could be debug maybe?).

Additionally, I #ifdef'd the bogus logic that was inflating the damageRect by
the outline width - I added a big comment as to why, but I would be jsut as
happy to simply remove it all instead of #ifdef'ing it out.

Any comments?

Comment 8

[rbs]

Wow, you have sprinkled a lot of tests. Where these caught by your NS_WARNING 
radar? Or is it just for precaution. It might be that the excessive precaution 
could penalize the common cases where the area isn't empty.

Comment 9

[Marc Attinasi]

All of the tests outside of the one in Invalidate itself were reactions to the
NS_WARNING. I think it is better to check outside of Invalidate and avoid the
vfcn call if possible, that is why I mentioned that the check inside of
Invalidate might wanna be DEBUG only - still not sure. Looking at
nsRect::IsEmpty though, it sure looks cheap:

PRBool IsEmpty() const {
   return (PRBool) ((width <=0) || (height <=0));
}

it is inline too (see nsRect.h).

Comment 10

[rbs]

Comment on attachment 53817 [details] [diff] [review]
PATCH to check for empty damagerRects and avoid calling Invalidate. Also cleans up Invalidate a bit.

Indeed the check could be kept in DEBUG now that you have fixed callers.

Also, mind changing the rect test to (the change is the most frequent case):
-   return (PRBool) ((width <=0) || (height <=0));
+   return (PRBool) ((height <=0) || (width <=0));

r=rbs

Comment 11

[Marc Attinasi]

Roger, how did you determien that height is 0 more often than width for empty rects?

And, I'll incormporate your comments - thanks!

Comment 12

[rbs]

Since most frames pass through nsLineLayout, I just happened to mostly stumble 
at that case when debugging other problems (just intuition, no particular data 
to backup).

Comment 13

[Marc Attinasi]

Attachment: Additional patch to reverse order of check in nsRect::IsEmpty

Comment 14

[David Hyatt]

sr=hyatt

Comment 15

[Marc Attinasi]

Fix checked in to trunk. 

Comment 16

[Judson Valeski]

minusing. perf improvment acknowledged. if the improvment is more significant
that we're assuming right now, please re-nominate.

Comment 17

[Marc Attinasi]

Yea, the perf improvement is pretty tiny in most cases.