458031 - Long (tall) DIVs using -moz-box-shadow with blur radius make Firefox run really slowly

Status: VERIFIED FIXED

(Core :: Web Painting, defect, P2)

Description

[Matthew Morley]

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!

Comment 1

[Matthew Morley]

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.

Comment 2

[Ria Klaassen (not reading all bugmail)]

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 :).

Comment 3

[Michael Ventnor]

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.

Comment 4

[philippe (part-time)]

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

Comment 5

[Alfred Kayser]

Attachment: Testcase with slowing impact of shadow effect

Comment 6

[j.j.]

Attachment: testcase2

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

Comment 7

[Michael Ventnor]

Attachment: Patch

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.

Comment 8

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

+    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.

Comment 9

[Michael Ventnor]

Attachment: Patch 2

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...

Comment 10

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

+
+    // 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.

Comment 11

[Michael Ventnor]

Attachment: Patch 3

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

Comment 12

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

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.

Comment 13

[Michael Ventnor]

Attachment: Patch 4

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

Comment 14

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

-    shadowContext = blurringArea.Init(shadowRect, blurRadius, 1, renderContext);
+    shadowContext = blurringArea.Init(dirtyGfxRect, blurRadius, 1, renderContext);

We should be intersecting shadowRect with dirtyGfxRect.

Comment 15

[Michael Ventnor]

Attachment: Patch 5

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.

Comment 16

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

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?

Comment 17

[Michael Ventnor]

Attachment: Patch 6

I think this is how you wanted it.

Comment 18

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

+
+  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.

Comment 19

[Michael Ventnor]

Attachment: Patch 7

Addresses comments.

Comment 20

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

+  // 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.

Comment 21

[Michael Ventnor]

Attachment: Patch 8

Change the comment.

Comment 22

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

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.

Comment 23

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

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

Comment 24

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

Pushed ce72e9a5dca0 to trunk.

Comment 25

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

Michael, please do file that followup bug.

Comment 26

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

Pushed 7324e52b2415 to 1.9.1

Comment 27

[Alfred Kayser]

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

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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

Comment 30

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

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

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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.