Spun off from bug 97895. Invalidates happening on hidden nsWindows during paint suppression are causing painting to happen on visible widgets. This is because of the way Invalidates are translated to coordinates relative to the toplevel window, losing the key information of exactly which widget was invalidated. Simon, feel free to reassign as needed (but not to me, I don't have a Mac).

So I'm trying to figure out how we can make this work on Mac, and not having much success. Each widget could maintain a dirty region, and we could intersect this with incoming updates to decide what to paint. But that doesn't do much for widgets with large overlapping areas, which is most of the problem. I really think a cleaner solution to this is to keep widgets/views for hidden content viewers really hidden, as I described in bug 97895.

do dcone's front->back painting changes help this at all (bug 69010)?

I don't think so. I did think of one thing I could do. I could give each widget a 'pending update' flag, and then only draw a widget in response to an update event if the flag is set. That way, we may invalidate large areas of the screen, but we could avoid repainting it all unnecessarily.

Well, the 'pending update' flag doesn't work, because I can't tell if the incoming update event was OS-generated (e.g. window exposed) or is the result of an earlier Invalidate() call. So I can't ignore updates based on whether the window has had any known invalidates.

If you need to know whether it is an OS event, you can pass a boolean to HandleUpdateEvent(), false by default and true when called from nsMacEventHandler::UpdateEvent().

That's not it; we'll get update events from the OS as a result of two kinds of invalidates: 1. Internal calls to InvalRect/Rgn 2. OS-level events like window expose (Windowshade, user unobscuring part of the window). and, when we receive the update event, we can't distinguish its cause(s). This means that we can't know to ignore udpate events that happen as a result of invalidates on certain widgets.

I tried a solution where when we call Invalidate(rect) of Invalidate(rgn) for an invisible widget, we accumulate the rect or the rgn into a member variable. When the widget is made visible, the whole area is really invalidated. It doesn't work. When loading a simple page, we have only 3 such calls. They happen right before "Document loaded successfully" but by that time we have already repainted the old page. For those familiar with the traces from bug 69010, here is what it looks like (formatted to 80 chars/line): ---- UpdateWidget() parent rect top=68 left=10 width=737 height=372 New code updating visible level 1 rect top=68 left=10 width=737 height=372 New code NOT updating visible level 2 rect top=15 left=273 width=16 height=16 New code updating visible level 2 rect top=0 left=0 width=737 height=372 New code updating visible level 3 rect top=0 left=0 width=737 height=372 New code updating INVISIBLE level 4 rect top=0 left=0 width=737 height=372 New code updating visible level 4 rect top=0 left=0 width=737 height=372 New code updating visible level 5 rect top=0 left=0 width=722 height=372 New code NOT updating INVISIBLE level 2 rect top=15 left=273 width=0 height=0 Invalidating invisible rect top=68 left=10 width=738 height=373 Invalidating invisible rect top=68 left=732 width=16 height=343 Invalidating invisible rect top=68 left=732 width=16 height=373 Document resource:///res/samples/test0.html loaded successfully ---- These 3 calls corresponds to (1) the entire content area, (2) the scrollbar without the arrows and (3) the scrollbar with the arrows. The old page is repainted when we update the "level 5" widget, before these 3 calls are made. I'll look into it again after the holydays.

If someone wants to play with it between Xmas and new year's eve... Note: this patch contains the test code from bug 69010

This patch adds code to not do invalidates on widgets which are themselves invisible, or which have a parent that is not visible. This avoids invalidating widgets in a hierarchy for which paint suppression is active. My tests show that this patch reduces page load time from 1470ms to 1280ms (about 7%)

Ooops, patch contains some paint debugging changes too. Ignore those, and the whacky spacing.

a cleaner way to write this would be: PRBool nsWindow::ContainerHierarchyIsVisible() { nsCOMPtr<nsIWidget> theParent = dont_AddRef(GetParent()); while ( theParent ) { PRBool visible; theParent->IsVisible(visible); if (!visible) return PR_FALSE; nsIWidget* grandparent = theParent->GetParent(); theParent = dont_AddRef(grandparent); } return PR_TRUE; } right?

IMO, the patch works because the View manager keeps a separate list of the invalidates that must be done on invisible widgets (if I understood correctly what Kevin explained me last week). However to make the Mac toolkit really behave like other platforms, we probably should mix the 2 patches above and accumulate invalidates on invisible widgets like in attachment #62601 [details] [diff] [review] but using the definition of invisible widget from attachment #65398 [details] [diff] [review].

Update on perf numbers: Current trunk build page load: ~1500 Trunk with patch in attachment 65398 [details] [diff] [review]: ~1260 That's about 6%.

You're too modest: 1260/1500 = 0.84, that's a 16% (sixteen!) percent improvement. What a beautiful day. I'll attach a patch that does what I described in my previous comments and see if it doesn't spoil the fun (and if it does, I guess you can check in your patch as- is because the View manager does the right thing for us anyhow).

Time with the second patch, plus the dcone patched fixed to account for invisible widgets (cleaned version of attachment 62519 [details] [diff] [review]) is in the 1260 ballpark, so that patch doesn't really help performance for the pages we're testing.

If I can get an sr, I've love to check this in for 0.9.8. Hyatt?

Probably obvious, but I assume you don't want to leave these uncommented: -//#define PAINT_DEBUGGING // flash areas as they are painted -//#define INVALIDATE_DEBUGGING // flash areas as they are invalidated +#define PAINT_DEBUGGING // flash areas as they are painted +#define INVALIDATE_DEBUGGING // flash areas as they are invalidated Fix the indentation in ContainerHierarchyIsVisible() sr=hyatt

This patch takes pageload time from 960 to 768ms on an optimized Mach-O build (20%)

with hyatt's suggestion a=asa (on behalf of drivers).

Checked in.

Simon, your change affects only two functions: nsWindow::Invalidate(const nsRect &aRect, PRBool aIsSynchronous) nsWindow::InvalidateRegion(const nsIRegion *aRegion, PRBool aIsSynchronous) I think you should look into these ones too to see if they are sometimes called when the container hierarchy is not visible: nsWindow::Validate() nsWindow::Update() nsWindow::HandleUpdateEvent(RgnHandle regionToValidate) nsWindow::UpdateWidget(nsRect& aRect, nsIRenderingContext* aContext) nsWindow::Scroll(PRInt32 aDx, PRInt32 aDy, nsRect *aClipRect) nsWindow::FindWidgetHit(Point aThePoint) nsWindow::Show(PRBool bState) Note: nsWindow::Show() should still update |mVisible| if the container hierarchy is not visible Otherwise I talked to Kevin again and there is no need to merge my patch that accumulates invalidates for invisible widgets, even for the sake of correctness.

In my build I added an assertion in UpdateWidget() if it was ever called on a widget with an invisible ancestor, and that was never the case. I didn't test the other methods you list; we could throw some assertions in for testing, I suppose.

I suggested to check the visibility of the hierarchy not as much for a question of performance (I guess there isn't anything else to grab beyond 16-20% :-) as for a question of correctness. We are a bit too exposed to changes in the Compositor. Validate() especially could be a problem, and maybe FindWidgetHit() too if a UI widget someday does weird things with widget visibility. In all the above functions, we should really be using ContainerHierarchyIsVisible() instead of mVisible. Some assertions would be nice to have indeed. It is not worth replacing mVisible everywhere if we know that the function always returns true in the current code base.

Note that ContainerHierarchyIsVisible() doesn't check "this->mVisible"; it just starts looking at the parent. Maybe it should be changed to check mVisible too.

Agreed. A small patch for this cleanup + add some assertions would be nice. No need to open a new bug report.

Important note for the Mac folks.... Kevin warned me that future performance improvements with regard to the painting on the Mac may no longer be noticeable if we beat the paint suppression timer on most pages. It would be interesting to do the same test as jrgm did in bug 34887 comment #75 and see how many pages now beat the timer on an average machine. It used to be 33 out of 40.

i would hardly call any machine hooked up to the netscape network an average machine. Our pipeline is one of the largest and fastest in the world. think about modem users who will most definately _never_ beat the paint suppression timer.

End-users don't run the page load tests. I should have written "an average Netscape developer machine". In fact, it would be even more interesting to check it on a super-duper Netscape developer machine because if the super-duper machine shows that we always beat the timer, we'll know that we can no longer rely on the page load tests to measure performance in painting.

I checked this with current trunk builds on win2k and Mac, running with 0ms page to page delay time: "Cached" case: Mac OS9.1, 450MHz, 256MB, page times: avg 2.2s, max 6.0s, painted: 15 of 40 win2k, 500MHz, 128MB, page times: avg 1.2s, max 3.5s, painted: 8 of 40 "Fully Uncached" case: Mac OS9.1, 450MHz, 256MB, page times: avg 2.2s, max 6.0s, painted: 17 of 40 win2k, 500MHz, 128MB, page times: avg 1.2s, max 3.5s, painted: 11 of 40 Note: the win2k build formerly did not paint any pages. That was wrong at the time since some of the pages clearly take longer than the paint suppression threshold time to elapse. I don't know if this was specifically fixed in the interim, or if it is just better behaved now for some unknown reason.