69010 - nsRenderingContext::FillRect is waaaaay slow because we paint too much [gfx]

Status: RESOLVED DUPLICATE of bug 34887

(Core :: Layout, defect, P4)

Description

[Mike Pinkerton (not reading bugmail)]

I was doing some painting profiles and i noticed that the normally harmless 
FillRect() calls were taking up over 15% of our redraw time. I put in some 
printf's and discovered that we are frequently painting large 800x600 rects with 
a clip region that's like 20x20.

on mac, this is _very_ slow. Changing the FillRect call to only paint the size of 
the clip area dropped the times for FillRect down to a meager 1.5% of paint time. 
However, not everything draws correctly now. Since we're no longer relying on 
overkill to cover up everything under the sun, we have to make sure we line up 
what we paint with the cliprgn. Problem is, they're in two totally different 
coordinate systems, and I can't find how to translate from one to the other.

Help would be greatly appreciated. It shouldn't take more than 5 minutes for 
someone who knows GFX at all.

Comment 1

[Mike Pinkerton (not reading bugmail)]

after probably 5 hours tracking this down, i think it might be bogus. if it 
paints at all, it's slow. doesn't matter where the rect is, how big it is, how 
big the clip rgn is, etc etc. if PaintRect determines it has to paint anything, 
it's gonna take a lot of time and it's going to be slow.

i'm exhausted and going to bed now. :(

Comment 2

[Kevin McCluskey (gone)]

If all/most of the calls to fillrect are comming from the background drawing
code in CSSRendering we could intersect the fill rect with the damage rect to
reduce the size of fillrect. This would not be as optimal as restricting the
fillrect to the cliprect because clipping may restrict the actual fillrect to a
smaller rectangle than the damagerect, but it would be cross-platform and simple
to do.


Several months ago, [e-mail] and I worked on reducing the amount
painting on the Mac, by making sure that only the top-level child window
recieved the paint message. I don't know if that was ever checked in.

Peter: do you remember?

The approach was: If a paint rect could be totally enclosed within a child
window and the child window had the highest stacking order, then only the child
window would receive the paint request. The current code (unless this fixed was
checked in) would ask all windows which intersected the paint rect to paint.
Child windows are opaque, so there is no need to repaint the window or child
windows if they totally obscure those below them.

This may reduce the number of calls to fillrect in the first place.

Comment 3

[Peter Lubczynski]

I think the code is still there, in nsObjectFrame.cpp, #IFDEFed out. I think it 
had problems due to invalid damage rects being passed in but that may have been 
since fixed. dunno

Comment 4

[Simon Fraser [no longer active]]

Won't this problem be helped by fixing bug 34887?

Comment 5

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

> Won't this problem be helped by fixing bug 34887?

Probably not. Improvements in 34887 will simply let us use smaller clip regions 
when we're painting content that's covered by other opaque content. If I 
understand pinkerton correctly, in the cases he's looking at we're already 
setting a small clip region before we paint.

I'm not really sure what the bug is, in light of pinkerton's second comment. 
What is making Mac fillRects slow ---
a) the fact that a clip region is set
b) the fact that the original rect is large
c) the fact that the original rect overlaps the clipped-out region
?

If a), then we could introduce separate mandatory and discretionary clip regions 
that I talked about some time ago, do GFX-side clipping to discretionary clip 
regions, and avoid setting mandatory clip regions as much as possible. If b) 
(hard to imagine) then we can do GFX-side clipping to the damage rect. If c) 
then either (maybe both) of the previous approaches would help.

I think we've been around the block on this before...

Comment 6

[dcone (gone)]

I think this bug should be looking at the size of the rectangle for the fill.. 
if it is larger than it should be.. we should limit its size.   There may not be 
a huge speed increase... since clipping should stop the rasterization outside 
the cliprect which is were most cycles are burned up.. but the rectangle for the 
fill should be the correct size if possible.  I will look into this ASAP... 

Comment 7

[Mike Pinkerton (not reading bugmail)]

we should be looking at minimizing the number of paints as well.

Comment 8

[Kevin McCluskey (gone)]

Updated status whiteboard.

Comment 9

[Mike Pinkerton (not reading bugmail)]

this bug is (now) about painting too much. all my work profiling the large paint 
rect showed that as long as we paint something (size of rect and size of clip 
aren't really an issue), fillRect is going to be slow. So let's not paint over 
and over again.

Comment 10

[Kevin McCluskey (gone)]

Setting milestone to future, but we will be looking at painting issues as part 
of our ongoing perf work. It there is a specific test case we will address it 
earlier.

Comment 11

[dcone (gone)]

update my keywords, updated milestone

Comment 12

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

There are other bugs about this, notably the one about forcing opaque frames to 
have views so that the view manager can clip stuff out. I think that's the way 
to go.

Comment 13

[saari (gone)]

I assume you mean opaque nsFrameFrames and not just any frame...

Comment 14

[Kevin McCluskey (gone)]

In general isn't it the case that most frames with opaque backgrounds don't
overlap because of the nature of flow based layout. (Usually, you have to
absolutely position the frame to get it to overlap which creates a view for the
frame.) If this is true, maybe we only need to force the frame types (which have
style settings which make them opaque) that tend to overlap to create a view
instead of forcing all of the opaque frames to have views.

In addition, the Mac may be painting too much because the "child window"
implementation on the Mac causes all child windows which intersect the damaged
rect to be painted. On WIN32 if the damaged rect is contained within a child
window only that child window will be asked to paint. It will not ask it's
parent window to paint in that case.  We should add code to the Mac to check to
see if the damage rect is contained within a top most childwindow and only ask
that window to repaint instead of asking all of the child windows which
intersect the damaged rect to paint.

Comment 15

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

> In general isn't it the case that most frames with opaque backgrounds don't
> overlap because of the nature of flow based layout.

No. Suppose the document body has a solid background. Then suppose that document 
contains a table with background colors on the cells. Now suppose one of those 
cells contains a P element with a background set. The opaque elements don't 
fully obscure the elements behind them in general, but they might if your damage 
region is small (e.g., when you're editing such a page).

Mac painting should do what happens on the other platforms: as each child window 
is asked to paint the area it intersects with the damage region, subtract that 
painted area from the damage region before passing to the next child window in 
z-order. In other words, exploit the fact that child windows are assumed to be 
opaque. The view manager will do its magic dance to get things right when the 
views held by the windows are not actually opaque. The Mac painting code should 
*never* issue paint events for overlapping areas of the screen in response to a 
single native update request.

Ultimately we should get rid of child windows as much as possible. It would be a 
huge win if we could get rid of native scrolling widgets.

Comment 16

[Kevin McCluskey (gone)]

"No. Suppose the document body has a solid background. Then suppose..."

What meant to say is the most frames with opaque backgrounds normally overlap
with only a small set of other frames most notably the container frames such as
body and table. I was thinking that we would create views for the container
frames if they are opaque, but we would not create views for every frame that
was opaque. The goal would be to eliminate SOME of the fillrects that are
comming from the totally obscured container frames.

Robert: I know you tried creating views for all frames that were opaque but ran
into problems with some of the assumptions about the view tree. I was wondering
if limiting the creation of the views to just opaque bodies and tables would help?

Comment 17

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

We could be selective and only make views for certain frame types that are 
opaque. I'm not sure I see the value in that, except that it means we can ignore 
some bugs that are revealed by making all frame types eligible for having views. 
IMHO it is much cleaner to have no arbitrary restrictions on which frames can 
have views and which cannot; since we use views in several different 
circumstances, such restrictions will constantly come back to bite us.

There are a number of places in the code that simply assume certain frames do 
not have associated views, and therefore break (typically because they forget to 
sync the view with the frame after reflow). I tracked down a lot of them and 
they were easy to fix once I understood the code and how reflows (or equivalent 
frame state changes) were being performed.

Other than frame/view synchronization, I encountered problems with selection and 
mouse capture. Currently selection and mouse capture depend on which frames have 
views; typically you can't select content that's outside the view containing the 
start of the selection. This seems very wrong because whether the system needs a 
view for a frame or not should have no impact on selection behaviour. I had some 
patches that make selection independent of view structure. They seemed to work 
well.

Comment 18

[Mike Pinkerton (not reading bugmail)]

is the comment about bringing mac in line with other platform's dirty rect 
painting still applicable?

if this is topPerf, should we change the bug to that and then have dcone fix for 
0.9?

Comment 19

[Mike Pinkerton (not reading bugmail)]

ok, well, taking back so it doesn't fall off the radar.

Comment 20

[Mike Pinkerton (not reading bugmail)]

roc? kmcclusk? can you respond to my last question?

Comment 21

[Kevin McCluskey (gone)]

"Mac painting should do what happens on the other platforms: as each child 
window  is asked to paint the area it intersects with the damage region, 
subtract that painted area from the damage region before passing to the next 
child window in z-order"

Looking at the paint logic for Mac it looks like it is capable of rendering from  
front to back but there are some bugs which prevent us from doing this according 
to a comment in the code which sets up the front to back rendering. In addition 
it doesn't look like it has any logic to subtract the currently rendered child 
window from the clipregion when rendering from front to back.

The current code just cycles through the child windows from back to front 
intersecting their rect with the damage rect and painting them.

http://lxr.mozilla.org/seamonkey/source/widget/src/mac/nsWindow.cpp#1628


[e-mail] is currently overloaded with printing issues. Maybe Waqar can 
take a look?

Comment 22

[Simon Fraser [no longer active]]

cc pierre, who may remember something about child window painting.

Comment 23

[Mike Pinkerton (not reading bugmail)]

waqar, do you have time for this?

Comment 24

[Mike Pinkerton (not reading bugmail)]

i cornered waqar and guilted him in front of his peers into taking this ;)

Comment 25

[Kevin McCluskey (gone)]

Setting milestone to mozilla0.9.1.

Comment 26

[waqar]

Working on 0.9 crasher bugs

Comment 27

[Kevin McCluskey (gone)]

Taking this bug

Comment 28

[Kevin McCluskey (gone)]

Moving to Mozilla0.9.6

Comment 29

[R.K.Aa.]

is bug 95952 related to this one?

Comment 30

[Kevin McCluskey (gone)]

Reassigning to dcone.

Don: See comment #21. It explains what needs to be done.

Comment 31

[dcone (gone)]

Attachment: Patch for nsWindow.cpp that paints front to back.. and clips.

This patch paints from front to back and clips out anything that has been
painted, keeps areas from being painted more than once.

Comment 32

[Simon Fraser [no longer active]]

Can you get some performance numbers on this?

Comment 33

[Mike Pinkerton (not reading bugmail)]

i ran pageload tests on osx before/after

w/out patch: avg 1894 ms
with patch:  avg 1779 ms

that's a 6% speedup with the patch.

Comment 34

[dcone (gone)]

Attachment: iframe1

Comment 35

[dcone (gone)]

Attachment: part two of test case

Comment 36

[dcone (gone)]

Attachment: This is the example to run.

If you test with this example.. and count how many redraws, (every draw I
increased i, and then printf at the end of the loop.)  I found the old way was
42 paints.. the new way was 1 paint.  This test case has one huge Iframe
Covering 41 smaller ones.  The test case points out all the un-needed paints,
and how the patch makes all those go away.

Comment 37

[Mike Pinkerton (not reading bugmail)]

they look ok to me, but beard knows that code best and the reasons for sticking
with back-to-front drawing. I'd like to see him bless this code before it goes in.

Beard? can you give it a look-see? thanks!

Comment 38

[Patrick C. Beard]

Please check that calls to ::NewRgn() succeed, and perhaps you can use 
the region pool code instead of raw calls to ::NewRgn(). More comments 
after I really read this code.

Comment 39

[Pierre Saslawsky]

The attached testcase doesn't seem like a good one to me.  IIRC, front-to-back 
painting only brings marginal improvements on fairly simple view hierarchies.  
When we have a few widgets that need to be punched out of the visRgn or clipRgn 
before drawing a background view (let's say 4 or 5 widgets) then back-to-front 
painting is faster, unless these objects cover a large area of the background 
view (let's say 80%).  IIRC again, one of the decisive facts for us going with 
back-to-front painting was that frameworks like PowerPlant proceed that way too 
(see LView::Draw()).

We should make sure that the gains reported by Pink in comment #33 are reflected 
across the board and that complex pages (especially forms or pref panels) are not 
hurt in the process.

Comment 40

[Pierre Saslawsky]

Another thought is that a 6% improvement on pageload tests just by reversing the 
drawing order seems quite big.  How much time do we spend drawing anyhow? 15%? 
more?  6% could represent one third of the total time.  It doesn't sound right.
Maybe the clipping that takes place with front-to-back painting provides a 
workaround for another problem, like bug 7179 / bug 34887.

Comment 41

[saari (gone)]

The only case where front to back is slower is where you have to calculate the
damage region from front to back, and then paint back to front to properly
handle transparent views. And then it is only slower on a slow machine with many
objects in many layers. As noted, we don't do that level of logic when
transparent things are present. The other way it could be slower is if you have
slow non-rectangular region painting which is possible but not likely, splitting
into fairly optimal horizontal biased rects is not rocket science, and the speed
trade off is still much better to do the logic on chip than push bits over the
graphics bus and pipeline.

And the wins get bigger, not smaller, as object overlapping complexity increases.

And 6% speedup is totoally reasonable and doesn't surprise me at all. Drawing is
expensive, especially on Linux, a bit less on Mac, and Win32 moves at light
speed compaired to the others. Why do you think Hyatt's paint supression had
such a noticeable impact? Because we painted less.

Trust me, I spent months working on this problem in a previous life, and here
too. Draw less, be happy.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> Drawing is expensive, especially on Linux, a bit less on Mac, and Win32 moves at
> light speed compaired to the others.

Profile data that I've heard about show that *we* spend a considerably larger
percentage of time drawing on Mac than on Linux.

Comment 43

[dcone (gone)]

"Another thought is that a 6% improvement on pageload tests just by reversing 
the drawing order seems quite big.... "  

This is not just a reversal of order.. the fix includes not drawing an area more 
than once, the order is reversed to make sure all areas are covered by the draw.

Comment 44

[Patrick C. Beard]

The patch looks reasonable to me, except for the caution about the region
allocation code.

Comment 45

[Pierre Saslawsky]

About QuickDraw, things are not as clear-cut as what we believed, saari and I.  
I'll attach a test code that shows that FillRect() is a tiny little bit slower if 
we clip out a few small patches in the painting area (let's say 1% slower + 1% 
per clipped-out patch in my tests).  It also shows that the painting is 
significantly faster if we clip out a large area.  However these gains are 
reduced as the region becomes more complex.  If we clip out 50% of the painting 
area in a single rectangular region, the painting time is reduced by more than 
45% but if we clip out the same 50% in a region made of 100 litle patches, the 
gains are only 25%.  And if we clip out each little patch separately with 
DiffRgn() and SetClip(), it may even go much slower (twice as slow in that case).

DrawString() benefits from clipping too but in a much lesser degree and if we 
count the time that is spent in a single DiffRgn() + SetClip(), it may not 
benefit at all unless the clipping accounts for anywhere between 10% and 80% of 
the painting area (for instance with the sample string I used in my tests, the 
time taken for clipping is comparable to the time taken by one DrawString).  It 
shouldn't matter anyhow because when Layout is done, strings are very rarely 
hidden by something else than the limits of the frame they are in.

Result: the test code shows that clipping out small patches does slow down the 
painting a little bit but not significantly when the total clipped-out area is 
really small.  As the clipped-out area gets bigger (more than 10% for FillRect - 
we don't care about DrawString), painting goes faster.  So, let's clip.

If you want to play with it, note that the testcode should be run in Viewer, not 
Mozilla.

---

About the attached patch, things are not exactly what I thought too.  The 
painting is not done front-to-back: we just paint the children in reverse order.  
If we have a parent widget and N children, we don't paint the children first and 
then the parent.  We continue to paint the parent first and then the children N 
through 1 instead of 1 through N in the current code.  Painting in reverse order 
has a minor inconvenient: it displays complex pages bottom-to-top, which is a bit 
weird (try Viewer Test #9 on a slow machine for instance).

Otherwise, I think I found the main reason for the 6% performance gain but I 
can't quite explain it yet.  If you turn paint flashing on 
("nglayout.debug.paint_flashing"), you can see that the patch fixes the problem 
where the old page gets repainted when going to a new page (bug 97895).  Two 
observations on this:
1) Both FRONT_TO_BACK and USEREGION must be defined in order to fix the above 
problem.  Why? I would have thought that only USEREGION would be necessary.
2) Bug 97895 was supposed to get fixed on the Mac by implementing 
nsWindow::Validate (bug 107828) and dealing correctly with hidden widgets (bug 
107827).  I'm afraid this patch will hide the problem but not really fix it.

After reading the bug report I thought that the patch would improve the 
performance in a fairly clear way by painting front-to-back and not painting the 
same area twice.  But it's not what's happening: we paint bottom-to-top instead 
of front-to-back (with the inconvenience mentioned above) and the performance 
gains seem to come from another problem that was indirectly fixed or hidden in a 
way that needs to be investigated.

Comment 46

[Pierre Saslawsky]

Attachment: test code for Mac Viewer (don't check it in!)

Comment 47

[dcone (gone)]

This patch fixes an obvious problem.. painting children over and over again, 
that are covered by another child is un-nessisary.  The order of painting needs 
to be changed inorder to have complete coverage.  The speed of using regions in 
large chunks or small disjoint chucks just over complicates the problems that 
are addressed by this patch.  

If you use only USEREGION, you can have areas of the window which look like they 
were not painted.  If a child paints.. white for example and then another child 
that is on top gets cliped out.. it looks like an area has not been painted.

I think the results of the performance tests (run by Pinkerton), and the test 
case I wrote (shows the coverage of the widgets and show a decrease in the 
number of paints) is reason enough that this patch works as advertised.  We 
might want to discuss painting the windows frames after all the children have 
been painted with the resulting region.. that may also increase the speed.

Comment 48

[Simon Fraser [no longer active]]

> Otherwise, I think I found the main reason for the 6% performance gain but I 
> can't quite explain it yet.  If you turn paint flashing on 
> ("nglayout.debug.paint_flashing"), you can see that the patch fixes the problem 
> where the old page gets repainted when going to a new page (bug 97895).  Two 
> observations on this:
> 1) Both FRONT_TO_BACK and USEREGION must be defined in order to fix the above 
> problem.  Why? I would have thought that only USEREGION would be necessary.

I don't immediately understand why this would be the case.

> 2) Bug 97895 was supposed to get fixed on the Mac by implementing 
> nsWindow::Validate (bug 107828)

It turns out that this is not sufficient. The paint suppression code in PresShell 
acutally does not call Validate() any more. Since we don't have a fix for bug 
107827, the problem still exists, but I think it's timing dependent.

I agree with Pierre that we need to better understand the implications of this 
patch, rather than relying on a single 6% perf number.

Comment 49

[saari (gone)]

A point of order, just so we're clear. It doesn't matter if you paint back to
front or front to back UNLESS you've got transparency to deal with. It was
stated before that we run a different code path in that case so ignore it. All
that matters is that you're only painting a region of the screen once. If you
calculate a clip region front to back, and then paint back to front using that
info. you can achieve the same effect.

That said, I haven't looked at this patch, but I don't want people to get hung
up on the back to front, front to back thing. Just paint less by using clipping
info.

Pierre's test shows exactly what I would expect. The missing part of his test is
that he doesn't test speed when you paint things more than once with painters
alogrithm vs. painting a screen region once with clipping info. _That_ is what
we're really interested in measuring, and what we're really talking about.

Comment 50

[dcone (gone)]

I really really dont understand what the friction is.  We paint less.. we dont 
paint areas more than once.  Speed increases.. this seems to be very .. 
intuitive to me.  I guess the only thing I can to.. is count how many paints 
with the preformance tests.. before and after.. will that be enough?  Speed was 
increased.. I dont see any bugs because of this patch..or hear any arguments why 
it hurt the product.  All I see is positive. 

Comment 51

[saari (gone)]

I think the only concern is that it might be hiding another bug, correct?

Painting less is a Good Thing (TM) and I'm all for it.

Comment 52

[Simon Fraser [no longer active]]

On further consideration, I don't think this patch will help performance in the 
most common cases much. I'm not saying that the patch is not a good thing, only 
that oftentimes we just don't run into the situation where we have overlapping 
widgets. I also verified that it does not help the issue discussed in bug 97895.

To clarify, this patch only addresses the relative paint ordering of child 
widgets, and not frames within those widgets. In addition, child ordering only 
matters when the widgets overlap. Even in the old code, the clipping that we do 
ensure that we don't over-draw when painting the child widgets.

By debugging in nsRenderingContextMac::FillRect (which is what this bug was 
originally about), I verified that the patch has zero impact on the number of 
times this is called, and the regions that it fills, for the Mozilla chrome 
showing a normal (frameless) web page.

I think dcone's test case is a pathological case. I need to debug and find out 
why we behave so badly in it with the old code. I'm also running performance 
tests with and without his patch.

Comment 53

[Simon Fraser [no longer active]]

BTW, the patch as-is will leak 2 regions per UpdateWidget(). That's not good.

Comment 54

[dcone (gone)]

yes it will leak.. I saw that.. I was going to change the code to keep that 
region around all the time.. and set it empty when that function was complete.. 
so that region would be used many times and not re-allocated over and over. 
(Patricks comments to re-use the regions) I also need to check to see that the 
NewRgn() succeeded.. but I was going to do that in the constructor.. or init 
which ever is appropriate.  My test case.. was there only to prove that the 
algorithm was correct in its assumptions. The test pinkerton ran (I think they 
are John Morrisons performance tests) where there to measure what kind of 
performance we realized.


Comment #9 says this bug is about painting to much.. so thats what I addressed.  
I think the number of times FillRect is called (since it is slow) will be 
decreased by this code.  If a child is partially covered..or totally covered.. 
less FillRects will be called.  

The bottom line for me.. fundamentally.. this is how the paint should happen 
when you paint graphics.. and they can be on top of one another.. and clippin is 
efficient enough and fast enough to handle this.  Those things considered.. I 
think this fix passes that criteria.  If after this fix goes in.. and we find 
other areas that cause extra paints.. those need to be addressed, but seperately 
than this.  I don't see any fundamental arguments that argue against this method 
for updating the children of a window.  

Comment 55

[Patrick C. Beard]

Use the region pool, Luke:

http://lxr.mozilla.org/mozilla/source/gfx/src/mac/nsRegionPool.h#76

Comment 56

[Simon Fraser [no longer active]]

Attachment: Cleaned up patch

This is dcone's patch, but with some cleanup:
* used StRegionFromPool
* spacing made consistent with the rest of the file
* declared variables just before use (this is C++, hey)

I also made nsRectToMacRect static and inline.

I'm happy with this patch, and I have verified that it does indeed speed page
load by 6-9%. Profiling shows that the increase in performance was by reducing
the time spent in handling the paint event for widget update, rather than
changing the number of updates, so the patch seems to behave as intended.

I don't think we can close this bug based on this patch; we still do lots of
redundant FillRect() (mostly in regions which get over-painted by other
frames).
I'm happy to put my sr on the patch.

Comment 57

[Pierre Saslawsky]

The latest patch failed for me with these errors:
Hunk #6 FAILED at 2402 when patching nsWindow.cpp
Hunk #1 FAILED at 181 when patching nsWindow.h

Comment 58

[Pierre Saslawsky]

With "Ignore space changes", the patch worked...

Comment 59

[Pierre Saslawsky]

Please forgive me if I'm wrong but apparently....
1) The only reason why the new code is faster is because we don't redraw the old 
page before displaying a new one.
2) The new code doesn't redraw the old page because of a bug where we subtract 
the rectangle of a widget from the update region even when the widget is 
invisible.

Without that bug, the new code would not bring any performance gain except in 
particular cases.  This being said, we now know (thanks dcone) that we have a 
potential 6-9% performance improvement to grab on the Mac.

Could someone on the internal network please run the pageload tests with the 
above fix to confirm that what I'm saying is true?  I'm going to attach some test 
code; the fix is in there, ifdef'd DONT_COUNT_INVISIBLE_WIDGETS.

The test code traces all the updates that occur in Mozilla during a simple page 
load.  I'll also attach a log with some comments to help understand what's 
happening.  You can see for instance  how in the new code an invisible widget 
that is inserted at the end of the list prevents its siblings and their children 
from being displayed.  It also shows how with the fix the new code and the old 
code don't have any difference in terms of how UpdateWidget() is called except 
for the inversion in the order of the siblings: the number of calls and, more 
importantly, the update rects are the same.

Otherwise, some of the data in the log is rather baffling when it comes to the 
widget hierarchy and the coordinate system of child widgets.  I need to make some 
sense of it.  Those who have worked on Widget (beard/dcone/pink/sfraser) may want 
to take a look, or maybe even play with the test code.  Apparently there could be 
some nice gains beyond the 6% already reported.

Comment 60

[Pierre Saslawsky]

Attachment: test code to trace calls to UpdateWidget() + fix for invisible widgets

Comment 61

[Pierre Saslawsky]

Attachment: log of UpdateWidget() + comments

Comment 62

[Simon Fraser [no longer active]]

Pierre is correct, I think. Nice sleuthing! Not subtracting out the area for 
invisilble widgets negates all of the performance gains of this patch; the page-
load tests are something like:

Original:       1506 ms
dcone patch:    1360
pierre patch:   1508

Now to answer some of pierre's questions:

- Why are we asked to redraw the old page?  Apparently the new page is inserted
  in the widget hierarchy as invisible and we should not have an update request
  for the content area right before showing the new page.
  
This is covered by bug 107827, and was originally filed as bug 97895. The gist
is that we do paint-suppressed loads of the new page while the old page is still
visible. We build views/widgets for the new page, and invalidate them, while
paint suppression is active on ther pres shell. On Windows, those invalidates
do not resulting in painting, because the OS sends updates on a per-widget basis,
and we can filter out updates on those widgets we know we shouldn't paint yet:

<http://lxr.mozilla.org/seamonkey/search?string=IsPaintingSuppressed>

(not sure how this works). On Mac, we're screwed, because we don't have native
widgets; the "invisible" widgets take up an area on the screen, and invalidates
on that area will cause the OS to send an update event to the window. The crux
is that on Mac, all widget updates are "flattened" into a top-level-window
update, so we're forced to redraw every widget that covers that area. This
is why we repaint when leaving the page:

* While loading the new, paint-suppressed page, we make widgets (nsWindows)
  for its scrolling views, and in making these, do a number of large invalidates.
* A bit later, we change the visible URL in the url bar, and do a synchronous
  update (editor turns view batching off)
* In handling the synchronous update we do ::BeginUpdate/::EndUpdate in Mac
  code, which forces us to redraw the whole area that was invalidated, including
  that for paint-suppressed widgets.


- Why are some child updates done in relative coordinates?

nsWindow::mBounds is in coordinates relative to its parent. It's just that
many of our widgets are in the top-left corner of their parent.

- What the heck does the widget hierarchy look like? Why, when we paint the Stop 
button or
  the Throbber or pretty much everything else, do we end up evaluating whether 
the URL bar
  icon or the web page needs a refresh?
  
I'm not totally sure what you mean here, but I think it's answered by my
discussion above. If you've previously invalidated any widget, and then
someone does a synchronous update, then we redraw everything in the window
that's dirty.

Comment 63

[Pierre Saslawsky]

About bug 107827: I updated that bug after a (failed) attempt to accumulate 
invalidates on invisible widgets into a member variable, and really invalidating 
when the widget is made visible.

About coordinates and widget hierarchy: I found it weird to see how when we 
update the children of the Throbber of the Stop button, we end up updating the 
URL bar icon or the web page in the second level of recursivity (as grand-
children).  When this happens, UpdateWidget() is indeed called by direct 
reentrance, not indirectly through DispatchWindowEvent(paintEvent). I'll do more 
debugging after the holydays.

Comment 64

[Pierre Saslawsky]

A note about my previous comments...  When we update the children of the Throbber 
or the Stop button, we dont really update the URL bar icon or the web page.  We 
just check if they should be updated and we don't because they are not in the 
updateRgn. It still shows that something may be wrong in the widget hierarchy.

Comment 65

[dcone (gone)]

I think that this fix still needs to be checked in.. its the right way to handle 
updates of this nature (children of parents that don't have there own updates 
events).  The other problems that are mentioned should be tracked in there own 
bugs... and solved.  It has been my experience that performace gains are never 
from one big fix.. but many small steps.. some of these steps may even be a step  
backward.. but allows other steps to take bigger gains.  I feel that the patch 
here is a small step in the correct direction.  The other problems can then be 
tracked with a better understanding of how the updates.. and painting should 
happen.

Comment 66

[Simon Fraser [no longer active]]

So, is is the case that later children will always overlap earlier children in 
the child list? Are children always guaranteed to be in back-to-front order? What 
could the gains be if we painted the widgets from front to back (i.e. children 
before parents)?

Comment 67

[dcone (gone)]

Children can cover children before it in the list, and this will not stop that 
covered child from being painted currently.  At the moment.. children are are in 
back to front order and I dont see that being changed.

Widgets are different because they get there own events, so if one widget 
completely covering another.. the covered widget will (should) not get an 
update/paint event.  This is not so with the children of a widget since they are 
handled by the widget and not by the update/invalidate etc mechanism for 
painting.  Back to front painting is a good choice for this, nice and clean.

Comment 68

[Simon Fraser [no longer active]]

> Widgets are different because they get there own events, so if one widget 
> completely covering another.. the covered widget will (should) not get an 
> update/paint event.

We know this isn't quite true on Mac, since the Mac widget code is unable to 
determine which widget OS-originating invalidates are directed at.

> This is not so with the children of a widget since they
> are handled by the widget and not by the update/invalidate etc mechanism for 
> painting.  Back to front painting is a good choice for this, nice and clean

Sorry, I don't understand this. By "the children of a widget" here, do you mean 
frames (as in layout frames)? My comment was referring to child widgets of a 
parent widget.

Comment 69

[dcone (gone)]

Any widget that has a native window can get is own update/paint events 
independent of other widgets with native windows. These types of widgets are 
independent and dont need front to back with clipping.  If a child can not get 
its own paint or update events.. then the front to back is needed unless its 
guarenteed that it does not cover anyhing that is part of the child list its 
part of.  The front to back with clipping is a way to guarentee that the 
smallest areas get painted.. and areas are not painted twice for objects whos 
drawing is not managed.  If there are any other cases like this.. they to should 
be draw this way.. I am not aware of any, but there certainly may be.

Comment 70

[Pierre Saslawsky]

dcone (about comment #65): what version of the patch would you like to check in: 
with or without the fix for invisible widgets?  With the fix there isn't any 
performance gain (1508 ms instead of 1506).  Without the fix, we get the 6%-9% 
gain but we would expose ourselves to potentially serious update problems (maybe 
not in the current Navigator code-base but possibly in other modules or in 3rd-
party applications).

FYI, I dumped the widget hierarchy for the Navigator window and the Pref dialog.
The format is:
  level - childNumber  (top, left)  <width> x <height>

As you can see, we don't have a lot of widgets in the chrome but there are some 
interesting things:
1) siblings don't overlap each other.
2) we have 4 widgets on top of each other just for the web page, 3 of them have 
the same size.
3) the URL bar icons creates 2 widgets, one of which is always empty.


Navigator window: (displaying mozilla.org)
-----------------
        0-1 (0, 0)  600x400                         entire window
            1-1 (0, 0)  600x400                     entire window
                2-1 (68, 10)  590x316               web page with scrollbars
                    3-1 (0, 0)  590x316             web page with scrollbars
                        4-1 (0, 0)  590x316         web page with scrollbars
                            5-1 (0, 0)  575x301     web page w/o  scrollbars
                2-2 (15, 273)  0x0                  URL bar icon (empty rect)
                2-3 (15, 273)  16x16                URL bar icon

Pref dialog:
------------
        0-1 (0, 0)  521x410                         entire dialog
            1-1 (0, 0)  521x410                     entire dialog
                2-1 (46, 153)  363x312              pref panel
                    3-1 (0, 0)  363x312             pref panel
                2-2 (27, 11)  136x327               tree widget

I'll attach a patch if someone is interested in investigating how widgets are 
used in form controls.  I can also check in the debugging code that generated 
these logs and the previous ones.  After all, it's not the first time and maybe 
not the last one that we investigate update events.

Comment 71

[Pierre Saslawsky]

I vote for checking in dcone's patch because it is conceptually more correct to 
do a front-to-back painting of siblings even though it doesn't appear to bring 
much improvement in the general case.

The follow-up on over-painting during page load is under bug 107827.

Comment 72

[Simon Fraser [no longer active]]

Do we need dcone's patch to be fixed to account for invisible widgets?

Comment 73

[Pierre Saslawsky]

Yes, the patch needs to be fixed: it is incorrect to validate an area covered by 
invisible widget.  To merge the fix, look for DONT_COUNT_INVISIBLE_WIDGETS in 
attachment #62519 [details] [diff] [review].

Comment 74

[Simon Fraser [no longer active]]

With the patch from attachment 65398 [details] [diff] [review] (bug 107827), the patches in this bug don't 
have any additional impact on page load performance. My guess is that most web 
pages don't have child widgets which overlap. It seems that will only happen in 
the rare case of absolutely positioned iframes, as in the test case here.

Comment 75

[dcone (gone)]

based on that last comment.. I am marking this.. wont fix.  If there is
something that needs to be done.. that was put in this bug.. please open a new
bug, be specific about what you think needs to be fixed and refer to this bug.

Comment 76

[Simon Fraser [no longer active]]

That last comment does not mean that this bug is in any way invalid. It just
says that your patch doesn't have any impact on performance, which means that
it's the wrong patch to fix this bug.

We still paint way too much. Just turn off double buffering, and put a
breakpoint at FillRect; see it paint huge areas of background, which in the same
painting pass get drawn over.

Comment 77

[Kevin McCluskey (gone)]

"We still paint way too much. Just turn off double buffering..."

This issue is covered by bug 34887

Comment 78

[Simon Fraser [no longer active]]

So let's mark this bug appropriately.

*** This bug has been marked as a duplicate of 34887 ***