Last Comment Bug 190193 - Appearing scrollbars causing full screen repaints
: Appearing scrollbars causing full screen repaints
Status: RESOLVED FIXED
[fix][backed out of 1.4 final]
: perf, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Windows XP
: -- major (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
: Hixie (not reading bugmail)
Mentors:
resource:///res/samples/test13.html
Depends on:
Blocks: 21762 194638
  Show dependency treegraph
 
Reported: 2003-01-22 14:51 PST by Markus Hübner
Modified: 2005-02-14 18:46 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (609 bytes, text/html)
2003-01-22 20:08 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
obvious patch (1.65 KB, patch)
2003-01-22 20:26 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review

Description Markus Hübner 2003-01-22 14:51:03 PST
When viewing the dhtml viewer-demo things are fine as long as the animated 
elements are somewhere in the middle of the window. But when they move to the 
outside of a window and scrollbars appears the animation is very choppy.
I'd say that mozilla is doing a full screen repaint that is causing this 
slowdown.
This is actually happening in Mozilla for quite some time now - talking to roc 
on IRC he said Simon has been working on that already.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-01-22 14:52:41 PST
The text-entry bugs that smfr has been working on MAY be related. I'll
investigate this.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-01-22 20:08:17 PST
Created attachment 112364 [details]
Testcase

This testcase shows something interesting if you turn on paint flashing. When
the box moves from left to right, as soon as a horizontal scrollbar appears we
do a full screen repaint every time. (This is presumably a source of the
slowdown.) But when the box moves from top to bottom, when a vertical scrollbar
appears we repaint the whole window once but we don't do any more full repaints
until the box reaches the bottom of its cycle, moves to the top and the
scrollbar disappears. So  top to bottom motion isn't really a problem.
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-01-22 20:17:36 PST
The difference is probably due to the code in
nsContainerFrame::SyncFrameViewAfterReflow, which forces a full repaint when the
scrolled view changes width. It must be time to just whack this for good.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-01-22 20:26:56 PST
Created attachment 112365 [details] [diff] [review]
obvious patch
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-01-22 20:35:28 PST
So I tried the patch and it does fix the issue.

Also, to my surprise, I didn't find any rendering regressions in the small
amount of testing I did. dhtmlcentral.com, other random browsing, editing some
text in Composer, random XUL, all seem to work.

sfraser: this also fixes *some* of the "typing in a scrolled textarea
invalidates the whole thing" problem. With this patch, when the textarea is
scrolled we don't invalidate the whole thing, we just invalidate the typed line,
the line before, and (unfortunately) any part of the textarea content that is
below the level where the content would end if the textarea was scrolled all the
way to the top.

We should probably consider checking this in when 1.4a opens. It's got to be a
performance win.
Comment 6 Simon Fraser 2003-01-23 11:38:42 PST
This change worries me a bit, though I don't know the code well enough to say
what might regress.

How hard would it be to follow dbaron's suggestion, and move view resizing out
of reflow, doing it in a separate pass?
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-01-23 11:52:42 PST
FWIW, moving view sizing / positioning out of reflow would really only be a
workaround for the fact that we do min/max width computation in the same Reflow
function.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-01-23 12:23:37 PST
We can't take view sizing and positioning out of reflow unless we want to start
storing the combinedArea in every frame. I don't think it's worth it.

What we probably should do is apply this patch, and then in the future go even
further and make it so that moving and sizing views doesn't invalidate anything
at all, i.e., we force frames to do all invalidation.

sfraser: I'm worried about regressions too, but I think this is a case where the
code is clearly moving in the right direction, any regressions are not obvious
and won't be blockers, and by checking in at the beginning of 1.4a we'll have
the best chance to find any that do occur.
Comment 9 Roland Mainz 2003-01-24 11:16:23 PST
Robert O'Callahan wrote:
> We should probably consider checking this in when 1.4a opens. It's got to be a
> performance win.

What about asking for approval for 1.3beta instead ? Mozilla's scrollbar
handling and rendering (both rendering the scrollbar itself and all the
"sideeffects") s*cks a lot - and this patch seems to cure some of these 
performance issues...
Comment 10 Markus Hübner 2003-02-15 06:05:59 PST
Can we get this into 1.3 final?
As Roland pointed out this would considerably help major scrollbar & sideeffects
perf issues.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-02-15 09:08:25 PST
No. We don't do performance work for final releases. It's not worth the risk.
Comment 12 Roland Mainz 2003-02-16 14:59:21 PST
Markus Hübner wrote:
> Can we get this into 1.3 final?

No, I already discussed that with roc+moz on IRC.

> As Roland pointed out this would considerably help major scrollbar & 
> sideeffects perf issues.

... but I doubt this patch kills more than ~~~1/10 of the "Mozilla's scrollbar
rendering is (horribly) inefficient"-issues. For example - for each mouse-move
the whole scrollbar widget redraws itself instead of redrawing the thumb only
(this is bug 175411 ("XUL scrollbar rendering is _extremely_ inefficient") - and
using the OS native scrollbar widgets is not much better due other issues... ;-(
).
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-02-22 14:24:14 PST
Fix checked in.
Comment 14 (not reading, please use seth@sspitzer.org instead) 2003-05-29 20:49:42 PDT
note, this caused a regression, see bug #194638
Comment 15 (not reading, please use seth@sspitzer.org instead) 2003-05-30 13:49:43 PDT
fyi, this fix is on the trunk, but not part of 1.4 final.

Note You need to log in before you can comment on or make changes to this bug.