768775 - Gradient perf is unusably slow on test case

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

Test case:
http://estelle.github.com/mobileperf/#slide14

Profile:
http://people.mozilla.org/~bgirard/cleopatra/?report=AMIfv96oQne_-iA9yvusK2c_xPq2XCLcoJk67PE7MsQMtNOphnzr3DxVtg7gKhAcrajRN0kiqtlfGG1z_3h66deiYF8fBZ_wqOJatXDfu3160FzwxCg3HzMk8HIfKsqE8HK_VrhkOc85Yh9nrPKpGgvLSHAhuaVeRg

We're orders of magnitude slower then chrome. There's certainly some low hanging fruits here.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: A reduced test case

The reason webkit is so much faster is because it draws the gradient to an image and tiles that instead of drawing a repeating gradient over the size of the entire screen.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

FWIW, the webkit code is:
  WebCore::GeneratedImage::drawPattern()

Comment 3

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

The problem is getting pixel-snapping and joins between tiles right.

Comment 4

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

Attachment: fix

This should make things a lot better. In all the cases of small gradients being repeated that I've seen, they have been axis-aligned linear gradients, and this optimization should kick in.

Comment 5

[Benoit Girard (:BenWa)]

This patch makes a huge improvement! Not quite as good as WebKit on this page but at least the page works :).

I notice that when I'm spanning right with the arrow keys some slides disappear.

Comment 6

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

I had tried to push this, but there were reftest failures so I backed it out:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=61b94ce72be4

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/61b94ce72be4 is my attempt to fix some OSX failures, but wasn't quite enough:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13153677&tree=Mozilla-Inbound&full=1

There were other failures on Windows D2D:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13155509&tree=Mozilla-Inbound&full=1
These Windows failures are concerning. The basic issue is sampling. It's clearest in the linear-vertical*.html tests. The first row of pixels is generated partially by sampling the last stop of the gradient. In linear-repeat-* it's also quite obvious; there the last row of pixels is generated partially by sampling the first stop of the gradient. These artifacts are quite obvious and need to be fixed somehow.

Comment 8

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

The test failures are because Windows D2D is currently using Thebes to draw content in reftests :-(. I have a fix for the cairo-D2D code that would probably fix that, but a better approach is to fix bug 772726 so that on Tinderbox Windows D2D uses Azure to draw content in reftests.

Comment 9

[Binyamin]

Need to be fixed before Firefox 16 release what will support unprefixed "repeating-linear-gradient" http://hacks.mozilla.org/2012/07/aurora-16-is-out/ otherwise it will bring Firefox bad experience and tons of failures.

Comment 10

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

Attachment: Improve precision of bounds for cairo-D2D repeating stops

This reduces the effects of bug 582236.

Comment 11

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

Attachment: add some fuzziness/fail for reftests

Comment 12

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

Note that the additional fixes here are only needed because of bug 772726. I decided not to block relanding this on bug 772726, partly because I want to uplift the fixes here to Aurora and I don't want to land bug 772726 on Aurora.

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee21073be123
https://hg.mozilla.org/integration/mozilla-inbound/rev/874a4984caf2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e637d1f8ec6

Comment 14

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

Comment on attachment 637302 [details] [diff] [review]
fix

Review of attachment 637302 [details] [diff] [review]:
-----------------------------------------------------------------

This is a performance improvement that makes some pages *much* better. We should take it on Aurora because we have unprefixed CSS gradients there and we don't want to be stuck with super-bad performance in some cases.

Comment 15

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

Comment on attachment 645581 [details] [diff] [review]
Improve precision of bounds for cairo-D2D repeating stops

Review of attachment 645581 [details] [diff] [review]:
-----------------------------------------------------------------

This is a performance improvement that makes some pages *much* better. We should take it on Aurora because we have unprefixed CSS gradients there and we don't want to be stuck with super-bad performance in some cases.

Comment 16

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

Comment on attachment 645587 [details] [diff] [review]
add some fuzziness/fail for reftests

Review of attachment 645587 [details] [diff] [review]:
-----------------------------------------------------------------

This is a performance improvement that makes some pages *much* better. We should take it on Aurora because we have unprefixed CSS gradients there and we don't want to be stuck with super-bad performance in some cases.

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ee21073be123
https://hg.mozilla.org/mozilla-central/rev/874a4984caf2
https://hg.mozilla.org/mozilla-central/rev/5e637d1f8ec6

Comment 18

[Alex Keybl [:akeybl]]

Comment on attachment 637302 [details] [diff] [review]
fix

[Triage Comment]
Given where we are in the cycle, approving for Aurora 16 in support of new unprefixed CSS changes. We can always back out if this causes new regressions.

Comment 19

[Binyamin]

The bug still appears on Nightly 17.0a1 (2012-07-28). Tested on Win7 64-bit.

Comment 20

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

On this page? http://estelle.github.com/mobileperf/#slide14

Comment 21

[Marco Castelluccio [:marco]]

Did the patches land on Aurora?

Comment 22

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

Not yet. I'm waiting for Aurora approval for the regression fix in bug 779399.

Comment 23

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e4a0ba8918d
https://hg.mozilla.org/releases/mozilla-aurora/rev/68bc6eeb7b14
https://hg.mozilla.org/releases/mozilla-aurora/rev/d25d95292cad

Comment 24

[Binyamin]

It still has not been fixed on Nightly 17.0a1 (2012-08-20).

Comment 25

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

On which page do you still see the bug? http://estelle.github.com/mobileperf/#slide14?

Comment 26

[Binyamin]

@Robert, http://estelle.github.com/mobileperf/#slide14 `-moz-linear-gradient` seems rendering fine, but http://laukstein.com `repeating-linear-gradient` still is unusable slow.

Comment 27

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

I think that's a completely different problem. Please file it as a separate bug.

Comment 28

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

(And when you do, please specify your platform and about:support data.)

Comment 29

[Binyamin]

@Robert, `repeating-linear-gradient` and `background-size` bug 644444 https://bugzilla.mozilla.org/show_bug.cgi?id=644444 reported on 2011/03/23, the current status: RESOLVED FIXED.
I just added comment to update status and fix the bug.
Your mentioned patch in bug 768775 unfortunately did not fix the bug 644444.

Comment 30

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

OK, thanks. I reopened bug 644444.