378784 - [FIX]Don't reflow ancestors of a reflow root if we don't have to

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

If FrameNeedsReflow is called on a reflow root that is not itself dirty (just has dirty children), we shouldn't need to do anything with its ancestors.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 2

[Eli Friedman]

I'm pretty sure dbaron was trying to avoid requiring scanning mDirtyRoots; we don't want to make operations like changing the text in a lot of textboxes at the same time O(n^2) in the number of textboxes.

If it turns out scanning mDirtyRoots is unavoidable, we might want to turn it into a hashset.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Actually, in the case of textboxes we're not changing direct kids of a reflow root.  So we're not calling FrameNeedsReflow() on the reflow root itself, and hence wouldn't fall into this case...

I agree that the scanning is suboptimal, but the only way I can think of to eliminate it in general is to change the FrameNeedsReflow contract to take the bits that should be added instead of adding them before calling FrameNeedsReflow (and then to base the wasDirty decision on the state before those bits were added).

Which might well be the way to go, come to think of it.  David, is there a reason you didn't do it that way?

Comment 4

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

I suppose I didn't do it that way because it seemed harder at the time, although I eventually found that the marking dirty almost always ended up right next to the call -- so it could become part of FrameNeedsReflow, I suppose.

Why do you need an extra scan of mDirtyRoots without it, though?  Why does it matter?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Why do you need an extra scan of mDirtyRoots without it, though?

Because we're trying to maintain the invariant that each dirty root is present in mDirtyRoots once.  If aFrame is the root we've decided we might need to add to mDirtyRoots, we have no way to tell, by the time we're in FrameNeedsReflow whether it's in mDirtyRoots just by looking at its bits.  So we have to scan the array.

Comment 6

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

Ah, right.  OK, so that does make sense.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: So this is also an option

David, you want to take a look at the API changes and maybe the nsPresShell.cpp changes and let me know what you think?  If you like this approach, I'll need to get rbs to review the mathml changes too...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Second option merged to tip

Comment 9

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

Comment on attachment 263555 [details] [diff] [review]
Second option merged to tip

>+#define FRAME_IS_REFLOW_ROOT(_f)                   \
>+  ((_f->GetStateBits() & NS_FRAME_REFLOW_ROOT) &&  \
>+   (_f != aFrame || !targetFrameDirty))
>+

Probably makes more sense to define this inside the function.

> NS_IMETHODIMP
>-PresShell::FrameNeedsReflow(nsIFrame *aFrame, IntrinsicDirty aIntrinsicDirty)
>+PresShell::FrameNeedsReflow(nsIFrame *aFrame, IntrinsicDirty aIntrinsicDirty,
>+                            nsFrameState aBitsToAdd)
> {
>-  NS_PRECONDITION(aFrame->GetStateBits() &
>+  NS_PRECONDITION(aBitsToAdd &
>                     (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN),
>                   "frame not dirty");

You should fix this assertion text, and you should also assert that aBitsToAdd contains no other bits.

>   // Set NS_FRAME_HAS_DIRTY_CHILDREN bits (via nsIFrame::ChildIsDirty) up the
>   // tree until we reach either a frame that's already dirty or a reflow root.
>   nsIFrame *f = aFrame;
>-  PRBool wasDirty = PR_TRUE;
>   for (;;) {
>-    if (((f->GetStateBits() & NS_FRAME_REFLOW_ROOT) && f != aFrame) ||
>-        !f->GetParent()) {
>-      // we've hit a reflow root or the root frame
>+    if (FRAME_IS_REFLOW_ROOT(f) || !f->GetParent()) {
>+      // we've hit a reflow root or the root frame.  Note that if the reflow
>+      // root in question is aFrame, we have wasDirty set to true no matter
>+      // whether it was actually dirty before whatever chane triggered this

s/chane/change/

(Ah, the joy of reviewing in a textarea with spellcheck.)

>+      // call.  So in that case we have to explicitly check whether we're in
>+      // the list.

So why is wasDirty always true if the root in question is aFrame?  And we're only checking the list if wasDirty was false, so the comment really doesn't make sense to me.

>Index: layout/generic/nsAbsoluteContainingBlock.cpp

Up to here, but the rest looks like it should be quick.

Comment 10

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

Comment on attachment 263555 [details] [diff] [review]
Second option merged to tip

>Index: layout/generic/nsBlockFrame.cpp
>     RenumberLists(presContext);
> 
>     presContext->PresShell()->
>-      FrameNeedsReflow(this, nsIPresShell::eStyleChange);
>+      FrameNeedsReflow(this, nsIPresShell::eStyleChange,
>+                       NS_FRAME_HAS_DIRTY_CHILDREN);

This pattern occurs twice in AttributeChanged (with different frames) -- seems like you could check the return value from RenumberLists before calling FrameNeedsReflow.

>Index: layout/generic/nsBlockFrame.h
>+  // If this returns PR_TRUE, the block it's called on should get the
>+  // NS_FRAME_HAS_DIRTY_CHILDREN bit set on it by the caller.

And a reflow scheduled if needed.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> So why is wasDirty always true if the root in question is aFrame? 

Er... that comment is left over from the previous patch version.  I'll remove it.

> seems like you could check the return value from RenumberLists

Sure.

> And a reflow scheduled if needed.

Indeed.  Will change comment accordingly.

Comment 12

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

Comment on attachment 263555 [details] [diff] [review]
Second option merged to tip

>Index: layout/generic/nsTextFrame.cpp
>   PRBool markAllDirty = PR_TRUE;
>   if (aAppend) {
>     markAllDirty = PR_FALSE;
>     nsTextFrame* frame = NS_STATIC_CAST(nsTextFrame*, GetLastInFlow());
>     frame->mState &= ~TEXT_WHITESPACE_FLAGS;
>-    frame->mState |= NS_FRAME_IS_DIRTY;
>     targetTextFrame = frame;
>   }
> 
>   if (markAllDirty) {

Could you just make this an if/else and remove markAllDirty?

Also, GetLastInFlow should probably be GetLastContinuation.

>     // Mark this frame and all the next-in-flow frames as dirty and reset all
>     // the content offsets and lengths to 0, since they no longer know what
>     // content is ok to access.
>     nsTextFrame*  textFrame = this;
>+    nsFrameState state = mState;
>     while (textFrame) {
>       textFrame->mState &= ~TEXT_WHITESPACE_FLAGS;
>       textFrame->mState |= NS_FRAME_IS_DIRTY;
>       textFrame->mContentOffset = 0;
>       textFrame->mContentLength = 0;
>       textFrame = NS_STATIC_CAST(nsTextFrame*, textFrame->GetNextContinuation());
>     }
>+    // Clear the NS_FRAME_IS_DIRTY flag we just set...  although maybe it's not
>+    // worth worrying since we're not going to be a reflow root?
>+    mState = state & ~TEXT_WHITESPACE_FLAGS;

Maybe optimize for fewer continuations and just don't set NS_FRAME_IS_DIRTY on this with a test?  I think that would be clearer?

Or, if you want to optimize, replace the while(textFrame) with a for(;;) { ... if (textFrame) break; ... } and set NS_FRAME_IS_DIRTY at the bottom.  (I think that's still clearer than what you have now.)

Either way, add a comment "// don't set NS_FRAME_IS_DIRTY on this, since we call FrameNeedsReflow below".

>Index: layout/generic/nsTextFrameThebes.cpp

Same changes here.


>Index: layout/mathml/base/src/nsMathMLContainerFrame.h
>+  // aBits are the bits to pass to FrameNeedsReflow()when we call it.

Missing space.


>Index: layout/mathml/base/src/nsMathMLmactionFrame.cpp

Bad indentation in both changes, perhaps.  (Does this file have tabs?)

>Index: layout/mathml/base/src/nsMathMLmtableFrame.cpp
>+    // XXXbz I have no idea why this is reflowing the _parent_ instead of
>+    // us... so to be safe, just set all the dirty bits.  If we were reflowing
>+    // ourselves, just NS_FRAME_IS_DIRTY would do, I think.
>     PresContext()->PresShell()->
>-      FrameNeedsReflow(mParent, nsIPresShell::eStyleChange);
>+      FrameNeedsReflow(mParent, nsIPresShell::eStyleChange,
>+                       NS_FRAME_HAS_DIRTY_CHILDREN | NS_FRAME_IS_DIRTY);

Just set NS_FRAME_IS_DIRTY, which subsumes NS_FRAME_HAS_DIRTY_CHILDREN.

In fact, in PresShell::FrameNeedsReflow, you should probably just assert that aBits equals NS_FRAME_IS_DIRTY or NS_FRAME_HAS_DIRTY_CHILDREN, rather than doing complicated bitmask tests.  It doesn't make sense to do anything else.  (Were there other places you did this in the patch?  If so, they should be corrected.)



>Index: layout/tables/nsTableOuterFrame.cpp
>-  AddStateBits(NS_FRAME_HAS_DIRTY_CHILDREN); // also means child removed
>   PresContext()->PresShell()->
>-    FrameNeedsReflow(this, nsIPresShell::eTreeChange);
>+    FrameNeedsReflow(this, nsIPresShell::eTreeChange,
>+                     NS_FRAME_HAS_DIRTY_CHILDREN);

Could you preserve the comment?

>Index: layout/xul/base/src/nsBoxFrame.cpp
>     // If our parent is not a box, there's not much we can do... but in that
>     // case our ordinal doesn't matter anyway, so that's ok.
>     if (parent) {
>       parent->RelayoutChildAtOrdinal(state, frameToMove);
>-      mState |= NS_FRAME_IS_DIRTY;
>       // XXXldb Should this instead be a tree change on the child or parent?
>       PresContext()->PresShell()->
>-        FrameNeedsReflow(frameToMove, nsIPresShell::eStyleChange);
>+        FrameNeedsReflow(frameToMove, nsIPresShell::eStyleChange,
>+                         NS_FRAME_IS_DIRTY);
>+      // XXXbz tossing this in is a little weird, but this is
>+      // basically what the code _used_ to do...
>+      mState |= NS_FRAME_IS_DIRTY;

It wasn't this way pre-reflow-branch.  Just remove those extra three lines.

Though I think it actually would be better to call FrameNeedsReflow on parent instead.  There's a chance I analyzed the pre-reflow-branch code more carefully when I was doing the reflow branch, but I suspect not.



Thanks for doing this -- it looks like it will fix a bunch of reflow branch regressions.

Just waiting on the question in comment 9 before r+sr.

Comment 13

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

Comment on attachment 263555 [details] [diff] [review]
Second option merged to tip

OK, yeah, it makes sense without the comment, so r+sr=dbaron with those changes.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

rbs, could you look over the MathML changes?

> Though I think it actually would be better to call FrameNeedsReflow on parent
> instead.

I guess that's in fact what the pre-reflow-branch code did... OK, I'll do that, with a style change on the parent to be safe.

Comment 15

[Jonathan Watt [:jwatt]]

As requested in bug 369827 comment 11, this is a heads up that nsSVGForeignObjectFrame's FrameNeedsReflow calls have changed.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip

Merged to the SVG changes.

David asked me to go ahead and check this in with his reviews because it's blocking other things, but I'd still like rbs to look over the MathML changes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 18

[Jeff Walden [:Waldo]]

I cringe at saying a 140K patch doesn't need tests, but if the patch shouldn't change behavior, writing random tests probably isn't productive.  :-\

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yeah... the size has more to do with "diff -8" than anything else.  And yeah, it shouldn't change any existing correct behavior, other than making some cases faster... but that's hard to test.  :(

Comment 20

[rbs]

Comment on attachment 263934 [details] [diff] [review]
Merged to tip

r=rbs, the MatML bit looks reasonnable.

------
 The
+   * bits to add should be some combination of NS_FRAME_IS_DIRTY and
+   * NS_FRAME_HAS_DIRTY_CHILDREN.
[...]
+PresShell::FrameNeedsReflow(nsIFrame *aFrame, IntrinsicDirty aIntrinsicDirty,
+                            nsFrameState aBitsToAdd)
 {
+  NS_PRECONDITION(aBitsToAdd == NS_FRAME_IS_DIRTY ||
+                  aBitsToAdd == NS_FRAME_HAS_DIRTY_CHILDREN,
+                  "Unexpected bits being added");

Nit: mismatch? the comment seems to imply that isDirty | hasDirtyChildren could
also be a valid input, but this isn't what the precondition is saying.


Index: layout/mathml/base/src/nsMathMLmtableFrame.cpp

+    // XXXbz I have no idea why this is reflowing the _parent_ instead of
+    // us...

This is on purpose (out of need -- somewhat like you may have seen in other places). No need for the XXX comment.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Good catch on the nsIPresShell comment.  Fixed.  Also removed the XXX comment in question.

Comment 22

[Mats Palmgren (inactive)]

s/eithr/either

Comment 23

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Tinderbox seems to claim that this regressed Tp/Tp2 a little... Not sure why.  :(