Closed
Bug 350148
Opened 18 years ago
Closed 18 years ago
scrolling regression in gtk1/gtk2 (worse in cairo) not noticed with windows
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: timeless, Assigned: roc)
References
(Depends on 1 open bug, )
Details
(Keywords: perf, regression)
Attachments
(8 files, 1 obsolete file)
(deleted),
application/zip
|
Details | |
(deleted),
application/x-bzip
|
Details | |
(deleted),
application/x-bzip
|
Details | |
(deleted),
application/x-bzip
|
Details | |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
amorphous steps to reproduce:
load something like gmail and use your mouse wheel to scroll up/down a lot.
pause, waiting for gecko to catch up, this takes a while.
steps to reproduce:
1. load jar:http://webwizardry.net/~timeless/317375-testcase.html.zip!/317375-testcase.html
(there's a .bz2 and a plain, if you'd rather a 1.5 or 35mb file instead of a 2.5mb file)
2. enter one of the following into the urlbar and press enter:
javascript:void (function(){for (var x=0; x<10000; x++)
window.scrollTo(0,x*10)})()
javascript:void (function(){for (var x=0; x<10000; x++)
window.scrollTo(0,document.height/2+(1-2*(x%2))*x*1000)})()
expected results:
if you use windows or a gtk1/gtk2 build from before bug 317375, gecko paints responsively/quickly, doesn't lag and actually looks nice (especially w/ the second js url where you get a heisenberg cat thumb running away from itself).
actual results:
on some systems the system's cpu is pegged and no painting happens until the end which takes longer to reach.
note: the regression window was chased using mozilla.org nightlies, and then the likely candidate (plus all its bustage fixes) was built and that showed the performance difference that was noticed between last year and now. general cairo performance is much worse than noncairo at the moment, so we ignored cairo.
windows doesn't seem to have any problems with this, not sure why.
Comment 1•18 years ago
|
||
Updated•18 years ago
|
Flags: blocking1.9?
Assignee | ||
Comment 2•18 years ago
|
||
Are we repainting the whole window after each scroll movement, post 317375, while we weren't before?
that seems to be a reasonable conclusion. note that i'm not in a position to answer this question and bz isn't cc'd, so i'm not sure to whom your question was addressed.
Assignee | ||
Comment 4•18 years ago
|
||
It's easy to see, just turn on paint flashing in a debug GTK2 build; set nglayout.debug.paint_flashing and then press capslock
Comment 5•18 years ago
|
||
fullafterjprof.html - many scrolls on www.gmail.com after bug 317375
fullbeforejprof.html - many scrolls on www.gmail.com before bug 317375
shortafterjprof.html - short scroll on www.gmail.com after bug 317375
shortbeforejprof.html - short scroll on www.gmail.com before bug 317375
Comment 6•18 years ago
|
||
On the testcase I profiled, we only repaint the newly-exposed strip. We also blit the widget and all (see the control flow in the profile).
Comment 7•18 years ago
|
||
Attachment #235646 -
Attachment is obsolete: true
Comment 8•18 years ago
|
||
Comment 9•18 years ago
|
||
Assignee | ||
Comment 10•18 years ago
|
||
We're spending a lot of time in ComputeRepaintRegionForCopy, to see what can and can't be scrolled via bitblt. This now traverses the frame tree where it used to just traverse the view tree.
Assignee | ||
Comment 11•18 years ago
|
||
The testcase contains big tables. For any table intersecting the displayed area, we traverse all the rows to while constructing the display list. We could avoid that using a cursor optimization like the block's line cursor, and that's probably a good thing to do. I don't know if it will be enough to resolve this bug.
Status: NEW → ASSIGNED
Comment 12•18 years ago
|
||
Ah, indeed. The view tree would be smaller by far here. And the large table thing is basically bug 241796, I think. I suspect that using a cursor would help some. Bailing out of the row traversal once we're "far enough down" if we can (e.g. if none of the rows overflow upwards, since unlike row ends row starts are nicely increasing -- cells don't span up) would help the rest of the way.
Depends on: 241796
Assignee | ||
Comment 13•18 years ago
|
||
(In reply to comment #12)
> Bailing out of the row traversal once we're "far enough down" if we
> can (e.g. if none of the rows overflow upwards, since unlike row ends row
> starts are nicely increasing -- cells don't span up) would help the rest of the
> way.
That'll be included in the cursor optimization one way or another.
Comment 14•18 years ago
|
||
(In reply to comment #11)
> The testcase contains big tables. For any table intersecting the displayed
Exactly testcase it is Gmail.com mail inbox, without chats, with 100 emails on page, Full screen.
Assignee | ||
Comment 15•18 years ago
|
||
If everything is actually visible, table row cursor optimizations won't help :-(
Assignee | ||
Comment 16•18 years ago
|
||
But then I guess you wouldn't be scrolling...
Assignee | ||
Comment 17•18 years ago
|
||
I've implemented the table row cursor but it doesn't seem to help at all :-(
Assignee | ||
Comment 18•18 years ago
|
||
(In reply to comment #7)
> Created an attachment (id=235669) [edit]
> Updated after Jprof more hit count, JP_RTC_HZ=8192
This is a "big scroll", right? so each scroll operation repaints the entire window because the visible areas before and after scrolling don't overlap?
Assignee | ||
Comment 19•18 years ago
|
||
If that's the case, then I'm confused, because ComputeRepaintRegionForCopy should not be showing up in the profile as anything significant, because it should be really fast when the incoming rectangle, the intersection of the areas visible before and after, is empty...
Reporter | ||
Comment 20•18 years ago
|
||
the original problem involves using a mouse wheel or similar device, i.e. you're never scrolling a full page, you always have some slice from the previous view onscreen. some of my js testcases didn't honor that faithfully.
note that 100 email "rows" (6 lines per message because of wrapping) will never fit on a 420px high screen if you expect to read it (10pt font) and have a couple of rows for a header (say 250px) and a footer (250px).
My guess is that there are about 36 screens (this is based on a shipping product that isn't gecko) so something like 15120px tall.
i'm not sure what normal jitter is, but it's probably 10-60px. The most common user input does not involve using scrollbars, so you'd *never* get full painting, at best you'd painting 95% fresh and having to deal w/ 5% that moved from bottom to top or top to bottom.
Assignee | ||
Comment 21•18 years ago
|
||
Okay, so only the "small scroll" test is really relevant. And it's where we do trigger the extra analysis pass.
I may be able to somehow combine the analysis pass with the construction of the display list for actual painting, into a single display list construction. It would add complexity which is why I didn't do it initially.
Comment 22•18 years ago
|
||
OK, so bonsai breaks up its output into tables with 20-70 rows in them. So the testcase in the URL bar doesn't benefit that much from the table cursor (well, sometimes we only have to walk 10 rows instead of 50... but sometimes not, since a lot of rows can fit on screen at once, and given that bug 240275 is fixed the simple act of walking rows is not too bad, since we do nothing for them).
Reporter | ||
Comment 23•18 years ago
|
||
bz: well, gmail only has 100 rows which isn't really that much more than 50, and there's only one such table. it's just very hard to use gmail as a testcase, and it seemed like bonsai emulated gmail tolerably well.
Comment 24•18 years ago
|
||
Did bonsai regress too, in your testing?
Comment 25•18 years ago
|
||
I have tried to investigate what function is slowly....
Way:
nsViewManager::CanScrollWithBitBlt->
nsLayoutUtils::ComputeRepaintRegionForCopy->
AddItemsToRegion->
clip.IntersectRect(aClipRect, NS_STATIC_CAST(nsDisplayClip*, item)->GetClipRect());
All this parts and calls can be commented, and I can't see any visible issues after disabling functionality on this way.
also I have tried to insert after
http://lxr.mozilla.org/mozilla/source/layout/base/nsLayoutUtils.cpp#751:
....
clip.height = 0;
....
It scrolling also works very fast without visible issues...
May be ROc can explain what problem here?
Comment 26•18 years ago
|
||
ALso i have check height values before and after Intersect function:
nsRect::IntersectRectTest: height = 0, temp = 13515, y = 0
nsRect::IntersectRectTest: after height = 13515
Assignee | ||
Comment 27•18 years ago
|
||
So if you just comment out the call from ComputeRepaintRegionForCopy to AddItemsToRegion, the problem goes away? That is very interesting.
Assignee | ||
Comment 28•18 years ago
|
||
Actually I don't believe that because AddItemsToRegion has only 5 hits in bug317375_short_rtc/shortafterjprof.html
Comment 29•18 years ago
|
||
(In reply to comment #28)
> Actually I don't believe that because AddItemsToRegion has only 5 hits in
> bug317375_short_rtc/shortafterjprof.html
>
Problem not in hits on this functions, ... I think problem in wrong calculated height.... and paint actions that try to paint this calculations....
see
........................
also I have tried to insert after
http://lxr.mozilla.org/mozilla/source/layout/base/nsLayoutUtils.cpp#751:
....
clip.height = 0;
....
It scrolling also works very fast without visible issues...
.........................
When height = 0 all works fast, when it = 13515 then slow...
Assignee | ||
Comment 30•18 years ago
|
||
Okay. Now that I've finally got around to checking, it turns out that on trunk we're repainting the whole window when I scroll gmail. I think that's the problem, and I should be able to fix it.
That bonsai testcase was a total red herring, guys.
Assignee | ||
Comment 31•18 years ago
|
||
This fixes the problem at hand. It's very simple, we were concluding that Gmail's all-enclosing IFRAME had a border which needed to be painted, the border rect intersects the area being scrolled, but isn't being scrolled itself, so ComputeRepaintRegionForCopy says "need to repaint everything".
In fact there is no border on that IFRAME. The border-style is set to something other than 'none', but the width is zero. The solution is trivial, just check the border width instead of the style. Other code ensures that when the style is none, the computed border width is zero.
Attachment #238361 -
Flags: superreview?(dbaron)
Attachment #238361 -
Flags: review?(dbaron)
Comment 32•18 years ago
|
||
Would:
return GetStyleBorder()->GetBorder() != nsMargin(0, 0, 0, 0);
be better? If not really, should probably use NS_FOR_CSS_SIDES here instead of manual for loop.
Comment 33•18 years ago
|
||
(In reply to comment #31)
> Created an attachment (id=238361) [edit]
> fix border visibility check
I have tested this patch, it works some faster, but not so fast as with clip.height = 0; or with commented lines ...
May be something else tries to redraw there?.
Assignee | ||
Comment 34•18 years ago
|
||
(In reply to comment #32)
> Would:
>
> return GetStyleBorder()->GetBorder() != nsMargin(0, 0, 0, 0);
>
> be better?
Yes.
Assignee | ||
Comment 35•18 years ago
|
||
(In reply to comment #33)
> (In reply to comment #31)
> > Created an attachment (id=238361) [edit]
> > fix border visibility check
>
> I have tested this patch, it works some faster, but not so fast as with
> clip.height = 0; or with commented lines ...
>
> May be something else tries to redraw there?.
Enable paint flashing and see if there's any painting difference. I'm not sure why there would be.
I do have some ideas for to make things faster still, I'll get to that tomorrow.
Comment 36•18 years ago
|
||
Comment on attachment 238361 [details] [diff] [review]
fix border visibility check
It'd probably be good to use NS_FOR_CSS_SIDES for the for-loop.
r+sr=dbaron
Attachment #238361 -
Flags: superreview?(dbaron)
Attachment #238361 -
Flags: superreview+
Attachment #238361 -
Flags: review?(dbaron)
Attachment #238361 -
Flags: review+
Assignee | ||
Comment 37•18 years ago
|
||
I'll just go with bz's "return GetStyleBorder()->GetBorder() != nsMargin(0, 0, 0, 0);"
Assignee | ||
Comment 38•18 years ago
|
||
If gmail did use a border on its IFRAME, the previous patch would of course be defeated. This testcase tests such a situation. Adding an outline would also cause the same sort of problem.
Assignee | ||
Comment 39•18 years ago
|
||
This patch fixes that last testcase. We observe that when the visible region's bounds are wholly contained inside the inner edge of a border or outline, the border or outline is not in fact visible. Then when we do our scroll analysis, such a border or outline will not be considered when we look for non-moving but visible content that could interfere with the bitblt.
Attachment #238457 -
Flags: superreview?(dbaron)
Attachment #238457 -
Flags: review?(dbaron)
Assignee | ||
Comment 40•18 years ago
|
||
We should take both patches.
Assignee | ||
Comment 41•18 years ago
|
||
Here's another easy optimization that will help a lot on all kinds of pages. When we build the display list for scroll analysis, we descend into the moving frame and its children only to look for content whose rendering depends on position and therefore can't be safely bitblitted. Currently this is only background-attachment:fixed content. We can keep a flag in the prescontext to record whether we've ever rendered any such content in this prescontext; if we haven't, there is no need to descend into the moving frame when building a display list for scroll analysis. This will hugely speed up ComputeRepaintRegionForCopy in cases when background-attachment:fixed is not used and we don't have to descend to find the placeholders for visible positioned elements ... typically most of the content visible in the area to be scrolled is in fact the scrolled element and its contents, so skipping traversal of that is a big win.
It slightly refactors code in BuildDisplayListForChild to share the test of NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO, and in the process fixes what I think is a possible bug where we might fail to render positioned elements whose placeholders are in something visibility:collapsed.
Attachment #238475 -
Flags: superreview?(dbaron)
Attachment #238475 -
Flags: review?(dbaron)
Assignee | ||
Comment 42•18 years ago
|
||
Altogether I hope these patches will put this bug to rest.
But I still don't understand why hacking AddItemsToRegion is making a difference after the first patch, and that worries me. In my tests, after the first patch, AddItemsToRegion isn't adding anything to the region, and it's not taking much time itself, so changing it in any way shouldn't be helping.
Comment 43•18 years ago
|
||
(In reply to comment #42)
> Altogether I hope these patches will put this bug to rest.
>
> But I still don't understand why hacking AddItemsToRegion is making a
> difference after the first patch, and that worries me. In my tests, after the
> first patch, AddItemsToRegion isn't adding anything to the region, and it's not
> taking much time itself, so changing it in any way shouldn't be helping.
>
I have tested all pathes together ... works very good, I think we can close this bug after applying all pathes...
PS: May be first patch can be implemented as
nsFrame.h:
...........
inline PRBool HasBorder... {
return GetStyleBorder()->GetBorder() != nsMargin(0, 0, 0, 0);
}
........
?
Comment 44•18 years ago
|
||
For the HasBorder thing, btw, do we want to have a static nsMargin that we compare to so that we don't have to keep creating nsMargins?
Assignee | ||
Comment 45•18 years ago
|
||
I hope it all just gets inlined away to a few compares with zero.
Reporter | ||
Comment 46•18 years ago
|
||
sorry. bonsai was slow and used to be faster, no one likes using google mail for testcases becuase it's too dynamic, so i tried something that seemed similar. i knew there was a risk it was iframe related :(.
Assignee | ||
Comment 47•18 years ago
|
||
The last patch will help the bonsai testcase as well, I hope.
Comment 48•18 years ago
|
||
Comment on attachment 238457 [details] [diff] [review]
fix borders and outlines interfering with scrolled children
This seems reasonable, although two ideas that you can take or leave:
* it might be nicer for these methods to call the base-class method, i.e., begin with:
if (!nsDisplayItem::OptimizeVisibility(...))
return PR_FALSE;
and then do the one bit that they add before returning true (especially if you need to add to the base class method in the future). That said, what you did is *probably* faster...
* I think you might be bailing too easily on the handling of border/outline-radius and especially outline-offset. Inflating by the outline-offset should be trivial; deflating by the largest or larger-applicable border/outline-radius shouldn't be too hard either.
Attachment #238457 -
Flags: superreview?(dbaron)
Attachment #238457 -
Flags: superreview+
Attachment #238457 -
Flags: review?(dbaron)
Attachment #238457 -
Flags: review+
Comment 49•18 years ago
|
||
Comment on attachment 238475 [details] [diff] [review]
prevent descending into the scrolled frame during BuildDisplayList for analysis, in most cases
You need to initialize mRenderedPositionVaryingContent to PR_FALSE in nsPresContext's constructor.
Why is NS_STYLE_VISIBILITY_COLLAPSE treated specially? (I know it's just code you're moving.) It normally (i.e., for most frame types) means the same thing as hidden. My initial intuition is that that test should be removed entirely.
Where's the test that this code is used for building the display list when doing scroll analysis? Or is it used more generally?
Assignee | ||
Comment 50•18 years ago
|
||
(In reply to comment #49)
> (From update of attachment 238475 [details] [diff] [review] [edit])
> You need to initialize mRenderedPositionVaryingContent to PR_FALSE in
> nsPresContext's constructor.
It's NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW so I don't think I do.
> Why is NS_STYLE_VISIBILITY_COLLAPSE treated specially? (I know it's just code
> you're moving.) It normally (i.e., for most frame types) means the same thing
> as hidden. My initial intuition is that that test should be removed entirely.
Will do.
> Where's the test that this code is used for building the display list when
> doing scroll analysis? Or is it used more generally?
When we're not doing scroll analysis, "if (aBuilder->GetRootMovingFrame() == aChild" will fail because GetRootMovingFrame() returns nsnull.
Assignee | ||
Comment 51•18 years ago
|
||
I'll explain the latter with a comment there and in nsDisplayList.h for GetRootMovingFrame.
Comment 52•18 years ago
|
||
Would this optimization prevent using BitBlt if the moving frame has a solid color background, since you never actually look at its background? (I'm assuming the moving frame would be the scrolled frame inside a scrollframe. Is that correct?)
Comment 53•18 years ago
|
||
Except I suppose with the current CSS spec, the moving frame would never have a background at all; it would be the scrollframe that has the background. I suppose I should look at how the scroll analysis works, since it doesn't currently make sense to me.
Comment 54•18 years ago
|
||
Comment on attachment 238475 [details] [diff] [review]
prevent descending into the scrolled frame during BuildDisplayList for analysis, in most cases
OK, this makes a little more sense after reading the comments in ComputeRepaintRegionForCopy.
r+sr=dbaron with the visibility:collapse check removed (are there any other similar checks elsewhere?)
Attachment #238475 -
Flags: superreview?(dbaron)
Attachment #238475 -
Flags: superreview+
Attachment #238475 -
Flags: review?(dbaron)
Attachment #238475 -
Flags: review+
Assignee | ||
Comment 55•18 years ago
|
||
(In reply to comment #53)
> Except I suppose with the current CSS spec, the moving frame would never have
> a background at all; it would be the scrollframe that has the background.
Right.
You're right, this could be a problem if we used ComputeRepaintRegionForCopy more generally (like if we used it to accelerate positioning changes). But at least it would be a problem of performance, not correctness. I could fix it by changing
+ if (aBuilder->GetRootMovingFrame() == aChild
to
+ if (aBuilder->GetRootMovingFrame() == this
Let me know if that's OK.
(In reply to comment #54)
> (From update of attachment 238475 [details] [diff] [review] [edit])
> OK, this makes a little more sense after reading the comments in
> ComputeRepaintRegionForCopy.
>
> r+sr=dbaron with the visibility:collapse check removed (are there any other
> similar checks elsewhere?)
Yes, here:
http://lxr.mozilla.org/mozilla/source/layout/generic/nsFrame.cpp#1152
I'll remove that too.
Assignee | ||
Comment 56•18 years ago
|
||
(In reply to comment #48)
> (From update of attachment 238457 [details] [diff] [review] [edit])
> This seems reasonable, although two ideas that you can take or leave:
>
> * it might be nicer for these methods to call the base-class method, i.e.,
> begin with:
>
> if (!nsDisplayItem::OptimizeVisibility(...))
> return PR_FALSE;
>
> and then do the one bit that they add before returning true (especially if you
> need to add to the base class method in the future). That said, what you did
> is *probably* faster...
I'll do this.
> * I think you might be bailing too easily on the handling of
> border/outline-radius and especially outline-offset. Inflating by the
> outline-offset should be trivial; deflating by the largest or larger-applicable
> border/outline-radius shouldn't be too hard either.
I don't want to add additional code to handle these very rare cases, so I'll skip this.
Assignee | ||
Comment 57•18 years ago
|
||
> Let me know if that's OK.
I took the liberty of doing that.
All patches checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Flags: blocking1.9? → in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•