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.

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.

Do you want review on this?

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.)

Bleh, I just realized the new damage computation code isn't right either. The space manager is really messy...

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...

(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.

Checked in. Leaving open for the moment for additional work, although I don't know if it will end up here.

The work to finish this is not likely to end up here.