231372 - GetCombinedArea could be faster/better

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

GetCombinedArea currently copies the rect into its out param.  It would probably
be faster to return a |const nsRect&| from this method (which would also mean
not having to do that silly null-check).  The method could also be inlined then,
if desired.

There is also a GetCombinedArea at
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsBlockFrame.cpp#2076
that does nothing with the rect and coule be removed.

David, doing this would probably make fixing the XXX comment at
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsBlockFrame.cpp#1383
harder.  What do you think?

The reason I'm filing this is that I have a profile here of loading a text/plain
document, and GetCombinedArea is taking 7.4% of the total pageload time (largely
due to the use of a single <pre> for all the text and the ensuing multiple calls
to nsBlockFrame::ComputeCombinedArea).

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

this makes GetCombinedArea inline and returning a const nsRect&

Comment 2

[David Baron :dbaron:]

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

>@@ -2285,28 +2280,26 @@ nsBlockFrame::ReflowLine(nsBlockReflowSt
...
>-  nsRect oldCombinedArea;
>-  aLine->GetCombinedArea(&oldCombinedArea);
>+  const nsRect& oldCombinedArea = aLine->GetCombinedArea();
> 
>   if (aLine->IsBlock()) {
>     rv = ReflowBlockFrame(aState, aLine, aKeepReflowGoing);
> 
>     // We expect blocks to damage any area inside their bounds that is
>     // dirty; however, if the frame changes size or position then we
>     // need to do some repainting
>     if (aDamageDirtyArea) {
>-      nsRect lineCombinedArea;
>-      aLine->GetCombinedArea(&lineCombinedArea);
>+      const nsRect& lineCombinedArea = aLine->GetCombinedArea();
>       if ((oldCombinedArea.x != lineCombinedArea.x) ||
>           (oldCombinedArea.y != lineCombinedArea.y)) {

You're going to break this, since this caller was relying on having a copy
while the original changes.  I didn't look too closely, so there may well be
other occurences of the same thing.

Comment 3

[David Baron :dbaron:]

And you can probably get rid of everything inside VERY_NOISY_REFLOW.  There are
only two occurrences and if anyone needs printfs, they can add printfs when they
need them.  I've needed printfs in many places, but never those.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think you should just return a plain nsRect copy. That's what we're doing in
most places. It avoids these very subtle bugs and in most cases is just as
efficient, assuming the method is inlined.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

OK - now returns an nsRect, and removed VERY_NOISY_REFLOW.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 139363 [details] [diff] [review]
patch v2

sr=bzbarsky

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in