Closed Bug 77002 Opened 23 years ago Closed 23 years ago

Avoid eager painting of Web page content in SeaMonkey

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.1

People

(Reporter: hyatt, Assigned: hyatt)

References

Details

(Keywords: perf)

Attachments

(17 files)

Implement paint tuning in layout
9.75 KB, patch
Details | Diff | Splinter Review
Patch to content. If onload beats the timer, this ensures we unlock.
798 bytes, patch
Details | Diff | Splinter Review
Patch #2 to layout. Keeps the caret from blinking while painting is locked.
10.33 KB, patch
Details | Diff | Splinter Review
Add some more suppression to keep positioned blocks from painting.
10.37 KB, patch
Details | Diff | Splinter Review
Enable paint suppression for XML and XUL as well. Check out those prefs panels, baby!
12.19 KB, patch
Details | Diff | Splinter Review
Suppress list controls.
12.97 KB, patch
Details | Diff | Splinter Review
Final patch to layout
14.20 KB, patch
Details | Diff | Splinter Review
Final patch to content
631 bytes, patch
Details | Diff | Splinter Review
Final patch to DOM
1.75 KB, patch
Details | Diff | Splinter Review
Final patch #2 to DOM (adds null checks, fix a spelling error, heh)
1.83 KB, patch
Details | Diff | Splinter Review
Final patch #2 to layout.
16.53 KB, patch
Details | Diff | Splinter Review
Final patch to libpr0n. Avoid loadgroups on cached images.
819 bytes, patch
Details | Diff | Splinter Review
Final patch #3 to layout.
16.61 KB, patch
Details | Diff | Splinter Review
Final patch #2 to content.
1.20 KB, patch
Details | Diff | Splinter Review
Final patch #4 to layout. Address hixie's concerns about onload switching to a failed page.
16.88 KB, patch
Details | Diff | Splinter Review
Final patch #2 to libpr0n.
3.32 KB, patch
Details | Diff | Splinter Review
Final patch #3 to libpr0n. Missed the interlacing passes.
3.39 KB, patch
Details | Diff | Splinter Review 
We eagerly paint in SeaMonkey because of event prioritization.  The following 
patches bring SeaMonkey to the same speed level as MFCEmbed and WinEmbed, and 
they do it in a way that all platforms should benefit from without having to 
think about event queues.
Status: NEW → ASSIGNED
Summary: Avoid eager painting in SeaMonkey → Avoid eager painting of Web page content in SeaMonkey
Target Milestone: --- → mozilla0.9

    
    

    
  
This patch on Win32 on my machine is about a 10-15% speedup (WOOF WOOF).  I'd 
like to see what effect it has on Mac and Linux.


    
    

    
  
Adding the nsCatFood keyword.  This patch stops all UI jumping and jittering in 
the prefs panels.

Keywords: nsCatFood

    
    

    
  


  

    
    

    
  
in nsIPresShell.cpp the following comment is no longer true:

+   * THIS APPLIES ONLY TO HTML DOCUMENTS

I notice that in nsCaret.cpp you null check the presShell, but you don't in
other cases; is that as you intended?

This comment in nsImageFrame.cpp is now slightly wrong:

+    // have to cover <img> in addition to <body> because of input image       \
 controls.

It should be "the canvas frame" instead of "<body>".

In a similar vain: 

+  PRBool mPaintingLockedDown; // For HTML documents only we initially lock    \
 down painting.

...shouldn't have the conditional in the comment.

Regarding all the special casing for <div> (input controls) <img> (image inputs)
and <select>s, is that all just because they have native widget thingies? And if
so does that mean that all that will go away with XBL form controls? And does it
mean that floats and absolutely positioned blocks are NOT affected by this, when
they should?

    
    

    
  
Apparently this should be Plat/OS All/All, setting as such for better searching,
adding perf keyword.

Keywords: perf
OS: Windows 2000 → All
Hardware: PC → All

    
    

    
  
Hixie, I only have to do that in nsCaret because it can outlive the presshell 
and uses a weak reference to examine the presshell.

The special casing for blocks, image inputs, and list controls is widget-
related, yes, and it will stay even with XBL form controls.


    
    

    
  
Cool!

- Does ``painting locked down'' mean ``suppress painting''? If so, might
  that be a better name to use? (e.g., IsPaintingSuppressed(),
  mPaintingIsSuppressed, etc.)

- Why not just add a Paint() method to nsCanvasFrame, and have it do the
  suppression? (Instead of checking frame type in nsHTMLContainerFrame.)
  roc is probably going to add the method anyway for different reasons.

- Does nsImageFrame still need to be special cased? Is that for the throbber
  or something?

- Will we end up with cases where there are ``dead areas''? For example,
  on a very slow link? (Looks like not, because the timer will just time-out.)

  What about if the event queue gets flooded? I seem to recall that we only
  service timers after we've serviced all the events in the event queue? (A
  quick check might be to try it with a very long document; e.g., LXR.)

Nice work!

(cc'ing people who know more about painting than I do.)



    
    

    
  
Compiles fine on linux, the prefs panel is ma-gni-fi-cient with this! The rest
looks a little faster too, but that's maybe just psychological :P

    
    

    
  
Mac numbers show ~5% speedup on my G4 500. Odd spread though, 
some pages show 0 change with others getter a 100-200ms boost. 
Definiltey looks better visually.

My P3 500 running Win2k shows a pretty steady 100ms speedup on all 
pages, very evenly distributed.

    
    

    
  
Chris, addressing your second point about making a paint method in canvasframe. 

(1) You still want the background to paint, which means you need all that code 
in the base class paint method to execute, but you don't want kids to paint.  I 
didn't want to duplicate that code in the canvas class.
(2) The base class *already* retrieves the frameType for another test, so 
there's no cost in putting it in the base class.
(3) nsImageFrame is special-cased for <input type=image> controls.  

I tested with long docs and didn't see any problems... the timer just fires and 
the page shows.  SeaMonkey is actually better at showing long docs early than 
MFCEmbed is right now.


    
    

    
  
I've tried the last patch in this bug and I think that it still has some
problems.  For example, loading mail messages actually feels slower to me.  I
think that it's because it isn't being shown until the timer actually fires and
it used to take less than 1.2 seconds for mail messages to be shown before.  I
see this in other places too including password dialogs.  The dialog will come
up, draw grey, there is a long pause ( feels like ~1.2 seconds ) and then the
dialog will fill in.  Any ideas?

    
    

    
  
If the document completes, I paint.  The timer becomes irrelevant.  Now this
does mean there will be situations with documents that take > 1.2 seconds to
full load in which you might not see content as early as you did before.  

For example, if a mail message was going to take 2 seconds to load, in SeaMonkey
before my patch, you may have seen some data after only 0.5 seconds and then
been locked waiting for the rest of the message to load.

With my patch you won't see jack for 1.2 seconds minimum (if the page takes
longer than 1.2 seconds to load).

I found this time delay to be just fine for normal use in both chrome and
content and have been using a build with this patch applied all weekend in both
chrome and content without seeing any slowdown (in fact, I see just the opposite).  

Maybe there's a Linux issue with timer starvation that keeps us from painting
after 1.2 seconds?

    
    

    
  
The password dialog clearly loads in well under 1.2 seconds, at which point I
immediately invalidate.  I tested mailnews and dialogs and don't see any of the
effects you describe on Win32.

I'm confused because in the case of dialogs, the timer should be irrelevant,
i.e., the chrome easily beats the timer and so painting should be unlocked very
rapidly.


    
    

    
  
In fact, on my machine mail messages display *faster*, since they load in far
less than 1.2 seconds and only do 1 paint.


    
    

    
  
David, I suggest (seriously) that you grab a P300 machine with Linux on it, and
try an optimised build both before and after your patch here. We wouldn't want
to end up with another bug for Linux specifically as bug 77051 painfully
illustrates.


    
    

    
  
fwiw, the (perceived) speed of the password dialog popup is ~0.4secs to open,
with another ~0.2secs to fill it, which is still noticably slower than other GTK
apps to bring up dialogs. Very rough guide, and without any of the patches in
this bug.


    
    

    
  
Dave, I'm talking about documents that take far less than 1.2 seconds to paint
here.  An example of what I'm talking about include the bugmail messages.  They
are small and fast to load.  The build that I'm using right now doesn't include
your patch and they load in far less than one second.  With the patch they are
definitely bound to the full 1.2 second pause.

There could be some timer starvation issues on linux but I doubt it and I don't
see how a timer starvation issue could cause this.  Let me do some more digging.

jg, I realize that you have problems with popup menus ( yes, they could be
faster! ) but this bug isn't releated to that.

    
    

    
  
This is where I announce to the world that I am a Dork.  Shaver pointed out that
I've missed applying attachment 31697 [details] [diff] [review] which would cause the symptoms that I've
been seeing.  Rebuilding. :)

    
    

    
  
Yep.  Works fine.

    
    

    
  
Yeah, it occurred to me that you might have missed the content patch this 
morning.  Yay!


    
    

    
  


  

    
    

    
  


  

    
    

    
  
Ok, I'm ready for r and sr.  I've made Hixie's and waterson's changes.  You can 
see the final patches in the bug.


    
  
Whiteboard: wanted for 0.9

    
  
Whiteboard: wanted for 0.9

    
    

    
  
I assume achimha@innotek.de accidentaly removed the stuff in the status field. 
Reverting back...
Whiteboard: wanted for 0.9

    
    

    
  
Lots of comments, nice clear code, I like it.

+    // Uh-oh.  We must be out of memory.  No point in keeping painting locked down.
+    mPaintingSuppressed = PR_FALSE;

You should probably return NS_ERROR_OUT_OF_MEMORY there, though it's possible
that you fail for some other reason (unregistered timer factory?).

sr=shaver on both "Final patch"es.


    
    

    
  


  

    
    

    
  
sr=jst for the dom and content changes. r=jst for the layout changes.

    
    

    
  
add cc

    
  
Whiteboard: wanted for 0.9 → wanted for 0.9, needs to bake on trunk after branch

    
    

    
  


  

    
    

    
  
Nit alert: almost all calls to IsPaintingSuppressed pass a PRBool that's been
initialized to PR_FALSE, but this one (at least) doesn't:


 PRBool nsCaret::MustDrawCaret()
 {
+  nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(mPresShell);
+  if (presShell) {
+    PRBool isPaintingSuppressed;
+    presShell->IsPaintingSuppressed(&isPaintingSuppressed);
+    if (isPaintingSuppressed)

/be

    
    

    
  
It doesn't matter.  I always init the out result in the function.  I only need
to init it to false in cases where the var is defined outside a null check for
the presshell.


    
    

    
  
I'm going to let this wait until 0.9.1.

Target Milestone: mozilla0.9 → mozilla0.9.1

    
    

    
  
Cleared the status whiteboard from 0.9 stuff since this has been retargeted for
0.9.1.
Whiteboard: wanted for 0.9, needs to bake on trunk after branch

    
    

    
  
I imagine that this could be an even bigger win on
GTK than on mswindows since painting is more expensive
in the former.  Any numbers?

My only concern is the greater length of time taken
to reach that first paint.  After that, great, suppress
away, but getting the first meaningful paint in front of
the user's eyes seems important in terms of perceived
speed.


    
    

    
  
in fact, delaying the first paint and painting more actually makes it feel 
faster. You see all the content at once. Also the current behavior paints very 
quickly, but you still can't read the page easily since it jiggles around a lot 
as images come in -- very distracting! The new way does more or less just one 
paint and it's all there at one time. Feels very professional and _snappy_. The 
current behavior feels lethargic, even if technically fastest to display any 
content.


    
    

    
  
hyatt: I know IsPaintingSuppressed is infallible, so this is only a nit -- it's
just that many (most) cases where the caller initializes the PRBool do not have
any presshell null check.

So it's just a foolish consistency thing, or else an XPCOM purity thing if you
*don't* want to assume IsPaintingSuppressed cannot fail.  Too bad we can't use a
concrete class with a boolean getter here -- it's not as if anyone is able to
substitute another impl. of nsIPresShell into Gecko!

/be

    
    

    
  
The following patch fixes a nasty problem in libpr0n.  It wasted time putting 
cached images (memory cache) into and out of loadgroups, which waste time 
calling into JS to update the SeaMonkey chrome.

This happened in particular for scrollbars.


    
    

    
  
Adam: Don't worry, with this patch we get an "initial paint" with only the
background of the page, then AT MOST 1.2 seconds later we get a second paint
with the entire top of the page, then as more of the document comes in we get
more paints as the rest of the page comes in.

Hyatt: dude, your libpr0n patch has tabs in it...

    
  
Blocks: 71668

    
    

    
  
just ran these tests on mac and saw a 17% improvement on jrgm's 
pageload tests....and oh my god, it _feels_ much faster.

    
    

    
  


  

    
    

    
  


  

    
    

    
  


  

    
    

    
  
r=pavlov on patch 32126 (final patch #2 to libpr0n.)

    
    

    
  
er, r=pavlov on the new patch

    
    

    
  
     SetOverflowClipRect(aRenderingContext);
   }
 
+  PRBool paintingSuppressed = PR_FALSE;  
+  nsCOMPtr<nsIPresShell> shell;
+  aPresContext->GetShell(getter_AddRefs(shell));
+  shell->IsPaintingSuppressed(&paintingSuppressed);
+  if (paintingSuppressed)
+    return NS_OK;
+

Should be shifted outside of:
SetOverflowClipRect(aRenderingContext);
[...]
aRenderingContext.PopState(clipState);

Otherwise the early return could leave the rendering context in a dangling 
state.

    
    

    
  
Ok, done.


    
    

    
  
Landed on trunk.

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

    
    

    
  
This seems to cause us to just not paint at _all_ on long pages (well, not 
paint until the page has completely loaded). For example, try this URL:

http://lxr.mozilla.org/mozilla/source/layout/html/style/src/nsCSSFrameConstructo
r.cpp

Was this the intended effect?

    
    

    
  
I'm seeing that too, Chris, in fact google.co.uk and mozilla.org both either
render not at all or only partially. Pulling something over the window to force
a re-draw clears it.

This build certainly shouldn't pass smoketests. FWIW I pulled shortly after
hyatt's checking was done and tinderboxes seemed ok with it; I blew away my
Cache and NewCache dirs, fired up my build and google.co.uk was the first casualty.


    
    

    
  
Filed bug 77634 for pages not being rendered.


    
    

    
  
Filed bug 77653 to track this issue.

    
    

    
  
Comparing the post carpool builds to the verif. builds from this a.m., here 
are the improvements I see. (Running over internal LAN, 500/128 win98, 500/128 
Linux, 450/256 G4).

                      First visit   Subseq. visits
win98                      4%             10%
linux                      3%              7%
mac (with virus on)        3%              8%
mac (with virus off)      13%             16%

Note: ref. bug 75988, I tested Mac both with the virus checker enabled (as it 
has previously been tested), and with it disabled.

    
    

    
  
Filed bug 77989 for a regression involving data: urls that might have been 
caused by this fix.

    
    

    
  
Markimg verified per last comments.
Status: RESOLVED → VERIFIED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: