Closed Bug 103266 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla0.9.6

People

(Reporter: rbs, Assigned: attinasi)

References

Details

(Keywords: perf, topembed)

Attachments

(2 files)

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));
   /*}*/
Keywords: perf
Status: NEW → ASSIGNED
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...
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.)
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? ;)
(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.
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.
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?
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.
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 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
Attachment #53817 - Flags: review+
Roger, how did you determien that height is 0 more often than width for empty rects?

And, I'll incormporate your comments - thanks!
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).
Priority: -- → P3
Target Milestone: --- → mozilla0.9.6
Summary: Many useless calls to Invalidate() in nsTextFrame → [FIX]Many useless calls to Invalidate() in nsTextFrame
Blocks: 56854
sr=hyatt
Fix checked in to trunk. 
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: edt0.9.4
Resolution: --- → FIXED
minusing. perf improvment acknowledged. if the improvment is more significant
that we're assuming right now, please re-nominate.
Keywords: edt0.9.4edt0.9.4-
Yea, the perf improvement is pretty tiny in most cases.
Keywords: topembed
You need to log in before you can comment on or make changes to this bug.