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.

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. :(

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, peterl@netscape.com 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.

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

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

> 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...

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...

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

Updated status whiteboard.

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.

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.

update my keywords, updated milestone

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.

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

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.

> 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.

"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?

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.

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?

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

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

"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 dcone@netscape.com is currently overloaded with printing issues. Maybe Waqar can take a look?

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

waqar, do you have time for this?

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

Setting milestone to mozilla0.9.1.

Working on 0.9 crasher bugs

Taking this bug

Moving to Mozilla0.9.6

is bug 95952 related to this one?

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

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

Can you get some performance numbers on this?

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.

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.

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!

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.

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.

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.

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.

> 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.

"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.

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

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.

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.

> 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.

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.

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.

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.

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.

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

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.

Use the region pool, Luke: http://lxr.mozilla.org/mozilla/source/gfx/src/mac/nsRegionPool.h#76

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.

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

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

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.

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.

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.

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.

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.

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)?

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.

> 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.

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.

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.

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.

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

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].

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.

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.

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.

"We still paint way too much. Just turn off double buffering..." This issue is covered by bug 34887

So let's mark this bug appropriately. *** This bug has been marked as a duplicate of 34887 ***