855464 - nsBlockReflowState::ClearFloats should call nsBlockFrame::WidthToClearPastFloats less

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yesterday I was looking over bent's shoulder at a profile of the B2G contacts app, and noticed we were spending a good bit of time in reflow state construction inside nsBlockFrame::WidthToClearPastFloats.

This is pretty easily avoidable (and was eliminated with a b2g18 version of the patch I'll attach).  Basically, WidthToClearPastFloats is somewhat expensive, and ClearFloats makes no effort to avoid calling it when it's not needed, but easily could do so.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Optimize nsBlockReflowState::ClearFloats better, given that nsBlockFrame::WidthToClearPastFloats is somewhat expensive.

This fixes a performance issue that showed up in a profile of the b2g
contacts app (though it's not clear what percentage of the time it was
given the difficulty of understanding output of 'perf').

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 730351 [details] [diff] [review]
Optimize nsBlockReflowState::ClearFloats better, given that nsBlockFrame::WidthToClearPastFloats is somewhat expensive.

Er, I should write this on top of m-c rather than my patches to bug 25888.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Optimize nsBlockReflowState::ClearFloats better, given that nsBlockFrame::WidthToClearPastFloats is somewhat expensive.

This fixes a performance issue that showed up in a profile of the b2g
contacts app (though it's not clear what percentage of the time it was
given the difficulty of understanding output of 'perf').

Comment 4

[Gregor Wagner [:gwagner]]

It seems to be a 1% win for the contacts app.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 730353 [details] [diff] [review]
Optimize nsBlockReflowState::ClearFloats better, given that nsBlockFrame::WidthToClearPastFloats is somewhat expensive.

>@@ -940,31 +940,40 @@ nsBlockReflowState::ClearFloats(nscoord 
[...]
>+
>+  if (!mFloatManager->HasAnyFloats()) {
>+    return aY;
>+  }
>+
>   nscoord newY = aY;
> 
>   if (aBreakType != NS_STYLE_CLEAR_NONE) {
>     newY = mFloatManager->ClearFloats(newY, aBreakType, aFlags);
               ^
It looks like this is the only caller of nsFloatManager::ClearFloats().  nsFloatManager::ClearFloats currently has an early-return if !HasAnyFloats() -- but now (with this patch) we will have already checked for that before we even enter nsFloatManager::ClearFloats().

So, the existing check in nsFloatManager::ClearFloats() will now be redundant, so it could perhaps be converted to a precondition / assertion. Doesn't matter much, though; it's also fine as-is.

r=me regardless

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c46ce2cb5026

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c46ce2cb5026