Last Comment Bug 805831 - Add additional short lived gradient cache for border corners
: Add additional short lived gradient cache for border corners
Status: RESOLVED FIXED
[Snappy]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 8
: -- normal (vote)
: mozilla19
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
Depends on:
Blocks: 750871
  Show dependency treegraph
 
Reported: 2012-10-26 09:02 PDT by Bas Schouten (:bas.schouten)
Modified: 2012-11-10 13:48 PST (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add an additional cache for border corner gradients (21.92 KB, patch)
2012-10-26 09:02 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Add an additional cache for border corner gradients v2 (21.95 KB, patch)
2012-10-26 09:09 PDT, Bas Schouten (:bas.schouten)
joe: review+
Details | Diff | Splinter Review

Description Bas Schouten (:bas.schouten) 2012-10-26 09:02:02 PDT
Created attachment 675585 [details] [diff] [review]
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) 2012-10-26 09:09:21 PDT
Created attachment 675588 [details] [diff] [review]
Add an additional cache for border corner gradients v2

Fixed some whitespace.
Comment 2 Joe Drew (not getting mail) 2012-10-26 13:12:29 PDT
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) 2012-10-26 19:35:06 PDT
(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) 2012-10-26 21:56:37 PDT
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) 2012-10-27 07:09:51 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/ea35621ef12a

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