381385 - Get rid of nsFloatCache

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Eli Friedman]

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?

Comment 1

[David Baron :dbaron:]

Most of these replacements didn't exist until pretty recently; probably nobody has looked at why the float cache exists since then.

Comment 2

[Eli Friedman]

Attachment: Part 1: gettting rid of the easy stuff

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.

Comment 3

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

Do you want review on this?

Comment 4

[Eli Friedman]

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

Comment 5

[Eli Friedman]

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

Comment 6

[Eli Friedman]

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 7

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

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

Comment 8

[Eli Friedman]

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

Comment 9

[Eli Friedman]

Attachment: Gettting rid of the easy stuff, final version

Comment 10

[Eli Friedman]

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

Comment 11

[Eli Friedman]

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