Closed
Bug 170330
Opened 22 years ago
Closed 12 years ago
Excessive repainting during reflow
Categories
(Core :: Layout, defect)
Core
Layout
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)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details |
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•22 years ago
|
||
Sounds like the proposed change will make our behavior in this testcase much
better...
Comment 2•22 years ago
|
||
Oh, and this might fix some other bugs too.
Reporter | ||
Comment 3•22 years ago
|
||
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•22 years ago
|
Comment 4•22 years ago
|
||
Adding bug 42657 so it will get checked when this is resolved.
Blocks: 42657
Reporter | ||
Comment 5•22 years ago
|
||
is that the right bug number?
Reporter | ||
Comment 6•22 years ago
|
||
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.
Reporter | ||
Comment 7•22 years ago
|
||
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•22 years ago
|
||
Comment on attachment 101876 [details] [diff] [review]
fix
r=dbaron
Attachment #101876 -
Flags: review+
Updated•22 years ago
|
Keywords: mozilla1.3,
patch
Updated•22 years ago
|
Whiteboard: [has review]
Comment 9•22 years ago
|
||
roc: Are you planning to land this during 1.3alpha?
Reporter | ||
Comment 10•22 years ago
|
||
Yes!
Comment 11•22 years ago
|
||
Attachment #101876 -
Flags: superreview+
Reporter | ||
Updated•22 years ago
|
Attachment #101876 -
Flags: approval1.3a?
Comment 12•22 years ago
|
||
Comment on attachment 101876 [details] [diff] [review]
fix
a=asa for checkin to 1.3a
Attachment #101876 -
Flags: approval1.3a? → approval1.3a+
Reporter | ||
Comment 13•22 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 14•22 years ago
|
||
I backed this out because it appears to have caused a Tp regression.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•22 years ago
|
Attachment #101876 -
Flags: approval1.3a+
Reporter | ||
Comment 15•22 years ago
|
||
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
Reporter | ||
Comment 16•22 years ago
|
||
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 17•22 years ago
|
||
Comment on attachment 108854 [details] [diff] [review]
New patch
you have an #if 0 in there, is that intentional?
Reporter | ||
Comment 18•22 years ago
|
||
Yes it's intentional.
> The 'outline' stuff is #ifdefed out because we don't really need it at
> this stage
Comment 19•22 years ago
|
||
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+
Reporter | ||
Comment 20•22 years ago
|
||
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•22 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.
Comment 22•22 years ago
|
||
Discussed in bug triage meeting. Any ideas about next move? Please advise in
this bug.
Reporter | ||
Comment 23•22 years ago
|
||
I've been away. I still don't know what is causing the Tp problem.
Comment 24•22 years ago
|
||
minusing per email thread w/ roc.
Comment 25•22 years ago
|
||
If this is off-base, I apologize in advance.
Is this bug related to bug 84920?
Reporter | ||
Comment 26•22 years ago
|
||
I doubt it.
Comment 27•22 years ago
|
||
Concerning Bob's input in comment #21 - what are we intending to do?
Updated•22 years ago
|
Keywords: helpwanted
Updated•22 years ago
|
Keywords: mozilla1.3
Whiteboard: [has review] → [has review][wgate]
Comment 29•22 years ago
|
||
Is the patch still okay? I expect that it's bit-rotted by now.
Comment 30•22 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•21 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•21 years ago
|
||
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•21 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•21 years ago
|
||
Okay, I was lieing again. I think we can still safely save a UnionRect()
operation in the HasView() case. Big whoop.
Comment 35•21 years ago
|
||
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•21 years ago
|
||
No real activity here for several months.
Could we get a status update?
Updated•15 years ago
|
QA Contact: chrispetersen → layout
Reporter | ||
Updated•15 years ago
|
Assignee: roc → nobody
Comment 37•14 years ago
|
||
Is the patch still valid?
Comment 38•12 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
Closed: 22 years ago → 12 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•