User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080929033431 Minefield/3.1b1pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080929033431 Minefield/3.1b1pre Long (tall) DIV's using the -moz-box-shadow CSS3 hack make FF run really slow - scrolling down the page can take a long time and is very jerky. When the box shadow is removed then the problem no longer exists. I've added a 'max-height' to the page in question as a slight work-around but the page still renders very slowly. Reproducible: Always Steps to Reproduce: 1. create a div, say 1000px high or more 2. add -moz-box-shadow to the css for the div 3. view page and scroll up/down Actual Results: Very slow to scroll up/down - also causes css menu to work very slowly. shorter pages do not have this issue. Expected Results: nice smooth scrolling!

see also: http://www.sunbridgeroadmission.org.uk/?id=8 (ie it's not the photo's causing it to go slow!) Try page-up and page-down for a better idea.

I tested the menus (on Windows) because the scrolling was already bad since about August 2006, it only got worse. Depends also on what machine you test it. This regressed on 7 July 2008 and the most likely cause is Bug 212633 :).

This is definitely due to the way we do box shadows, every time you scroll the 1000px div must be redrawn on the shadow context and blurred again, blurring is done on the CPU. I suppose we can somehow limit blurring to the dirty rect but I'm not sure how to perfect it.

It is not that bad on OS X, but scrolling is slower than the latest Fx 3.0x release.

This test allows on/off switching of box-shadow. Bug doesn't effect shadows without blur radius.

This patch will optimize how much the computer has to blur, by calculating a smaller rect that will still create the exact same blur result within the dirty rect. All box-shadow sites are now almost as smooth as butter, and I've tested it with a lot of sites that use box-shadow to deal with all the corner-cases I can think of (which is the reason this patch may be a little complex). The sooner we get hardware blur the better.

+ renderContext->SetFillRule(gfxContext::FILL_RULE_WINDING); Why do you need this? Why do you need to mess with the rounded corner stuff? It would seem to me that all you need to do here is compute a smaller temporary surface rectangle (which should not depend on the rounded corners), but the actual RoundedRectangle/Rectangle operation that draws the box we're to shadow does not need to change at all. In fact it seems to me the right thing to do would be to pass the dirty rect into the boxblur object and have that object do all the work to adjust the temporary surface size. That should be simpler and also make it possible for other blur users to take advantage of dirty-rect optimization.

Ah yes, this approach works much better. Thanks Roc, I don't know what I was thinking with that old approach or why this one didn't occur to me...

+ + // Only paint what's in the dirty rect + renderContext->NewPath(); + renderContext->Rectangle(dirtyGfxRect); + renderContext->Clip(); Don't do this. It's not necessary and could cause problems if the dirty rect is not pixel aligned.

Removed that bit of code and also found another small way to make it slightly faster.

As we discussed in real life, that clipping code is actually needed since we're not computing correct pixels for part of the shadow now, so we shouldn't try to draw them.

Yeah, I was not only wondering why the clipping wasn't needed, but why removing it didn't seem to change behaviour.

- shadowContext = blurringArea.Init(shadowRect, blurRadius, 1, renderContext); + shadowContext = blurringArea.Init(dirtyGfxRect, blurRadius, 1, renderContext); We should be intersecting shadowRect with dirtyGfxRect.

No. If the dirty rect only intersects a blurred edge of the shadow (which shadowRect doesn't include) then it won't get painted. I have a screenshot if you don't understand or you're skeptical :) However, shadowRectPlusBlur is available and sounds like what you want.

Yeah, I don't understand since the rectangle passed to blurringArea.Init is inflated by the blurRadius. Your shadowPaintRect.IsEmpty call would cause a problem if the intersection of the rectangles is empty, sure, so you should take it out. I guess the problem is that when the intersection of the rectangles is empty the rect's position becomes undefined. I reckon you should pass in the dirtyGfxRect as a parameter to blurringArea.Init so that it can inflate the shadowRect (effectively producing shadowRectPlusBlur) and then intersect with dirtyGfxRect. Then everyone's happy. Maybe nsContextBoxBlur can do the clipping to dirtyGfxRect too?

I think this is how you wanted it.

+ + gfxRect mDirtyRectIntersect; Add a comment explaining what this is. Actually I'd change the name to mRequiredShadowArea. + // Make sure its not EVEN_ODD like in some cases (box-shadow) and thus cause problems What does this mean? What you have here is correct, I think, but it produces temporary surfaces that are larger than necessary because gfxAlphaBlur outsets mDirtyRectIntersect by the blur radius but then does not intersect again with the shadow area.

Addresses comments.

+ // We need to find the rect which contains the pixels we want to return. + // This area could get slightly smaller still by intersecting this rect with rectWithBlur + // and make that the temp surface size, but that would require API changes + // which we won't do at this stage. This comment doesn't make sense because we *are* intersecting with rectWithBlur. How about just // Determine the area of the shadow we need. and then after blur.Init, // XXX the temporary surface will be the mRequiredShadowArea inflated by // blurRadius in each direction so that the required shadow pixels are computed // correctly. We could actually use a smaller temporary surface by observing // that where the temporary surface is outside the rectWithBlur, the pixel // values are guaranteed to be fully transparent, so we could intersect the // inflated mRequiredShadowArea with rectWithBlur to compute the temporary // surface area. But we're not doing that right now because it's a bit more // complex.

Change the comment.

I think we should actually file a followup bug, though, to fix that XXX comment. The way things are with this patch, when the entire shadow fits in the dirty rect (e.g. the object is entirely on the page), we'll always create a temporary surface which is the box inflated by 2*blurRadius in each direction. That's an annoying waste of work.

This fixes 466353 which is blocking, so this should be blocking

Pushed ce72e9a5dca0 to trunk.

Michael, please do file that followup bug.

Pushed 7324e52b2415 to 1.9.1

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081213 Shiretoko/3.1b3pre ID:20081213034757.

There is still a noticeable lag on attachment 342806 [details] when using the scroll bar and shadow enabled. Can we do more perf improvements?

(In reply to comment #28) > There is still a noticeable lag on attachment 342806 [details] when using the scroll bar > and shadow enabled. Can we do more perf improvements? Will this be handled by bug 468018? As I can see the patch on it is already checked into trunk but the perf issues are still there with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081213 Minefield/3.2a1pre ID:20081213020516

We probably just need to speed up gfxBlur with SSE or something. Can you file a separate bug for that?

(In reply to comment #30) > We probably just need to speed up gfxBlur with SSE or something. Can you file a > separate bug for that? Sure. It's bug 469589 now. Alfred has already verified it with 1.9.1b3. I can verify the perf improvements with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20081213 Minefield/3.2a1pre and on OS X. Setting flags accordingly. I would think that it's a good testcase for Litmus.