359392 - [FIX]Very slow rendering of the top bar on mozilla.org

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Current trunk Linux cairo build

STEPS TO REPRODUCE:
1)  Start the browser
2)  Load http://www.mozilla.org
3)  Hit the down arrow 5-6 times (enough to hide the image banner at the top),
    then the up arrow 5-6 times.  Repeat

EXPECTED RESULTS:  Scrolling with no problems.

ACTUAL RESULTS:  Bringing the top banner back into the viewport is really slow -- browser freezes up, X CPU usage spikes.

I'll attach some profiles in a sec...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: zipped-up jprof profile

Relevant bits:

Total hit count: 123537

102308 of them under _XReadPad (and most of those in _end).  The stack to that is:

_XReadPad
XGetImage
_get_image_surface
_cairo_xlib_surface_acquire_source_image
_cairo_surface_acquire_source_image
_cairo_pattern_acquire_surface_for_surface
_cairo_pattern_acquire_surface
_cairo_pattern_acquire_surfaces
_cairo_image_surface_composite
_cairo_surface_composite
_cairo_surface_fallback_composite
_cairo_surface_composite
_composite_trap_region
_clip_and_composite_trapezoids
_cairo_surface_fallback_fill
_cairo_surface_fill
_cairo_gstate_fill
INT__moz_cairo_fill_preserve
gfxContext::Fill()
nsThebesImage::ThebesDrawTile(gfxContext*, gfxPoint const&, gfxRect const&,
                              int, int)
nsThebesRenderingContext::DrawTile(imgIContainer*, int, int, nsRect const*)
nsCSSRendering::PaintBackgroundWithSC(nsPresContext*, nsIRenderingContext&,
                                      nsIFrame*, nsRect const&, nsRect const&,
                                      nsStyleBackground const&,
                                      nsStyleBorder const&,
                                      nsStylePadding const&, int, nsRect*)

Comment 2

[Boris Zbarsky [:bzbarsky]]

I tried doing a sysprof, and all it tells me is that 90% of the time is spent in /usr/X11R6/bin/X.  There's some lack of debuginfo X RPMs for Fedora Core 4... ;)

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

You're not going to like this answer, but you may need to update your X server (or your distro).  FC4 ships with an X server whose RENDER impl has a bug with tiled image drawing, so we can't use it for tiled image drawing.  Unfortunately, we draw all background images as always repeating (even if they're not actually repeating)...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Updating my distro is probably a non-starter (can't afford the downtime), and I see no update for X for FC4.  Do you know whether there's a Fedora bug on this?  I can't locate one...

In any case, it's not about me.  If I'm the only one who will run into this problem, no need to worry.  Worst case, I do exactly what I do now -- run non-cairo binaries (and if that becomes impossible to do with trunk Gecko I stop using trunk Gecko).  The real question is how many other users are affected by this and how hard it would be to improve it from "really slow" to "slower than the fast path, but bearable".

For example, the old GTK2 GFX manages to tile images in a sane time, even using the "slow" tiling to work around broken X (bug 256328)....  What's cairo doing differently?

Comment 5

[Boris Zbarsky [:bzbarsky]]

And of course the sane solution would be for Fedora to fix this so that Firefox 3 actually runs sanely...  Then again, I wouldn't be surprised if they've stopped updates of FC4 already.

Comment 6

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

We could make non-repeating images draw without repeating...

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

FC4 seems to have 6.8.2, which contains the bug, see http://lxr.mozilla.org/seamonkey/source/gfx/cairo/cairo/src/cairo-xlib-surface.c#101 .  I did some code a while back to fall back to X tiling when we can, see http://lxr.mozilla.org/seamonkey/source/gfx/cairo/cairo/src/cairo-xlib-surface.c#1208 -- but that can only work when we don't need to handle alpha.  FC4 will never see an update for this; the update is FC5 (and now FC6).

We no longer have 1-bit alpha; everything gets a full 8bpp alpha mask -- so we can't use 1bpp alpha with AND/OR operations.  Cairo's using XRender in this case, except where it knows that xrender is buggy, it'll end up going through the client-side software rendering path.

We could make not-really-repeating images draw without repeating, which would fix this specific case.  I had some semi-broken code to do that in nsImageThebes, but I recently rewrote the DrawTile function there and purposely got rid of that code.  That type of thing should live inside cairo -- it should be possible to make the xlib surface's composite function figure out whether it really needs to repeat or not.

Another alternative is to just expand out the repeats using single blits; I just wrote code to do this for win32 yesterday, and the basic algorithm would be the same for xlib.  Doing this wouldn't always be faster than just doing the expansion in software, however (e.g. if you're tiling a 10x10 image into a 500x500 square, it'll probably be faster to pull down the data, composite, and push it back than to do 2500 composite operations), but I ended up limiting the win32 case to falling back if more than 5 tiles had to be done in either direction.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Another alternative is to just expand out the repeats using single blits;

This sounds like the way to go to me (in terms of code simplicity, if nothing else).  I assume this would need to be done in cairo itself, right?  Can you possibly point me to where in cairo?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't tile when we don't have to

This makes us not tile when we don't absolutely have to.  This was simpler than doing it in cairo, since we had all the info we needed right there....  I'm not quite sure how to get some of it efficiently in cairo, and especially what function to then call to just paint the image).

This does help a _lot_ over here with mozilla.org, though not with actually repeated backgrounds (which I'll file a separate bug on).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 246745 [details] [diff] [review]
Don't tile when we don't have to

Actually, that uses the wrong offset in the case when there's more than one tile... I'll update the patch.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Slightly better

Actually, the first patch was correct after all.  This patch asserts the things that made it so, and uses a simpler test to detect the "inside a single tile" case.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix one more arithmetic bug

This should be the last patch, honest!  ;)

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 246930 [details] [diff] [review]
Fix one more arithmetic bug

This looks fine, I think; should help out on windows as well.

Comment 14

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

Comment on attachment 246930 [details] [diff] [review]
Fix one more arithmetic bug

Would be nice to have this in cairo as well. Someone else's problem I guess.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk for 1.9a1.  Thanks for the reviews!