805831 - Add additional short lived gradient cache for border corners

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: Add an additional cache for border corner gradients

Some workloads are showing very large times spent generating the gradients in order to render border corners. Considering a lot of designs will have several similar borders, and a lot of borders will have multiple corners using the same gradients, or a gradient exactly reversed, we could benefit considerably from a cache when using Azure content. This patch adds an Azure-specific drawing path for common border corners and caches gradients here.

This is only guaranteed to give correct (and in some cases more correct) rendering with the Direct2D Azure and Quartz Cairo backends. For other cairo backends it could create un-antialiased border corner transitions since that at least used to be pixman's behaviour for hard stops.

On my profiles this makes up to a 30% difference in painting for certain workloads.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Add an additional cache for border corner gradients v2

Fixed some whitespace.

Comment 2

[Joe Drew (not getting mail)]

Comment on attachment 675588 [details] [diff] [review]
Add an additional cache for border corner gradients v2

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

::: gfx/2d/Types.h
@@ +121,5 @@
>  
> +  uint32_t ToABGR() const
> +  {
> +    return uint32_t(r * 255.0f) | uint32_t(g * 255.0f) << 8 |
> +      uint32_t(b * 255.0f) << 16 | uint32_t(a * 255.0f);

What am I missing, since this seems like it's going to overwrite the r bits with a.

Also, indent correctly.

::: layout/base/nsCSSRenderingBorders.cpp
@@ +53,5 @@
> +    : mColor1(aColor1.ToABGR()), mColor2(aColor2.ToABGR())
> +    , mBackendType(aBackendType)
> +  { }
> +
> +  BorderGradientCacheKey(const BorderGradientCacheKey* aOther)

Did you mean this to be BorderGradientCacheKey& aOther? As is, this isn't a copy constructor (and it seems like this class would be fine with the default copy constructor).

@@ +1273,5 @@
> +  const twoFloats gradientCoeff[4] = { { -1, +1 },
> +                                       { -1, -1 },
> +                                       { +1, -1 },
> +                                       { +1, +1 } };
> +  

whitespace (which I can't help but notice you removed from the place you copied this from :) )

@@ +1535,5 @@
> +
> +  const twoFloats centerAdjusts[4] = { { 0, +0.5 },
> +                                        { -0.5, 0 },
> +                                        { 0, -0.5 },
> +                                        { +0.5, 0 } };

indentation is wrong here

Comment 3

[Bas Schouten (:bas.schouten)]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #2)
> Comment on attachment 675588 [details] [diff] [review]
> ::: layout/base/nsCSSRenderingBorders.cpp
> @@ +53,5 @@
> > +    : mColor1(aColor1.ToABGR()), mColor2(aColor2.ToABGR())
> > +    , mBackendType(aBackendType)
> > +  { }
> > +
> > +  BorderGradientCacheKey(const BorderGradientCacheKey* aOther)
> 
> Did you mean this to be BorderGradientCacheKey& aOther? As is, this isn't a
> copy constructor (and it seems like this class would be fine with the
> default copy constructor).

Nope, this is what the templates need.

Comment 4

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/651c802473d5 - apparently it wasn't what something needed, since it doesn't build.

Comment 5

[Bas Schouten (:bas.schouten)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ea35621ef12a

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ea35621ef12a
https://hg.mozilla.org/mozilla-central/rev/08c38354ae7c