930451 - SVG doesn't cache its gradient stops

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Andreas Gal :gal]

Azure backends rely on this for caching. I will factor out the caching code in nsCSSRendering and then use that for SVG as well. nsCSSRenderingBorders has its own cache, thats fine I guess, because those gradients are kinda special.

Comment 1

[Andreas Gal :gal]

Attachment: Part 1: Factor out CSS gradient cache into a new class Rendering

Comment 2

[Andreas Gal :gal]

Attachment: Part 2: Use the new Rendering gradient cache for CSS border gradients as well.

Comment 3

[Andreas Gal :gal]

Attachment: Part 3: Use the Rendering gradient cache for SVG gradients as well.

Comment 4

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

Comment on attachment 821910 [details] [diff] [review]
Part 1: Factor out CSS gradient cache into a new class Rendering

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

Did you forget to "hg add" the new files?

Comment 5

[Andreas Gal :gal]

Attachment: Part 1: Factor out CSS gradient cache into a new class Rendering

Amateur mistake. How embarrassing. Sigh.

Comment 6

[Andreas Gal :gal]

Attachment: Part 2: Use the new Rendering gradient cache for CSS border gradients as well.

Comment 7

[Andreas Gal :gal]

Attachment: Part 3: Use the Rendering gradient cache for SVG gradients as well.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 821967 [details] [diff] [review]
Part 1: Factor out CSS gradient cache into a new class Rendering

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

Driving by...

What else is planned for the Rendering class?  I'm asking because it has a very general name, so I'm anticipating more things are coming.  If that's the case, it may be worth putting a comment in the class header to cover this...

::: layout/base/Rendering.h
@@ +16,5 @@
> +  static void Shutdown();
> +
> +  static Rendering &Instance();
> +
> +  gfx::GradientStops *GetGradientStops(gfx::DrawTarget *aDT,

You may get called on the white space usage of "Class *var" instead of "Class* var", but I think all of those are inherited from the cut and paste code, so that may be a way to leave it?

Comment 9

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

Comment on attachment 821967 [details] [diff] [review]
Part 1: Factor out CSS gradient cache into a new class Rendering

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

::: layout/base/Rendering.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsTArray.h"
> +#include "gfxPattern.h"

I don't think you need to #include gfxPattern.h.

@@ +9,5 @@
> +#include "mozilla/gfx/2D.h"
> +
> +namespace mozilla {
> +
> +class Rendering {

This needs a more descriptive name.

LayoutGradientCache?

@@ +14,5 @@
> +public:
> +  static void Init();
> +  static void Shutdown();
> +
> +  static Rendering &Instance();

We don't need an instance of this class. Just make the methods static.

Comment 10

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

Comment on attachment 821970 [details] [diff] [review]
Part 3: Use the Rendering gradient cache for SVG gradients as well.

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

::: gfx/thebes/gfxPattern.cpp
@@ +106,5 @@
> +                            gfxGradientCache &aCache)
> +{
> +  if (mPattern) {
> +    mStops = nullptr;
> +    nsTArray<GradientStop> stops;

nsAutoTArray<GradientStop,3> or something like that

@@ +120,5 @@
> +    mStops = aCache.GetOrCreateGradientStops(aDT,
> +                                             stops,
> +                                             (cairo_pattern_get_extend(mPattern) == CAIRO_EXTEND_REPEAT)
> +                                             ? mozilla::gfx::EXTEND_REPEAT
> +                                             : mozilla::gfx::EXTEND_CLAMP);

This file should have "using mozilla::gfx;" if it doesn't already so we don't have to use these prefixes.

Comment 11

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

Comment on attachment 821967 [details] [diff] [review]
Part 1: Factor out CSS gradient cache into a new class Rendering

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

Actually, since you're going to use this from gfx, I think it should live in gfx.

Comment 12

[Andreas Gal :gal]

Rendering was meant as an abstraction nsCSSRendering and something equivalent for SVG rendering (nsSVGRendering, we just don't have it) into -> Rendering, that is used by both. As usual, I am totally neutral on naming. I will name it anything you want.

Comment 13

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

I think we should move it to gfx/thebes and call it gfxGradientCache.

Comment 14

[Andreas Gal :gal]

I thought we are deleting thebes? Happy to do it but isn't gfx/src better?

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

There's a lot of stuff in gfx/thebes that's not gfxContext-related (e.g. all of our text stuff).  I'd put it there with a gfx prefix (like gfxGradientCache), and we can have a reshuffling at some point in the future when we're bored :)

Comment 16

[Andreas Gal :gal]

Attachment: Part 1: Factor out CSS gradient cache into a new class gfxGradientCache

Comment 17

[Andreas Gal :gal]

Attachment: Part 2: Use the new Rendering gradient cache for CSS border gradients as well (r=roc).

Comment 18

[Andreas Gal :gal]

Attachment: Part 3: Use the gradient cache for SVG gradients as well (r=roc).

Comment 19

[Andreas Gal :gal]

Attachment: Part 1: Factor out CSS gradient cache into a new class gfxGradientCache

Comment 20

[Matt Woodrow (:mattwoodrow)]

Fixed some test failures and crashes, and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e981813e29a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/74a6f39a79e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/1574b1da1773

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e981813e29a8
https://hg.mozilla.org/mozilla-central/rev/74a6f39a79e6
https://hg.mozilla.org/mozilla-central/rev/1574b1da1773

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Backports on top of bug 934860:
Aurora: https://tbpl.mozilla.org/?tree=Try&rev=72effc5856f0
Beta: https://tbpl.mozilla.org/?tree=Try&rev=c13c3ee1695c

Comment 23

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #22)
> Backports on top of bug 934860:
> Aurora: https://tbpl.mozilla.org/?tree=Try&rev=72effc5856f0
> Beta: https://tbpl.mozilla.org/?tree=Try&rev=c13c3ee1695c

Had to do another beta push due to Try not liking to build Windows off beta with the switch to rev2 build slaves.
https://tbpl.mozilla.org/?tree=Try&rev=0059fd7074b6

The good news is that we're all green :)

Comment 24

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 822668 [details] [diff] [review]
Part 1: Factor out CSS gradient cache into a new class gfxGradientCache

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None, but fixes a performance regression caused by bug 934860
User impact if declined: SVG gradient performance regression if uplift bug 934860.
Testing completed (on m-c, etc.): Been on m-c for a few days, Ryan has done try pushes for aurora/beta that came back green.
Risk to taking this patch (and alternatives if risky): Should be fairly low risk, mostly just refactoring and then caching gradients in one extra place.
String or IDL/UUID changes made by this patch: None

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/019969972ed0
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3bb77bde73f
https://hg.mozilla.org/releases/mozilla-aurora/rev/e94179cf1740

https://hg.mozilla.org/releases/mozilla-beta/rev/93a81f9ea22f
https://hg.mozilla.org/releases/mozilla-beta/rev/9ca1991e2e5c
https://hg.mozilla.org/releases/mozilla-beta/rev/0aff8f2eff7e

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/93a81f9ea22f
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/9ca1991e2e5c
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/0aff8f2eff7e