170330 - Excessive repainting during reflow

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

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

Go to http://dhtmlcentral.com and scroll one of the 'windows' with paint
flashing on. You'll see that we repaint a huge area that extends outside the
visible area of the window.

The window has two views (actually more, but two views matter). There is a view
for the "outside" frame, whose size is fixed, and the frame is
"overflow:hidden". There is a view for the inside frame, whose size grows with
the content, and which gets repositioned to achieve the scrolling effect.

When the inside frame is moved layout tries to repaint the whole inside frame,
but the view manager clips the repainted area to the outside view. So no problem
there. The problem is that the outside frame's 'overflow area', which determines
its view size, grows to contain the entire inside frame even though the outside
frame is overflow:hidden! This means that the outside frame's view also grows,
which forces a repaint of the outside view over the area where the inside frame
is (or would be, if it wasn't clipped).

I'm working on a patch for this. My goal is to apply clipping to the
overflowAreas of child frames before adding them to the overflowArea of the
parent frame. Does that sound reasonable?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Just a silly testcase

Sounds like the proposed change will make our behavior in this testcase much
better...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, and this might fix some other bugs too.

Comment 3

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

That testcase is actually a different bug, I think. That seems to be an example
of the overflowArea confusion where relative positioning and the rest of layout
disagree on the meaning of overflowArea. See bug 131475.

Actually I think the way to fix that testcase is not to consider clipping when
calculating the area of a table cell, but to exclude absolutely positioned
frames from that calculation.

Comment 4

[Christopher Hoess (gone)]

Adding bug 42657 so it will get checked when this is resolved.

Comment 5

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

is that the right bug number?

Comment 6

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

Attachment: fix

Here's a fix. I've abstracted out the final phase of overflow calculation into
a new helper function nsFrame::ComputeOverflowArea. It only adds in the child
bounds if 'overflow' is not 'hidden'. It also takes 'outline' into account,
which helps pave the way for 'outline' to actually work properly. We call this
everywhere it needs to be called.

I admit that the way inline frames get their OUTSIDE_CHILDREN bit set is a
little tricky. But it seems right.

I also modified the view system so that some of its invalidations are masked
using the child clip region, when those invalidations are being performed by a
parent on behalf of a child.

This patch fixes the overpainting while scrolling in http://dhtmlcentral.com.
It also fixes the testcase that bz attached, but as I said, that's just a side
effect which we shouldn't really be counting on.

Comment 7

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

The comment in the last change in nsBlockFrame should be added to to say

"We do this because relatively positioned frames that are also spans have two
contributors to the overflow area: overflowing inline children and overflowing
absolute children. The combined area computed by RelativePositionFrames for a
relatively positioned span does not include the absolute children."

I'll fix that.

The removal of the state-setting code in nsBlockFrame::ComputeFinalSize works
because ComputeOverflowArea is called unconditionally after the only call to
ComputeFinalSize.

Comment 8

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

Comment on attachment 101876 [details] [diff] [review]
fix

r=dbaron

Comment 9

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

roc:  Are you planning to land this during 1.3alpha?

Comment 10

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

Yes!

Comment 11

[kinmoz]

Comment on attachment 101876 [details] [diff] [review]
fix

sr=[e-mail]

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 101876 [details] [diff] [review]
fix

a=asa for checkin to 1.3a

Comment 13

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

Fix checked in.

Comment 14

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

I backed this out because it appears to have caused a Tp regression.

Comment 15

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

Attachment: New patch

Here's another attempt. What I've done is optimize ComputeOverflowArea in two
ways. The 'outline' stuff is #ifdefed out because we don't really need it at
this stage. Also, now we only look at the style info if the frame has a view;
this is OK because any overflow:hidden block will have a view. Also I think
that when we make 'outline' work right to draw on the outside of the frame, we
should set things up so any frame with 'outline' also has a view, which will
make many things simpler and easier.

I used a new HasView() method which is very fast (much faster than checking
GetView() != nsnull) --- it only needs to check the state bit. Eventually we
could use this elsewhere for slightly cleaner and significantly faster code.

Comment 16

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

Comment on attachment 108854 [details] [diff] [review]
New patch

need reviews for new version of patch

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 108854 [details] [diff] [review]
New patch

you have an #if 0 in there, is that intentional?

Comment 18

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

Yes it's intentional.

> The 'outline' stuff is #ifdefed out because we don't really need it at
> this stage

Comment 19

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

Comment on attachment 108854 [details] [diff] [review]
New patch

nsLineLayout.cpp:
>+    // We're conservative here; the combined area for this span/frame
>+    // is the union of the area that the frame computed during reflow
...
>+    // But in practice it's OK to have an overflow area that's larger than necessary.
>+    // (Although if it happens too often it could become a paint performance issue).

Really, it's not that great, considering implementations of 'overflow: auto'
and 'overflow: scroll'.  But then again, those need to account for margins
too...

r+sr=dbaron

Comment 20

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

I tried to check this in but it caused another Tp regression.I even tried
removing the extra call to SyncFrameViewAfterSizeChange from nsLineLayout, to
see if that was causing the Tp regression, but apparently not. So I backed out.

Right now I have no idea why this is causing a Tp regression :-(.

Comment 21

[Bob Clary [:bc] (inactive)]

what kind of Tp regressions are we looking at compared to the benefits of this
patch? Maybe the benefits outweigh the cost? Nominating nsbeta1/topembed for
consideration.

Comment 22

[Michael Buckland]

Discussed in bug triage meeting.  Any ideas about next move?  Please advise in
this bug.

Comment 23

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

I've been away. I still don't know what is causing the Tp problem.

Comment 24

[Judson Valeski]

minusing per email thread w/ roc.

Comment 25

[Andrew Hagen]

If this is off-base, I apologize in advance.

Is this bug related to bug 84920?

Comment 26

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

I doubt it.

Comment 27

[Markus Hübner]

Concerning Bob's input in comment #21 - what are we intending to do?

Comment 28

[Samir Gehani]

adt: nsbeta1-

Comment 29

[Adam D. Moss]

Is the patch still okay?  I expect that it's bit-rotted by now.

Comment 30

[Markus Hübner]

Well, comment #21 sounds reasonable - we either go with it and do some testing 
on the trunk or anyone is capable of solving this intricate, remaining problem.

Comment 31

[Adam D. Moss]

Hmm.  How strange that this 'fix' causes performance regressions!

Bug in the approach or a bug in the patch?  (Or, indeed,
a bug in the Tp testcases in terms of not being
representative?  Or...?)

After a casual browse of the patch, here's a reshuffle
of nsFrame::ComputeOverflowArea().  It removes some work
(a union and the overflow test) in the !HasView() case,
and moves the |#if 0| stuff into the if() block which is
AFAICS the only path in which it'd do useful things.

I haven't compiled/tested this tweak.  If it's broken
then it's either my misunderstanding or it's a fault in
the logic that the reshuffle was based on (ie. a clue).

void
nsFrame::ComputeOverflowArea(nsRect& aOverflowArea,
                             const nsRect& aCombinedChildren)
{
  // the frame's rect in its own coordinate system
  nsRect r(0, 0, mRect.width, mRect.height);

  // Don't do the following mildly expensive stuff unless we have a view;
  // any block frame with overflow:hidden will have a view
  // (see nsContainerFrame::FrameNeedsView).
  // HasView() is very cheap; it just checks a bit on mState.
  if (HasView()) {
    // overflow:hidden doesn't need to take account of the child area
    const nsStyleDisplay* display;
    ::GetStyleData(mStyleContext, &display);
    if (NS_STYLE_OVERFLOW_HIDDEN == display->mOverflow
        && (display->IsBlockLevel() || display->IsFloating())) {
      // The following code is temporarily disabled. We'll turn it on
      // when proper support for 'outline' lands. Note that it assumes
      // any frame with 'outline' has a view. --- roc
#if 0
      // Add in the outline width, which overflows our border area
      const nsStyleOutline* outline;
      ::GetStyleData(mStyleContext, &outline);
      
      nscoord width;
      outline->GetOutlineWidth(width);
      r.Inflate(width, width);
#endif
      aOverflowArea = r;
    }

    // Set state bit to indicate whether there is any overflow
    if (aOverflowArea.x < 0
	|| aOverflowArea.y < 0
	|| aOverflowArea.XMost() > mRect.width
	|| aOverflowArea.YMost() > mRect.height) {
      mState |= NS_FRAME_OUTSIDE_CHILDREN;
    } else {
      mState &= ~NS_FRAME_OUTSIDE_CHILDREN;
    }

  } else {

    // Make the default combined area: frame U children
    aOverflowArea.UnionRect(aCombinedChildren, r);
    // WARNING: aCombinedChildren might be the same rect as aOverflowArea.
    // It may have been destroyed now. Don't use it below.

    // aOverflowArea is frame U children here so there can't be overflow (?)
    mState &= ~NS_FRAME_OUTSIDE_CHILDREN;
  }
}

Comment 32

[Adam D. Moss]

Attachment: patch roughly updated to trunk

The previous patch has started to seriously bitrot.  This patch updates it
against the trunk; I hand-applied several hunks, there's a few I'm not sure
about, and one that I've left out entirely (but #if 0'd).  You can see my
'uncertainty' points in the patch where I've added a #warning ADAM

This patch doesn't include the reshuffle suggested in my previous comment.

This compiles and runs.  I can't verify that it works as advertised (don't have
a debug / paint-flashing build).

Comment 33

[Adam D. Moss]

On further inspection I think that all of my rearrangement of
nsFrame::ComputeOverflowArea() is bogus for various reasons, with the possible
exception of the #if0 section move.

Comment 34

[Adam D. Moss]

Attachment: suggested replacement for nsFrame::ComputeOverflowArea()

Okay, I was lieing again.  I think we can still safely save a UnionRect()
operation in the HasView() case.  Big whoop.

Comment 35

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

Some things that this patch did have already been done by bug 79315 or (maybe)
will be done shortly by bug 173277.  Also, nsIFrame::HasView already exists.

I think the slowdown may have been partly due to attempting to partly fix bug
197581, if my memory is correct.

Comment 36

[Andrew Hagen]

No real activity here for several months.

Could we get a status update?

Comment 37

[Carlos Alén Silva]

Is the patch still valid?

Comment 38

[Wayne Mery (:wsmwk)]

(In reply to Carlos Alén Silva from comment #37)
> Is the patch still valid?

given comment 35, I doubt it