Closed Bug 77002 Opened 24 years ago Closed 24 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: 24 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: