738143 - Don't do potentially wasteful reflows of reflow roots whose parent is dirty ("ASSERTION: Reflowing while a resize is pending is wasteful")

Status: NEW

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Reflowing while a resize is pending is wasteful: '!(GetStateBits() & NS_FRAME_IS_DIRTY)', file layout/svg/base/src/nsSVGForeignObjectFrame.cpp, line 163

This assertion was added in bug 734079. Based on the comment above the assertion, it seems like a case of http://mozillamemes.tumblr.com/post/19412101741/well-let-the-machines-think-for-us ;)

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, that assert is just bogus as discussed over e-mail...

Comment 3

[Jonathan Watt [:jwatt]]

Oh, yeah, I should just remove that.

Boris, how about having the presShell skip reflow roots whose parent has a dirty bit set, on the basis that its going to get a reflow coming via its parent, and if it waits until then it will reflow at the correct size?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  That seems like it would work.  David?

Comment 5

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

Yeah, that sounds fine.  (The reflow root will always have an is-dirty or has-dirty-children bit set.)

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch

The "It's not dirty anymore" comment that I'm modifying was originally written by you, dbaron:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsPresShell.cpp&rev=3.1129#6394

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 8

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

Comment on attachment 610254 [details] [diff] [review]
patch

>         if (!NS_SUBTREE_DIRTY(target)) {
>-          // It's not dirty anymore, which probably means the notification
>-          // was posted in the middle of a reflow (perhaps with a reflow
>-          // root in the middle).  Don't do anything.
>+          // It's not dirty anymore.  This may be because reflow came via an
>+          // ancestor reflow root that was reflowed before it, or it may be
>+          // because the FrameNeedsReflow call happened in the middle of a
>+          // reflow (perhaps with a reflow root in the middle).  Don't do
>+          // anything.
>+          continue;
>+        }

I don't think the "(perhaps with a reflow root in the middle)" adds anything useful, unless I'm misunderstanding what it's saying.  Otherwise good.

>+        if (target->GetCrossDocParentFrame() &&
>+            NS_SUBTREE_DIRTY(target->GetCrossDocParentFrame()) {
>+          // Reflowing this reflow root using its parent's current size could
>+          // be wasteful since it's parent is awaiting a reflow which may
>+          // resize it and require this reflow root to reflow again. Instead of
>+          // reflowing this reflow root now, we skip it and let its reflow come
>+          // via its parent.
>           continue;
>         }

Could you put the result of target->GetCrossDocParentFrame() in an nsIFrame* variable rather than calling the function twice?

You should also add a comment explaining why this is particularly useful for the SVG case, since in general this is a very weak optimization.

So what happens if the cross-doc parent frame is dirty, but the reflow for which its dirty leads to it not actually changing size?  What ensures we'd reflow in that case?

e.g., in the stack:

#0  nsPresShell::ResizeReflow
#1  nsViewManager::DoSetWindowDimensions
#2  nsViewManager::SetWindowDimensions
#3  DocumentViewerImpl::SetBounds
#4  nsDocShell::SetPositionAndSize
#5  nsFrameLoader::UpdateBaseWindowPositionAndSize
#6  nsFrameLoader::UpdatePositionAndSize
#7  ReflowFinished
#8  nsSubDocumentFrame::ReflowFinished

, nsViewManager::DoSetWindowDimensions (and probably some of the other points) has an !oldDim.IsEqualEdges(newDim) check that would prevent anything from propagating through

Comment 9

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

Comment on attachment 610670 [details] [diff] [review]
patch

Marking review- per the previous comment (particularly the last issue)... at least to prompt a response.