19256 - Excess frame redrawing during incremental reflow

Status: VERIFIED FIXED

(Core Graveyard :: GFX, defect, P2)

Description

[Simon Fraser [no longer active]]

While loading a long, simple document (just <Hn> and <P>), we continually redraw
frames that are visible in the window, while appending content which only affects
frames that are not visible.

It seems like this should be optimized to not redraw visible frames which have
not been affected by appended content.

Note: turn off double-buffering to see the frames being redrawn.

Comment 1

[troy]

I don't know why you think this is layout redrawing frames that aren't visible.
This is a view manager bug introduced in revision 1.31

It used to just update the newly exposed areas of the view, and now it has been
changed to update the entire view. Both the old and new areas

That means that as the document loads and the height of the scrolled view
increases the entire visible area is painted for each and every incremental
reflow

Comment 2

[Patrick C. Beard]

Troy, this should no longer be happening. I did accidentally add some code in

v1.31 of nsViewManager.cpp that would invalidate the entire parent view. I've

since changed it to only invalidate the view in question. There are two issues:



1. If a view is moved, the old bounds and new bounds are invalidated, so any

backgrounds, or exposed overlapping views are redrawn in the old position.



2. When a view is resized, the bounding box of the old and new sizes is

invalidated, for similar reasons to 1.



One other issue, we used to only invalidate the newly exposed area of a view

when it was resized. Does layout properly invalidate the area that was already

in view if a reflow occurs? Should the view manager only be invalidating

areas that are exposed?



I believe the state of things is better than before, but perhaps we can do

even better.

Comment 3

[Simon Fraser [no longer active]]

I don't see any improvement. Reopening

Comment 4

[Simon Fraser [no longer active]]

To see the problem, turn OFF double buffering (developer only), and load a
longish HTML document that is just a bunch of <p>s. Turn off your content timer
pref, so that you get lots of reflows (so the problem is more apparent):

user_pref("content.notify.ontimer", false);

Now load the document. Note that each time content is appended, we repaint the
visible frames, even though they have not changed.

Comment 5

[Simon Fraser [no longer active]]

The problem here is that nsViewManager::ResizeView() invalidates the entire view
when the size fo the view changes. It needs to be smarter, figure out of the
widget of the view has changed, and only invalidate the new area if so.

Comment 6

[Simon Fraser [no longer active]]

Here's a diff (unified, line numbers not supplied becase mine will be a little
off). This changes will cause us to only invalidate the new area of the view if
the width does not change. This assumes that adding to the bottom of a view can
never change what is displayed in the existing area.

 #if 1
-		// refresh the bounding box of old and new areas.
-		nscoord maxWidth = (oldWidth < width ? width : oldWidth);
-		nscoord maxHeight = (oldHeight < height ? height : oldHeight);
-		nsRect boundingArea(x, y, maxWidth, maxHeight);
-		UpdateView(parentView, boundingArea, NS_VMREFRESH_NO_SYNC);
+		// did the width change?
+		if (oldWidth != width)
+		{
+			// refresh the bounding box of old and new areas.
+			nscoord maxWidth = (oldWidth < width ? width : oldWidth);
+			nscoord maxHeight = (oldHeight < height ? height : oldHeight);
+			nsRect boundingArea(x, y, maxWidth, maxHeight);
+			UpdateView(parentView, boundingArea, NS_VMREFRESH_NO_SYNC);
+		}
+		else if (height > oldHeight)	// if we got longer
+		{
+			// refresh the new area
+			nsRect newArea(x, y + oldHeight, width, y + height);
+			UpdateView(parentView, newArea, NS_VMREFRESH_NO_SYNC);
+		}
+
 #else
 		// brute force, invalidate old and new areas. I don't understand

Comment 7

[Simon Fraser [no longer active]]

With this change, and the content.timer pref OFF (hence more reflows), loading a
test page drops from 9 to 5 seconds, because of the time saved painting.

Comment 8

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

I'll test thing out saturday / sunday on linux, so we can get it into
m12.

Comment 9

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

patch is working fine on linux, I've been running with it since last night, with
no problems.

sites I've been testing, to see the improvements:

http://tinderbox.mozilla.org/SeaMonkey/warnings.html
http://www.slashdot.org
http://cvs-mirror.mozilla.org/webtools/tinderbox/showbuilds.cgi?tree=SeaMonkey&hours=12&nocrap=1

Comment 10

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

another good test page:

http://cvs-mirror.mozilla.org/webtools/bonsai/showcheckins.cgi?&treeid=SeaMonkey

Comment 11

[Adam D. Moss]

(Reminder!)

Is the patch now in and the bug effectively closed?
--Adam

Comment 12

[Simon Fraser [no longer active]]

No, the patch has not been checked in. The problem is that the patch makes
assumptions about the way layout works (that, if the width of the view does not
change, you don't need to repaint the visible area), and there is no way for the
view manager to know if these assumptions are valid. Ideally, layout needs to
propagate that information back to the view system, or layout itself needs to do
smarter view invalidation when the view changes size.

Comment 13

[leger]

Moving "perf" to keyword field.  This is the are we use now :-)

Comment 14

[Patrick C. Beard]

Handing this to kevin, who's working on redraw optimizations.

Comment 15

[Kevin McCluskey (gone)]

Nominating for nsbeta3.

Comment 16

[Kevin McCluskey (gone)]

Marking nsbeta3+

Comment 17

[Kevin McCluskey (gone)]

I agree with Simon's analysis. The problem is that isn't clear whether resizing 
a view should cause only the newly exposed portion to be invalidated or the 
entire view should be invalidated. Layout sometimes assumes the entire view will 
be invalidated by the view system when the view changes size. Layout needs to be 
enhanced to pass a flag to view system to indicate whether all of the view or 
only the newly expanded portion of the view really needs to be invalidated. I 
can not see how to easily make this change within Layout. In general the view is 
simply sync'ed (i.e width, height) are made to match the dimensions of a frame. 
Currently, There isn't any info within the frames which indicates whether the 
reflow operation actually changed the appearance of only a portion of the view.

Marking nsbeta3-. Need to look at first thing, post RTM. This is really a layout 
issue, since it is in control of what gets invalidated.

Comment 18

[Asa Dotzler [:asa]]

adding keyword nominations

Comment 19

[Kevin McCluskey (gone)]

Set milestone to mozilla0.9

Comment 20

[Kevin McCluskey (gone)]

The layout code which calls ResizeView needs to determine if only the vertical
size is changing and if so it should resize the view without causing a repaint.
This will require adding an aPaint flag to ResizeView and modifying the layout
code to check for a height change.  It may also require determining if their are
any layout objects which depend on the vertical height of the document to
determine their height.

Comment 21

[Kevin McCluskey (gone)]

I hacked the viewmanager resize code to prevent the invalidate from happening on
view resizes and I ran jrgm's timing tests. It had no effect on average page
load time. 

Comment 22

[Kevin McCluskey (gone)]

Attachment: patch to containerframe and viewmanager to invalidate only the newly exposed region when the container changes only it's height

Comment 23

[Kevin McCluskey (gone)]

Added patch keyword

Comment 24

[Kevin McCluskey (gone)]

Added dependency on bug 63951 because the full-window invalidations which were
happening because of this bug were masking additional problems caused by bug 63951. 

Comment 25

[Kevin McCluskey (gone)]

patch 27067 puts things back to way they were originally. Originally, the
viewmanager used to only invalidate the newly exposed area when it was resized.
In the patch the container frame can specify this behavior by passing a new flag
to when the view is resized.

I have not seen update problems running with the patch.

Comment 26

[Kevin McCluskey (gone)]

Updated status.

Comment 27

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

The comment "//and left edge overlap" should read "//and bottom edge overlap".

If aView has a widget, then updating the parent view might not repaint anything 
if the child view is growing in size. In that case we'd need to update the child 
view instead (or as well). So to actually do what you're trying to do, I think 
you have to do this:

if (increasing width) { repaint right-hand strip in child; }
  else { repaint right-hand strip in parent; }
if (increasing height) { repaint bottom strip in child; }
  else { repaint bottom strip in parent; }

Forget about optimizing the corner where the bottom and right-hand strips 
overlap. The dirty rects are supposed to be merged into a single region anyway.

The change to nsContainerFrame.cpp looks like a grotesque hack. What reasonable 
argument can there be that "frameSize.width == width" is the correct condition? 
IMHO it should always be "TRUE", and if layout needs more repainting, it should 
do that explicitly somewhere else.

Comment 28

[Kevin McCluskey (gone)]

"If aView has a widget, then updating the parent view might not repaint anything
if the child view is growing in size. In that case we'd need to update the child
view instead (or as well). So to actually do..."

Good catch! I see there is call to InvalidateChildWidgets(widgetView,
widgetRect); in UpdateView which would take care of this, but its part of a
large chunk of code thats been #ifdef'ed out, probably when the code to batch
the invalidates was added.

I'll change the patch to explicitly invalidate the child if it has a widget in
the ResizeView method:


"Forget about optimizing the corner where the bottom and right-hand strips
overlap. The dirty rects are supposed to be merged into a single region anyway."

I agree that the dirty rects would be merged, but the optimization eliminates
some of the calls to platform's native invalidate which is expensive even if it
doesn't result in any extra painting. If only the width or height changes then
the native invalidate is suppressed by an optimization in
nsIViewManager::UpdateView which looks for 0 width or 0 height invalidates
(Which happen if only one dimension of the view is changed). This optimization
costs us nothing, the only difference is setting:

damageRect.height = shortHeight;

instead of

damageRect.height = longHeight;


"The change to nsContainerFrame.cpp looks like a grotesque hack. What reasonable
argument can there be that "frameSize.width == width" is the correct condition?
IMHO it should always be "TRUE", and if layout needs more repainting, it should
do that explicitly somewhere else."

I agree that the layout use of this flag is a hack. Unfortunetly, Layout is not
consistent in it's invalidation of views. Some frames always invalidate their
bounds when they change size, others rely on the invalidation comming from
container changing its size. This is why Simon's original patch doesn't work.

This was discussed in some length with buster before he left, karnaze, and Marc.

The passing of the flag for only height changes takes advantage of the nature of
flow based layout where height changes don't impact the layout of previously
positioned frames, while width changes do.

Until all of the frames invalidate themselves properly in response to their size
changing. We still need to pass this flag.

This pushes the intelligence for determining what sort of invalidation is
necessary back into layout where it belongs. The view system should be flexible
enough to handle either request; invalidate the entire view when changes size,
or just the newly exposed region patch. When we have fixed all of the frame's
invalidating issues we can choose to always pass PR_TRUE, or it may not need to
invalidate the container at all if all of the child frames have invalidated
themselves properly.

Comment 29

[Kevin McCluskey (gone)]

I filed bug 73825 to cover the layout issue of not invalidating the views
consistently.

Comment 30

[Kevin McCluskey (gone)]

Attachment: patch which includes invalidation of the child view if has a widget

Comment 31

[Marc Attinasi]

sr=attinasi

Comment 32

[Kevin McCluskey (gone)]

Robert: Could you review the lastest patch? Thanks.

Comment 33

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

Sorry Kevin, I'm a total crackhead. I was wrong; there is in fact no problem 
calling UpdateView on the parent view to invalidate whatever areas you want. The 
code that I myself wrote guarantees that all necessary widgets will be 
invalidated. So, you can always invalidate the two strips in the context of the 
parent without needing the extra conditionals. Sorry!!!!

The rest looks great.

Comment 34

[Kevin McCluskey (gone)]

Fix checked in.

Comment 35

[Chris Petersen]

Marking verified per last comments.

Comment 36

[Daniel Bratell]

I still see this in the table stress case. See bug 90503. I don't know if I 
should reopen this and make it a duplicate or let it be a new issue.