Closed Bug 97895 Opened 23 years ago Closed 23 years ago

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

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.6

People

(Reporter: sfraser_bugs, Assigned: bryner)

References

Details

Attachments

(3 files, 2 obsolete files)

Stack for the pre-page leave Invalidate() call that causes a redraw
4.12 KB, text/plain
Details
patch v2
8.70 KB, patch
Details | Diff | Splinter Review
first-cut patch from danm to impl Validate on mac
1.95 KB, patch
Details | Diff | Splinter Review 
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.
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.

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.

Reassigning to dbaron, or if he objects, maybe waterson?
Assignee: karnaze → dbaron
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.

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?
Summary: Old page gets redrawn when leaving a page (Back and Forward performance) → Old page gets repainted when leaving a page (Back and Forward performance)
Severity: normal → major
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.6
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.
Blocks: 91351
Check this web site...

http://www.fileplanet.com/

redraws after redraws here onload and unload.

I can take this one.
Assignee: dbaron → bryner
Status: ASSIGNED → NEW
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.
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.

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

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.

roc, do you have any thoughts on this?

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

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?

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

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

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?
> 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.
>> 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);
  }

Hmm, I stand corrected.

Now I wonder how our support for visible frames inside hidden frames is working.
Maybe it isn't.
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).


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?
Attachment #55495 - Attachment is obsolete: true
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
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?

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 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.
Attachment #55525 - Attachment is obsolete: true
Attached patch patch v2 — — Splinter Review 

    
    

    
  
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.

        Attachment #55682 -
        Flags: needs-work+

    
    

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

r/sr=blizzard

    
    

    
  
sr=hyatt (on 55680)


    
    

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


    
    

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

    
    

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

    
    

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

    
    

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

    
    

    
  
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.

Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: