Closed Bug 170330 Opened 22 years ago Closed 12 years ago

Excessive repainting during reflow

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: roc, Unassigned)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [has review][wgate])

Attachments

(4 files, 1 obsolete file)

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?
Attached file Just a silly testcase (deleted) —
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.
Keywords: perf, testcase
Adding bug 42657 so it will get checked when this is resolved.
Blocks: 42657
is that the right bug number?
Attached patch fix (obsolete) (deleted) — Splinter Review
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.
Blocks: 46257
Comment on attachment 101876 [details] [diff] [review] fix r=dbaron
Attachment #101876 - Flags: review+
Keywords: mozilla1.3, patch
Whiteboard: [has review]
roc: Are you planning to land this during 1.3alpha?
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
Closed: 22 years ago
Resolution: --- → FIXED
I backed this out because it appears to have caused a Tp regression.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #101876 - Flags: approval1.3a+
Attached patch New patch (deleted) — Splinter Review
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 :-(.
Blocks: 21762
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
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.
minusing per email thread w/ roc.
Keywords: topembedtopembed-
If this is off-base, I apologize in advance. Is this bug related to bug 84920?
Blocks: 186465
Concerning Bob's input in comment #21 - what are we intending to do?
Blocks: 117601
Blocks: 136688
Blocks: 107746
Keywords: helpwanted
Blocks: 203448
adt: nsbeta1-
Keywords: nsbeta1nsbeta1-
Keywords: mozilla1.3
Whiteboard: [has review] → [has review][wgate]
Is the patch still okay? I expect that it's bit-rotted by now.
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.
Blocks: 79315
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; } }
Attached patch patch roughly updated to trunk (deleted) — Splinter Review
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).
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.
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.
No real activity here for several months. Could we get a status update?
QA Contact: chrispetersen → layout
Assignee: roc → nobody
Is the patch still valid?
(In reply to Carlos Alén Silva from comment #37) > Is the patch still valid? given comment 35, I doubt it
Status: REOPENED → RESOLVED
Closed: 22 years ago12 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: