The default bug view has changed. See this FAQ.

Avoid eager painting of Web page content in SeaMonkey

VERIFIED FIXED in mozilla0.9.1

Status

()

Core
Layout
VERIFIED FIXED
16 years ago
10 months ago

People

(Reporter: David Hyatt, Assigned: David Hyatt)

Tracking

({perf})

Trunk
mozilla0.9.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(17 attachments)

9.75 KB, patch
Details | Diff | Splinter Review
798 bytes, patch
Details | Diff | Splinter Review
10.33 KB, patch
Details | Diff | Splinter Review
10.37 KB, patch
Details | Diff | Splinter Review
12.19 KB, patch
Details | Diff | Splinter Review
12.97 KB, patch
Details | Diff | Splinter Review
14.20 KB, patch
Details | Diff | Splinter Review
631 bytes, patch
Details | Diff | Splinter Review
1.75 KB, patch
Details | Diff | Splinter Review
1.83 KB, patch
Details | Diff | Splinter Review
16.53 KB, patch
Details | Diff | Splinter Review
819 bytes, patch
Details | Diff | Splinter Review
16.61 KB, patch
Details | Diff | Splinter Review
1.20 KB, patch
Details | Diff | Splinter Review
16.88 KB, patch
Details | Diff | Splinter Review
3.32 KB, patch
Details | Diff | Splinter Review
3.39 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

16 years ago
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.
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Summary: Avoid eager painting in SeaMonkey → Avoid eager painting of Web page content in SeaMonkey
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 1

16 years ago
Created attachment 31696 [details] [diff] [review]
Implement paint tuning in layout
(Assignee)

Comment 2

16 years ago
Created attachment 31697 [details] [diff] [review]
Patch to content.  If onload beats the timer, this ensures we unlock.
(Assignee)

Comment 3

16 years ago
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.
(Assignee)

Comment 4

16 years ago
Created attachment 31699 [details] [diff] [review]
Patch #2 to layout.  Keeps the caret from blinking while painting is locked.
(Assignee)

Comment 5

16 years ago
Created attachment 31704 [details] [diff] [review]
Add some more suppression to keep positioned blocks from painting.
(Assignee)

Comment 6

16 years ago
Created attachment 31708 [details] [diff] [review]
Enable paint suppression for XML and XUL as well.  Check out those prefs panels, baby!
(Assignee)

Comment 7

16 years ago
Adding the nsCatFood keyword.  This patch stops all UI jumping and jittering in 
the prefs panels.
Keywords: nsCatFood
(Assignee)

Comment 8

16 years ago
Created attachment 31709 [details] [diff] [review]
Suppress list controls.
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?

Comment 10

16 years ago
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
(Assignee)

Comment 11

16 years ago
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.

Comment 12

16 years ago
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.)

Comment 13

16 years ago
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

Comment 14

16 years ago
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.
(Assignee)

Comment 15

16 years ago
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?
(Assignee)

Comment 17

16 years ago
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?
(Assignee)

Comment 18

16 years ago
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.
(Assignee)

Comment 19

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

Comment 20

16 years ago
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.

Comment 21

16 years ago
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.
(Assignee)

Comment 25

16 years ago
Yeah, it occurred to me that you might have missed the content patch this 
morning.  Yay!
(Assignee)

Comment 26

16 years ago
Created attachment 31767 [details] [diff] [review]
Final patch to layout
(Assignee)

Comment 27

16 years ago
Created attachment 31768 [details] [diff] [review]
Final patch to content
(Assignee)

Comment 28

16 years ago
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.

Updated

16 years ago
Whiteboard: wanted for 0.9

Updated

16 years ago
Whiteboard: wanted for 0.9

Comment 29

16 years ago
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.
(Assignee)

Comment 31

16 years ago
Created attachment 31784 [details] [diff] [review]
Final patch to DOM
(Assignee)

Comment 32

16 years ago
Created attachment 31785 [details] [diff] [review]
Final patch #2 to DOM (adds null checks, fix a spelling error, heh)
sr=jst for the dom and content changes. r=jst for the layout changes.

Comment 34

16 years ago
add cc

Updated

16 years ago
Whiteboard: wanted for 0.9 → wanted for 0.9, needs to bake on trunk after branch
(Assignee)

Comment 35

16 years ago
Created attachment 31945 [details] [diff] [review]
Final patch #2 to layout.
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
(Assignee)

Comment 37

16 years ago
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.
(Assignee)

Comment 38

16 years ago
I'm going to let this wait until 0.9.1.
Target Milestone: mozilla0.9 → mozilla0.9.1

Comment 39

16 years ago
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

Comment 40

16 years ago
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
(Assignee)

Comment 43

16 years ago
Created attachment 32053 [details] [diff] [review]
Final patch to libpr0n. Avoid loadgroups on cached images.
(Assignee)

Comment 44

16 years ago
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...

Updated

16 years ago
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.
(Assignee)

Comment 47

16 years ago
Created attachment 32104 [details] [diff] [review]
Final patch #3 to layout.
(Assignee)

Comment 48

16 years ago
Created attachment 32105 [details] [diff] [review]
Final patch #2 to content.
(Assignee)

Comment 49

16 years ago
Created attachment 32113 [details] [diff] [review]
Final patch #4 to layout. Address hixie's concerns about onload switching to a failed page.
(Assignee)

Comment 50

16 years ago
Created attachment 32126 [details] [diff] [review]
Final patch #2 to libpr0n.

Comment 51

16 years ago
r=pavlov on patch 32126 (final patch #2 to libpr0n.)
(Assignee)

Comment 52

16 years ago
Created attachment 32129 [details] [diff] [review]
Final patch #3 to libpr0n. Missed the interlacing passes.

Comment 53

16 years ago
er, r=pavlov on the new patch

Comment 54

16 years ago
     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.
(Assignee)

Comment 55

16 years ago
Ok, done.
(Assignee)

Comment 56

16 years ago
Landed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 57

16 years ago
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?

Comment 58

16 years ago
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.

Comment 59

16 years ago
Filed bug 77634 for pages not being rendered.

Comment 60

16 years ago
Filed bug 77653 to track this issue.

Comment 61

16 years ago
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.

Comment 62

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

Comment 63

16 years ago
Markimg verified per last comments.
Status: RESOLVED → VERIFIED
See Also: → bug 1271691
You need to log in before you can comment on or make changes to this bug.