Closed
Bug 231372
Opened 21 years ago
Closed 21 years ago
GetCombinedArea could be faster/better
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.7alpha
People
(Reporter: bzbarsky, Assigned: Biesinger)
Details
(Keywords: perf)
Attachments
(1 file, 1 obsolete file)
14.42 KB,
patch
|
dbaron
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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).
Assignee | ||
Updated•21 years ago
|
Assignee | ||
Comment 1•21 years ago
|
||
this makes GetCombinedArea inline and returning a const nsRect&
Assignee | ||
Updated•21 years ago
|
Attachment #139354 -
Flags: review?(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.
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.
Assignee | ||
Comment 5•21 years ago
|
||
OK - now returns an nsRect, and removed VERY_NOISY_REFLOW.
Attachment #139354 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #139363 -
Flags: review?(dbaron)
Attachment #139363 -
Flags: review?(dbaron) → review+
Assignee | ||
Updated•21 years ago
|
Attachment #139363 -
Flags: superreview?(bz-vacation)
Reporter | ||
Comment 6•21 years ago
|
||
Comment on attachment 139363 [details] [diff] [review]
patch v2
sr=bzbarsky
Attachment #139363 -
Flags: superreview?(bz-vacation) → superreview+
Assignee | ||
Comment 7•21 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
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.
Description
•