97895 - Old page gets repainted when leaving a page (Back and Forward performance)

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Simon Fraser [no longer active]]

One reason for our poor back/forward performance is probably that we redraw the 
entire old page when leaving it, when the user clicks on a link, or goes Forward 
or Back, or even Reloads. This is easy to show by editing a local page, and doing 
a Reload; the page is redrawn once with the old source, then again with the new 
source. It's obvious to see if double buffering is turned off.

Comment 1

[Simon Fraser [no longer active]]

Attachment: Stack for the pre-page leave Invalidate() call that causes a redraw

Comment 2

[Simon Fraser [no longer active]]

The attachment shows the stack for a full-screen Invalidate that gets done before 
we leave the old page; the old page gets redrawn as a result of the invalidate 
here.

Comment 3

[Simon Fraser [no longer active]]

The reason for this Invalidate() is that we create a new nsWindow for the 
nsScrollPortView of the new page contents, then resize it, but this all happens 
while we're still showing the contents of the old page. Paint suppression ensures 
that the new pages gets redrawn later on.

I also note here that PresShell::InitialReflow() gets the prefs service, and the 
value of the "nglayout.initialpaint.delay" pref every time. This should probably 
be cached.

Comment 4

[karnaze (gone)]

Reassigning to dbaron, or if he objects, maybe waterson?

Comment 5

[Simon Fraser [no longer active]]

What's bad here is that nsScrollBoxFrame (and nsScrollFrame) always creates its 
scrolling view as visible. This seems wrong. We also need to fix the Mac code to 
ignore invalidates on invisible windows.

Comment 6

[Simon Fraser [no longer active]]

It turns out that we can always make the view hidden in 
nsScrollBoxFrame::CreateScrollingView(), because 
nsContainerFrame::SyncFrameViewAfterReflow() later makes it visible, but only 
after the resize that is causing the bad invalidate. I wonder if nsScrollFrame 
should be treated the same way?

Comment 7

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

I see the same problem on GTK and Windows.  I wonder at what level we should be
blocking this -- whether Invalidate should do a visibility check, or whether
Resize should do it before calling Invalidate, or what?  I guess it doesn't make
much sense for Invalidate to be called on a window that isn't visible.

Comment 8

[Benjamin Mucci]

Check this web site...

http://www.fileplanet.com/

redraws after redraws here onload and unload.

Comment 9

[Brian Ryner (not reading)]

I can take this one.

Comment 10

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

Just for reference, one thing bryner mentioned when we discussed this earlier
was that it would probably be easier to fix this with changes to the view code
than the widget code since they don't have to be done for each port.

Comment 11

[Simon Fraser [no longer active]]

I think the fix for this will require that one can hide or show entire view 
hierarchies, while maintaining the visibility of individual views. Views need 
some notion of 'latent visibility' or soemthing.

Note that the Mac widget code still needs fixing to not invalidate when a widget 
is not visible.

Comment 12

[David Hyatt]

Try implementing Validate().  I call Validate() when using paint suppression,
but Validate() is only implemented on Win32.

Comment 13

[Simon Fraser [no longer active]]

It seems that Validate() is called _before_ the invalidates that cause the 
problem. The root of the problem still seems to be that even when painting is 
suppressed (which I assume to mean that the content viewer is not visible), view 
creation can cause invalidates on screen.

Comment 14

[Brian Ryner (not reading)]

roc, do you have any thoughts on this?

Comment 15

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

We could add another layer of paint suppression in the view code, but I think
this is unnecessarily complex. Instead of adding new APIs and fields and whatnot
to the view code, isn't it simpler and more logical to just whack each widget
implementation to ignore invalidates on hidden widgets? This would require no
new interfaces and I really, really detest unnecessarily fat interfaces.

Having said that, I don't understand why this is a problem on Windows or any
other platform where we use native objects for widgets. Windows should already
suppress invalidates on hidden HWNDs, I would have thought.

Comment 16

[Simon Fraser [no longer active]]

We're creating visible views in nsScrollBoxFrame (and nsScrollFrame) even when 
painting is suppressed (the pres shell is not shown). I assume that these views 
will create visible widgets, but is it the case that visibility of a child widget 
depends on its parent widget being visible? Or is the visibility of child widgets 
indpendent of that of the parent?

Comment 17

[Brian Ryner (not reading)]

Yes, creating a visible view makes it so the widget is shown when CreateWidget
is called.  I've found that this is also happening in nsDocumentViewer::Init --
it creates a view as shown in MakeWindow, then calls CreateWidget, then hides
the widget.  On linux, this causes full-window invalidates on both the newly
created widget and the widget for the current page.  Making it create the view
initially as hidden gives me a 2.3% speedup.

So, in the case of the widget created by Scroll[Box]Frame, it is marked as shown
but its parent is hidden.  I think it's reasonable to assume that the child
widget is implicitly hidden.  We could certainly make Invalidate() ignore the
call if this widget OR any of its ancestors are hidden, but do we really want to
walk up the parent widget chain every time Invalidate is called?

Comment 18

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

I think it's unreasonable that calling Invalidate() on a widget that's hidden or
has a hidden ancestor can cause some other widget(s) to be repainted. After all,
we didn't ask for that widget to be repainted.

Note that if other widgets are being repainted, then some piece of code must be
walking up to a common ancestor and then back down again, just to translate the
damage rectangle into the right coordinate system for the widgets that get
painted. So it should be little additional burden to check for visibility on the
way up.

Comment 19

[Brian Ryner (not reading)]

Attachment: Show and hide the view instead of the widget for paint suppression

Comment 20

[Brian Ryner (not reading)]

This patch sets the root view's visibility instead of the widget's visibility
for paint suppression.  That lets us avoid two extra invalidates that were
happening because of the view's widget being shown and then immediately hidden
(one invalidate on the new view, one on the old view).  I'm seeing about a 3%
improvement in page load times with this on Linux.

Comment 21

[Brian Ryner (not reading)]

Note that this patch doesn't yet address the original invalidate Simon found,
these are separate.

Comment 22

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

Does setting the visibility of the view affect the visibility of its widget? 
And does the visibility of a view affect the visibility of its children?  Does
it affect how they handle UpdateView?

Comment 23

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

> Does setting the visibility of the view affect the visibility of its widget?

No.

> And does the visibility of a view affect the visibility of its children?

No. Visible children of hidden views will be painted.

Comment 24

[Brian Ryner (not reading)]

>> Does setting the visibility of the view affect the visibility of its widget?

>No.

roc, are you sure about that?  See the following code from nsView::SetVisibility:

  if (nsnull != mWindow)
  {
#ifndef HIDE_ALL_WIDGETS
    if (mVis == nsViewVisibility_kShow)
      mWindow->Show(PR_TRUE);
    else
#endif
      mWindow->Show(PR_FALSE);
  }

Comment 25

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

Hmm, I stand corrected.

Now I wonder how our support for visible frames inside hidden frames is working.
Maybe it isn't.

Comment 26

[Brian Ryner (not reading)]

roc, can you think of any problems that would arise from the patch I attached to
hide the root view instead of its widget during paint suppression?  If this will
cause unintended side effects, we can change the CreateWidget method to let the
caller specify whether the widget's visibility should be made the same as the
view's, but I prefer this solution because it's cleaner (and it also allows
UpdateView to do less work).

Comment 27

[Brian Ryner (not reading)]

I'm not seeing any performance boost at all on win32 from this patch... probably
because it implements the call to Validate().  We shouldn't really _need_ a
Validate method... it looks like this was just working around the invalidates
that were caused by showing and hiding the root view's widget.

hyatt, if these invalidates can be avoided (as in my patch), can we remove
Validate() entirely?

Comment 28

[Brian Ryner (not reading)]

Comment on attachment 55495 [details] [diff] [review]
Show and hide the view instead of the widget for paint suppression

actually, this doesn't work quite right... it seems to break paint suppression entirely

Comment 29

[Brian Ryner (not reading)]

The reason my previous patch breaks paint suppression is that
nsContainerFrame::SyncFrameViewAfterReflow (called from
PresShell::InitialReflow) determines that the view should be visible from the
frame's style data, and sets it to be visible... in the current case, that ends
up being a no-op because the view is already shown, even though its widget is
hidden, so nsViewManager::SetViewVisibility returns early and
nsView::SetVisibility is never called to sync the widget's visibility with the
view's visibility.  With my patch, the new visibility is not the same as the old
visibility, so SetVisibility is called and the view is shown.

So, using the view's visibility isn't going to work because reflows will happen
before paint suppression is done.  I'd suggest that instead, we let the caller
to nsView::CreateWidget specify whether the widget's visibility should be set to
the view's visibility.  Since widgets start off hidden, that will prevent the
invalidates.

That still leaves Simon's invalidate stack and the issue of invalidates on
widgets that are visible but have hidden ancestors.  I'd argue that this
checking should be done at the widget level, even though it means more work,
because someone could invalidate a region of the widget without going through
the view system at all.  Does that make sense, or should we not be worrying
about that case?

Comment 30

[Brian Ryner (not reading)]

Attachment: patch to make nsView::CreateWidget not set the widget visibility

Comment 31

[Brian Ryner (not reading)]

So, I'm actually reasonably sure that this patch fixes any painting that we
might have been doing.  I don't think the stack Simon attached is actually
causing any paints, because it's on the newly created widget, so the presshell
is suppressing painting.

This also means that implementing Validate() on all of the platforms will work
equally as well as my patch.  Should we do that instead?

Comment 32

[Brian Ryner (not reading)]

Comment on attachment 55525 [details] [diff] [review]
patch to make nsView::CreateWidget not set the widget visibility

ok, per conversation with hyatt, we're going to eliminate usage of Validate in nsDocumentViewer, but leave it as an API on nsIWidget... which I also now have an implementation for on gtk.

Comment 33

[Brian Ryner (not reading)]

Attachment: patch v2

Comment 34

[Brian Ryner (not reading)]

Attachment: first-cut patch from danm to impl Validate on mac

Comment 35

[Simon Fraser [no longer active]]

Comment on attachment 55682 [details] [diff] [review]
first-cut patch from danm to impl Validate on mac

This won't work for Carbon. You need InvalidateWindowRgn/Rect.

Comment 36

[Christopher Blizzard (:blizzard)]

Comment on attachment 55680 [details] [diff] [review]
patch v2

r/sr=blizzard

Comment 37

[David Hyatt]

sr=hyatt (on 55680)

Comment 38

[Brian Ryner (not reading)]

Checked in the patch... should I close this bug and file a separate bug on the
extra painting that's happening on Mac?

Comment 39

[John Morrison]

The bryner-man. Bryner-roo. Buh-ryner-ino:    
  http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey-Testerbox

Comment 40

[Mike Jaques]

Someone was not looking too close at the numbers, if lower is assumed to be 
better, in milliseconds, then the numbers there are better after this checkin, 
not worse.  Someone was seeing the ghost of 1590'ish, when in fact the number 
is 1490'ish.   The before number was hovering around 1520.  so its a 70 
millisecond increase in load time :)

This is a great tool however for finding quickly and accurately who's at fault 
for what :)

Comment 41

[John Morrison]

Um, which of "The bryner-man. Bryner-roo. Buh-ryner-ino." led you to believe
that I was complaining.

Comment 42

[Mike Jaques]

oh, sorry ;)  people normally, at least to date, dont normally comment on 
tinderbox, comment on speed-ups :)  my mistake.

Comment 43

[Brian Ryner (not reading)]

Closing out this bug.

I filed mac-specific bug 107827 for Invalidates on hidden widgets causing
visible widgets to paint, and bug 107828 for implementing Validate on mac.