Closed Bug 107827 Opened 23 years ago Closed 23 years ago

Mac is over-painting during page load

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Platform:
PowerPC
Mac System 9.x
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.8

People

(Reporter: bryner, Assigned: sfraser_bugs)

Details

(Keywords: perf)

Attachments

(2 files)

patch to accumulate invalidates on invisible widgets
10.42 KB, patch
Details | Diff | Splinter Review
Patch to avoid invalidate widgets if they or any of their parents are invisible
6.67 KB, patch
Details | Diff | Splinter Review 
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.
Status: NEW → ASSIGNED
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?
Keywords: perf
Target Milestone: --- → mozilla0.9.8
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).
Keywords: mozilla0.9.8+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: