Closed Bug 231372 Opened 20 years ago Closed 20 years ago

GetCombinedArea could be faster/better


(Core :: Layout, defect)

Not set





(Reporter: bzbarsky, Assigned: Biesinger)


(Keywords: perf)


(1 file, 1 obsolete file)

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
that does nothing with the rect and coule be removed.

David, doing this would probably make fixing the XXX comment at
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).
Keywords: perf
Target Milestone: --- → mozilla1.7alpha
Attached patch patch (obsolete) — Splinter Review
this makes GetCombinedArea inline and returning a const nsRect&
Attachment #139354 - Flags: review?(dbaron)
Comment on attachment 139354 [details] [diff] [review]

>@@ -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.
Attachment #139354 - Flags: review?(dbaron) → review-
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.
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.
Attached patch patch v2Splinter Review
OK - now returns an nsRect, and removed VERY_NOISY_REFLOW.
Attachment #139354 - Attachment is obsolete: true
Attachment #139363 - Flags: review?(dbaron)
Attachment #139363 - Flags: review?(dbaron) → review+
Attachment #139363 - Flags: superreview?(bz-vacation)
Comment on attachment 139363 [details] [diff] [review]
patch v2

Attachment #139363 - Flags: superreview?(bz-vacation) → superreview+
checked in
Closed: 20 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.