The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Layout: View Rendering
P2
normal
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Matthew Morley, Assigned: Michael Ventnor)

Tracking

(4 keywords)

Trunk
mozilla1.9.2a1
perf, regression, testcase, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

9 years ago
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!
(Reporter)

Comment 1

9 years ago
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 :).
Blocks: 212633
Component: General → Style System (CSS)
Keywords: regression, testcase-wanted
Product: Firefox → Core
QA Contact: general → style-system
Version: unspecified → Trunk
Flags: blocking1.9.1?
Keywords: perf
Summary: Long (tall) DIV's using the -moz-box-shadow CSS3 hack make FF run really slow → Long (tall) DIVs using -moz-box-shadow make Firefox run really slowly
Component: Style System (CSS) → Layout: View Rendering
QA Contact: style-system → layout.view-rendering
(Assignee)

Comment 3

9 years ago
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

9 years ago
It is not that bad on OS X, but scrolling is slower than the latest Fx 3.0x release.
OS: Windows XP → All
Hardware: PC → All

Comment 5

9 years ago
Created attachment 342231 [details]
Testcase with slowing impact of shadow effect
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 6

9 years ago
Created attachment 342806 [details]
testcase2

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

Updated

9 years ago
Keywords: testcase-wanted → testcase
Summary: Long (tall) DIVs using -moz-box-shadow make Firefox run really slowly → Long (tall) DIVs using -moz-box-shadow with blur radius make Firefox run really slowly
Assignee: nobody → ventnor.bugzilla
Flags: blocking1.9.1? → wanted1.9.1+
(Assignee)

Comment 7

8 years ago
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.
Attachment #350044 - Flags: superreview?(roc)
Attachment #350044 - Flags: review?(roc)
+    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.
(Assignee)

Comment 9

8 years ago
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...
Attachment #350044 - Attachment is obsolete: true
Attachment #350065 - Flags: superreview?(roc)
Attachment #350065 - Flags: review?(roc)
Attachment #350044 - Flags: superreview?(roc)
Attachment #350044 - Flags: review?(roc)
Blocks: 466353
+
+    // 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.
(Assignee)

Comment 11

8 years ago
Created attachment 350688 [details] [diff] [review]
Patch 3

Removed that bit of code and also found another small way to make it slightly faster.
Attachment #350065 - Attachment is obsolete: true
Attachment #350688 - Flags: superreview?(roc)
Attachment #350688 - Flags: review?(roc)
Attachment #350065 - Flags: superreview?(roc)
Attachment #350065 - Flags: review?(roc)
Attachment #350688 - Flags: superreview?(roc)
Attachment #350688 - Flags: superreview+
Attachment #350688 - Flags: review?(roc)
Attachment #350688 - Flags: review+
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.
(Assignee)

Comment 13

8 years ago
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.
Attachment #350688 - Attachment is obsolete: true
Attachment #350711 - Flags: superreview?(roc)
Attachment #350711 - Flags: review?(roc)
-    shadowContext = blurringArea.Init(shadowRect, blurRadius, 1, renderContext);
+    shadowContext = blurringArea.Init(dirtyGfxRect, blurRadius, 1, renderContext);

We should be intersecting shadowRect with dirtyGfxRect.
(Assignee)

Comment 15

8 years ago
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.
Attachment #350711 - Attachment is obsolete: true
Attachment #350714 - Flags: superreview?(roc)
Attachment #350714 - Flags: review?(roc)
Attachment #350711 - Flags: superreview?(roc)
Attachment #350711 - Flags: review?(roc)
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?
(Assignee)

Comment 17

8 years ago
Created attachment 350832 [details] [diff] [review]
Patch 6

I think this is how you wanted it.
Attachment #350714 - Attachment is obsolete: true
Attachment #350832 - Flags: superreview?(roc)
Attachment #350832 - Flags: review?(roc)
Attachment #350714 - Flags: superreview?(roc)
Attachment #350714 - Flags: review?(roc)
+
+  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.
(Assignee)

Comment 19

8 years ago
Created attachment 350851 [details] [diff] [review]
Patch 7

Addresses comments.
Attachment #350832 - Attachment is obsolete: true
Attachment #350851 - Flags: superreview?(roc)
Attachment #350851 - Flags: review?(roc)
Attachment #350832 - Flags: superreview?(roc)
Attachment #350832 - Flags: review?(roc)
+  // 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.
(Assignee)

Comment 21

8 years ago
Created attachment 350859 [details] [diff] [review]
Patch 8

Change the comment.
Attachment #350851 - Attachment is obsolete: true
Attachment #350859 - Flags: superreview?(roc)
Attachment #350859 - Flags: review?(roc)
Attachment #350851 - Flags: superreview?(roc)
Attachment #350851 - Flags: review?(roc)
Attachment #350859 - Flags: superreview?(roc)
Attachment #350859 - Flags: superreview+
Attachment #350859 - Flags: review?(roc)
Attachment #350859 - Flags: review+
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
Flags: wanted1.9.1+ → blocking1.9.1+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Priority: -- → P2
Whiteboard: [needs landing]
Pushed ce72e9a5dca0 to trunk.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Michael, please do file that followup bug.
Pushed 7324e52b2415 to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Target Milestone: --- → mozilla1.9.1b3

Comment 27

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

Updated

8 years ago
Blocks: 468018
There is still a noticeable lag on attachment 342806 [details] when using the scroll bar and shadow enabled. Can we do more perf improvements?
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
(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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.