Last Comment Bug 458031 - Long (tall) DIVs using -moz-box-shadow with blur radius make Firefox run really slowly
: Long (tall) DIVs using -moz-box-shadow with blur radius make Firefox run real...
Status: VERIFIED FIXED
: perf, regression, testcase, verified1.9.1
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: All All
P2 normal with 3 votes (vote)
: mozilla1.9.2a1
Assigned To: Michael Ventnor
:
: Jet Villegas (:jet)
Mentors:
http://www.sunbridgeroadmission.org.u...
Depends on:
Blocks: 212633 466353 468018
  Show dependency treegraph
 
Reported: 2008-10-01 06:32 PDT by Matthew Morley
Modified: 2008-12-14 16:05 PST (History)
13 users (show)
roc: blocking1.9.1+
hskupin: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase with slowing impact of shadow effect (17.85 KB, text/html)
2008-10-08 06:11 PDT, Alfred Kayser
no flags Details
testcase2 (350 bytes, text/html)
2008-10-12 13:45 PDT, j.j.
no flags Details
Patch (10.24 KB, patch)
2008-11-25 13:25 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 2 (3.54 KB, patch)
2008-11-25 14:57 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 3 (3.37 KB, patch)
2008-11-30 15:24 PST, Michael Ventnor
roc: review+
roc: superreview+
Details | Diff | Splinter Review
Patch 4 (3.03 KB, patch)
2008-11-30 20:31 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 5 (3.16 KB, patch)
2008-11-30 20:55 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 6 (8.63 KB, patch)
2008-12-01 14:17 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 7 (8.71 KB, patch)
2008-12-01 15:32 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 8 (9.02 KB, patch)
2008-12-01 15:48 PST, Michael Ventnor
roc: review+
roc: superreview+
Details | Diff | Splinter Review

Description User image Matthew Morley 2008-10-01 06:32:15 PDT
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 User image Matthew Morley 2008-10-01 06:34:45 PDT
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 User image Ria Klaassen (not reading all bugmail) 2008-10-01 12:06:45 PDT
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 User image Michael Ventnor 2008-10-01 17:04:55 PDT
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 User image philippe (part-time) 2008-10-01 18:56:29 PDT
It is not that bad on OS X, but scrolling is slower than the latest Fx 3.0x release.
Comment 5 User image Alfred Kayser 2008-10-08 06:11:43 PDT
Created attachment 342231 [details]
Testcase with slowing impact of shadow effect
Comment 6 User image j.j. 2008-10-12 13:45:07 PDT
Created attachment 342806 [details]
testcase2

This test allows on/off switching of box-shadow.
Bug doesn't effect shadows without blur radius.
Comment 7 User image Michael Ventnor 2008-11-25 13:25:14 PST
Created attachment 350044 [details] [diff] [review]
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-11-25 13:42:59 PST
+    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 User image Michael Ventnor 2008-11-25 14:57:10 PST
Created attachment 350065 [details] [diff] [review]
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-11-30 14:17:34 PST
+
+    // 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 User image Michael Ventnor 2008-11-30 15:24:38 PST
Created attachment 350688 [details] [diff] [review]
Patch 3

Removed that bit of code and also found another small way to make it slightly faster.
Comment 12 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-11-30 20:19:41 PST
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 User image Michael Ventnor 2008-11-30 20:31:24 PST
Created attachment 350711 [details] [diff] [review]
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-11-30 20:37:45 PST
-    shadowContext = blurringArea.Init(shadowRect, blurRadius, 1, renderContext);
+    shadowContext = blurringArea.Init(dirtyGfxRect, blurRadius, 1, renderContext);

We should be intersecting shadowRect with dirtyGfxRect.
Comment 15 User image Michael Ventnor 2008-11-30 20:55:34 PST
Created attachment 350714 [details] [diff] [review]
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-01 00:45:14 PST
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 User image Michael Ventnor 2008-12-01 14:17:33 PST
Created attachment 350832 [details] [diff] [review]
Patch 6

I think this is how you wanted it.
Comment 18 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-01 15:04:16 PST
+
+  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 User image Michael Ventnor 2008-12-01 15:32:27 PST
Created attachment 350851 [details] [diff] [review]
Patch 7

Addresses comments.
Comment 20 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-01 15:41:40 PST
+  // 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 User image Michael Ventnor 2008-12-01 15:48:51 PST
Created attachment 350859 [details] [diff] [review]
Patch 8

Change the comment.
Comment 22 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-01 15:51:55 PST
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-01 15:56:38 PST
This fixes 466353 which is blocking, so this should be blocking
Comment 24 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-02 13:39:50 PST
Pushed ce72e9a5dca0 to trunk.
Comment 25 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-02 13:41:03 PST
Michael, please do file that followup bug.
Comment 26 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-12 01:10:14 PST
Pushed 7324e52b2415 to 1.9.1
Comment 27 User image Alfred Kayser 2008-12-14 03:26:28 PST
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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2008-12-14 05:44:59 PST
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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2008-12-14 11:54:56 PST
(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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-14 13:37:04 PST
We probably just need to speed up gfxBlur with SSE or something. Can you file a separate bug for that?
Comment 31 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2008-12-14 16:05:45 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.