This is a partial fix for bug 157681 that was prompted by bug 500410. The basic idea is to provide fine-grained enough change hints for reflow that we can make any of the six possible calls to FrameNeedsReflow based on them. Then for style.top/left/bottom/right changes do an eResize reflow without NS_FRAME_IS_DIRTY set. This will skip reflowing the kids of the abs pos node unless its width actually changes and avoid clearing cached intrinsic widths. On the testcase for bug 500410, it drops the reflow part of the profile from 50% to 4%.

Comment on attachment 386770 [details] [diff] [review] Proposed changes >+ if (aHint & nsChangeHint_ClearDescendantIntrinsics) { >+ dirtyType = nsIPresShell::eStyleChange; Should we make it an error to not also have ClearAncestorIntrinsics set? I tend to think we should. >- if (hint & nsChangeHint_ReflowFrame) { >- StyleChangeReflow(frame); >+ if (hint & nsChangeHint_NeedReflow) { >+ StyleChangeReflow(frame, hint); You should probably either: (a) leave this, or (b) make it an error (assertion?) for any of the other reflow bits to be set without ReflowFrame also being set. or perhaps both. >-#ifdef DEBUG_roc > // Redefine these operators to return nothing. This will catch any use > // of these operators on hints. We should not be using these operators > // on nsChangeHints I seem to recall there may have been some configuration on which this caused compilation errors. You should try this patch on the try server before landing. (Making this #ifndef MSVC or something like that would be better than nothing, though.) > if (mZIndex != aOther.mZIndex) { >+ // XXXbz why do we need reflow here? > return NS_STYLE_HINT_REFLOW; > } probably needed to be before we had SyncFrameView. Now probably ok with SyncFrameView | Repaint, or maybe even just Repaint. But probably best not fixed here. >+ // Offset changes only affect positioned content, and can't affect any >+ // intrinsic widths. They also don't need to force reflow of >+ // descendants. >+ return nsChangeHint_NeedReflow; So... can you convince me that we never need ClearAncestorIntrinsics for position changes? Consider scroll frames, XUL, etc.? > if (mClipFlags != aOther.mClipFlags || mClip != aOther.mClip) { >+ // XXXbz could we use a more precise hint here? > NS_UpdateHint(hint, nsChangeHint_ReflowFrame); > } probably the same as z-index. +// XXXbz can this not return a more specific hint? If that's ever +// changed, nsStyleBorder::CalcDifference will need changing too. It's reflow only because it affects overflow areas, I think. So you could probably make it just return Repaint | ReflowFrame... although since it's inherited I could imagine that actually making things slower as we post reflows for all the descendants too, so we might want to hold off until we have a better solution for that. Holding review+ until I'm convinced we don't need ClearAncestorIntrinsics. Otherwise looks good.

Scrollframes don't cause any problems because we've made the intrinsic widths not depend on reflow; we always include the scrollbar width of an overflow:auto frame even if it's not currently visible.

> Should we make it an error to not also have ClearAncestorIntrinsics set? Yes; will add an assert. > You should probably either: > (a) leave this, or > (b) make it an error (assertion?) for any of the other reflow bits to > be set without ReflowFrame also being set. > or perhaps both. Ah, good point. I was thinking of this as a == test for some odd reason. I'll do (b). > You should try this patch on the try server before landing. Will do. > But probably best not fixed here. Filed bug 507764 (and updated the comment to reference it). > can you convince me that we never need ClearAncestorIntrinsics for position > changes? Consider scroll frames, XUL, etc.? That was my one worry, yes... It shouldn't be an issue for scrollframes, as roc says. For XUL, positioned stuff doesn't seem to be a problem for the one test I did (a box containing a rel pos div containing an abs pos div), but I guess we might have xul boxes somewhere that size to the overflow area.... And certainly changing the left/top of kids of a stack will affect the width of the stack. Let me test how that behaves with this patch. > so we might want to hold off until we have a better solution for that. OK.

And in fact with a stack testcase, I see us not resizing the stack on left/top change (at least until the window loses focus, at which point we apparently do a toplevel reflow??). Sounds lie I need to clear ancestor intrinsics after all. :( I really wish stack hadn't reused top/left/bottom/right here.

That sucks. We should figure out how to change stack so it doesn't force us to penalize normal Web content.

One simple option would be a new property, living in this struct, that controls which hint is used for left/top. We could use xul.css to set it to the right value on kids of a stack. Seems a little wasteful, but avoids any xul-user visible changes. Not sure about abs pos descendants of rel pos block kids of kids of a stack. Would need to test.

Comment on attachment 392046 [details] [diff] [review] Updated to comments >+ return NS_CombineHint(nsChangeHint_NeedReflow, >+ nsChangeHint_ClearAncestorIntrinsics);; No double ;. r=dbaron with that

Pushed http://hg.mozilla.org/mozilla-central/rev/25462849adcc with that change.

Backed out (http://hg.mozilla.org/mozilla-central/rev/7fb86c108ae7) for now to give the talos boxes a shot at the t-m merge without this patch.

This also seemed to cause a reftest failure; looking into that too.

The reftest failure was because a block was having its padding changed but not reflowing its kids, so they ended up at the wrong offsets. I'm just going to make us do a dirty reflow for padding changes.

Relanded with that change and a test for it: http://hg.mozilla.org/mozilla-central/rev/74fb7057f4dd

Seems to have resulted in a Tdhtml win of about 5% (+/-1) on all platforms.

Right; in particular, one of the Tdhtml tests looks a lot like the testcase in bug 500410 that prompted me to file this.... ;)