Closed
Bug 381385
Opened 17 years ago
Closed 14 years ago
Get rid of nsFloatCache
Categories
(Core :: Layout: Floats, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: sharparrow1, Assigned: sharparrow1)
References
Details
(Keywords: memory-footprint)
Attachments
(2 files)
20.96 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
20.84 KB,
patch
|
Details | Diff | Splinter Review |
I probably won't get to this for a while, but there's a lot of potential savings here. Each nsFloatCache object takes up 88 bytes (!) plus allocation overhead, and nothing that can't be easily computed otherwise is stored there. This could be a huge savings for float-heavy pages. Might be a perf win as well as a footprint win due to less allocation and better cache usage.
Going through the properties:
PRBool mIsCurrentLineFloat;
Unused outside of debug code.
nsMargin mMargins; (margins of the float)
frame->GetUsedMargin()
nsMargin mOffsets; (relative positioning of the float)
Already stored in computedOffsetProperty (I'm not sure that was intentional, though, since it's currently unused for floats).
nsRect mRegion; (region in space manager impacted by float)
nsIFrame::GetMarginRect() - relative position offset.
(This bit is the one I'm most worried about because it's used in nsBlockReflowState::RecoverFloats, which is a perf-critical codepath for long pages with floats.)
nsRect mCombinedArea; (overflow rect of the float)
frame->GetOverflowRect() + frame->GetPosition()
The last issue is its use as a list of floats. I think we can just store a pointer to the first float on the line in nsLineBox, then use frame->GetNextSibling() to iterate over the floats where needed.
I'm surprised nobody else has filed this; just nobody got around to it?
Most of these replacements didn't exist until pretty recently; probably nobody has looked at why the float cache exists since then.
Assignee | ||
Comment 2•17 years ago
|
||
Okay, here's part 1, getting rid of the members that are easy to get rid of. Saves 52 bytes footprint (12 nscoords and a bool) per float.
Getting rid of mRegion is trickier; calculating it is a bit involved, and I'm not sure all the needed information is still around after reflow.
Assignee: nobody → sharparrow1
Status: NEW → ASSIGNED
Do you want review on this?
Assignee | ||
Comment 4•17 years ago
|
||
Comment on attachment 266244 [details] [diff] [review]
Part 1: gettting rid of the easy stuff
You want to review it? :)
I didn't set the review flag immediately because I wanted to take another look at the patch myself.
A few of the changes aren't exactly equivalent to the old code; I think that the old damage computation code was a little off with respect to position: relative, although I haven't been able to come up with any testcases to show it (it's a bit difficult to come up with cases where a float's position changes without its size changing and without causing all sorts of other damage to trigger.)
Attachment #266244 -
Flags: review?(roc)
Assignee | ||
Comment 5•17 years ago
|
||
Bleh, I just realized the new damage computation code isn't right either. The space manager is really messy...
Assignee | ||
Comment 6•17 years ago
|
||
roc, you can go ahead and review this, though; fixing damage computation to take into account changes in the translation is really another bug.
Actually, I'm not completely sure if the float damage computation is broken or not; there are other bugs which make it hard to make a testcase for the bug I think is there.
Comment on attachment 266244 [details] [diff] [review]
Part 1: gettting rid of the easy stuff
+ aFloatCache->mRegion.x = region.x + borderPadding.left;
+ aFloatCache->mRegion.y = region.y + borderPadding.top;
+ aFloatCache->mRegion.width = region.width;
+ aFloatCache->mRegion.height = region.height;
aFloatCache->mRegion = region + nsPoint(borderPadding.left, borderPadding.top);
? Optional since you only moved the code...
+ mPresContext->PropertyTable()->GetProperty(floatFrame,
+ nsGkAtoms::computedOffsetProperty));
Why aren't you just calling floatFrame->GetProperty?
// XXXldb Should this be done after handling the combined area
// below?
+ // REVIEW: No, I think the current way is right; the combined area is for
+ // painting, right?
I agree, just delete David's comment
+ nscoord x = borderPadding.left + floatMargin.left + floatX;
+ nscoord y = borderPadding.top + floatMargin.top + floatY;
Again, you're just moving code, but you could make this an nsPoint and simplify
things later
+ // XXX Floats should really just get invalidated here if nevessary
"necessary"
Otherwise fine...
Attachment #266244 -
Flags: superreview+
Attachment #266244 -
Flags: review?(roc)
Attachment #266244 -
Flags: review+
Assignee | ||
Comment 8•17 years ago
|
||
(In reply to comment #7)
> + mPresContext->PropertyTable()->GetProperty(floatFrame,
> + nsGkAtoms::computedOffsetProperty));
>
> Why aren't you just calling floatFrame->GetProperty?
I didn't really think about it; I was copying existing code that did it this way.
Assignee | ||
Comment 9•17 years ago
|
||
Assignee | ||
Comment 10•17 years ago
|
||
Checked in. Leaving open for the moment for additional work, although I don't know if it will end up here.
Assignee | ||
Comment 11•14 years ago
|
||
The work to finish this is not likely to end up here.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•