190193 - Appearing scrollbars causing full screen repaints

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Markus Hübner]

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) (email my personal email if necessary)]

The text-entry bugs that smfr has been working on MAY be related. I'll
investigate this.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

Attachment: obvious patch

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 [no longer active]]

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-5, no longer working on Mozilla)]

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) (email my personal email if necessary)]

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]

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]

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) (email my personal email if necessary)]

No. We don't do performance work for final releases. It's not worth the risk.

Comment 12

[Roland Mainz]

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) (email my personal email if necessary)]

Fix checked in.

Comment 14

[(not reading, please use [e-mail] instead)]

note, this caused a regression, see bug #194638

Comment 15

[(not reading, please use [e-mail] instead)]

fyi, this fix is on the trunk, but not part of 1.4 final.