Excessive repainting during reflow

RESOLVED WORKSFORME

Status

()

Core
Layout
RESOLVED WORKSFORME
16 years ago
4 years ago

People

(Reporter: roc, Unassigned)

Tracking

(Blocks: 3 bugs, 4 keywords)

Trunk
helpwanted, perf, testcase, topembed-
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [has review][wgate])

Attachments

(4 attachments, 1 obsolete attachment)

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?
Created attachment 100248 [details]
Just a silly testcase

Sounds like the proposed change will make our behavior in this testcase much
better...
Oh, and this might fix some other bugs too.
Blocks: 45597, 155178
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.

Updated

16 years ago
Keywords: perf, testcase
Adding bug 42657 so it will get checked when this is resolved.
Blocks: 42657
is that the right bug number?
Created attachment 101876 [details] [diff] [review]
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.
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.

Updated

16 years ago
Keywords: mozilla1.3, patch

Updated

16 years ago
Whiteboard: [has review]
roc:  Are you planning to land this during 1.3alpha?

Comment 11

16 years ago
Comment on attachment 101876 [details] [diff] [review]
fix

sr=kin@netscape.com
Attachment #101876 - Flags: superreview+

Comment 12

16 years ago
Comment on attachment 101876 [details] [diff] [review]
fix

a=asa for checkin to 1.3a
Attachment #101876 - Flags: approval1.3a? → approval1.3a+
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
I backed this out because it appears to have caused a Tp regression.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

16 years ago
Attachment #101876 - Flags: approval1.3a+
Created attachment 108854 [details] [diff] [review]
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.
Attachment #101876 - Attachment is obsolete: true
Comment on attachment 108854 [details] [diff] [review]
New patch

need reviews for new version of patch
Attachment #108854 - Flags: superreview?(kin)
Attachment #108854 - Flags: review?(dbaron)
Comment on attachment 108854 [details] [diff] [review]
New patch

you have an #if 0 in there, is that intentional?
Yes it's intentional.

> The 'outline' stuff is #ifdefed out because we don't really need it at
> this stage
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
Attachment #108854 - Flags: superreview?(kin)
Attachment #108854 - Flags: superreview+
Attachment #108854 - Flags: review?(dbaron)
Attachment #108854 - Flags: review+
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 :-(.

Updated

16 years ago
Blocks: 21762

Comment 21

16 years ago
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.
Keywords: nsbeta1, topembed

Comment 22

16 years ago
Discussed in bug triage meeting.  Any ideas about next move?  Please advise in
this bug.
I've been away. I still don't know what is causing the Tp problem.

Comment 24

16 years ago
minusing per email thread w/ roc.
Keywords: topembed → topembed-

Comment 25

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

Is this bug related to bug 84920?

Updated

16 years ago
Blocks: 186465

Comment 27

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

Updated

16 years ago
Blocks: 117601

Updated

16 years ago
Blocks: 136688

Updated

16 years ago
Blocks: 107746

Updated

15 years ago
Keywords: helpwanted

Updated

15 years ago
Blocks: 203448

Comment 28

15 years ago
adt: nsbeta1-
Keywords: nsbeta1 → nsbeta1-

Updated

15 years ago
Keywords: mozilla1.3
Whiteboard: [has review] → [has review][wgate]

Comment 29

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

Comment 30

15 years ago
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

15 years ago
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

15 years ago
Created attachment 129499 [details] [diff] [review]
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

15 years ago
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

15 years ago
Created attachment 129500 [details]
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.
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

14 years ago
No real activity here for several months.

Could we get a status update?
QA Contact: chrispetersen → layout
Assignee: roc → nobody

Comment 37

8 years ago
Is the patch still valid?

Comment 38

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

given comment 35, I doubt it
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.