Last Comment Bug 761397 - [Azure][D2D] Reduce churn on surface creation for PushGroup/PopGroup/Clipping
: [Azure][D2D] Reduce churn on surface creation for PushGroup/PopGroup/Clipping
Status: RESOLVED FIXED
[Snappy:P1]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla16
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-04 15:03 PDT by Bas Schouten (:bas.schouten)
Modified: 2012-06-21 04:04 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Cache ID2D1Layers for D2D Azure backend (4.63 KB, patch)
2012-06-12 17:34 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review-
Details | Diff | Review
Part 1: Cache ID2D1Layers for D2D Azure backend v2 (6.32 KB, patch)
2012-06-13 14:58 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Review
Part 1: Cache ID2D1Layers for D2D Azure backend v3 (7.61 KB, patch)
2012-06-13 17:51 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Review

Description Bas Schouten (:bas.schouten) 2012-06-04 15:03:13 PDT
Surface creation is expensive for GPU drawing systems. I should do some work to reduce the amount of surface creations, for clipping that can be done simply by caching ID2D1Layer objects. For the Azure-Thebes wrapper it should probably be done inside the gfxContext wrapper.
Comment 1 Bas Schouten (:bas.schouten) 2012-06-12 17:34:52 PDT
Created attachment 632473 [details] [diff] [review]
Part 1: Cache ID2D1Layers for D2D Azure backend
Comment 2 Bas Schouten (:bas.schouten) 2012-06-12 17:42:51 PDT
The attached patch seems to reduce texture creation for clipping purposes by roughly 80% in a very naive test.
Comment 3 Jeff Muizelaar [:jrmuizel] 2012-06-13 13:57:27 PDT
Comment on attachment 632473 [details] [diff] [review]
Part 1: Cache ID2D1Layers for D2D Azure backend

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

::: gfx/2d/DrawTargetD2D.cpp
@@ +157,3 @@
>    , mPrivateData(NULL)
>  {
> +  mCachedLayers.resize(5);

Use a named constant instead of 5 and a comment about why you chose 5 even if that comment is "chosen arbitrarily"

@@ +968,5 @@
> +  } else {
> +    mRT->CreateLayer(byRef(clip.mLayer));
> +  }
> +  mCurrentCachedLayer++;
> +

Feels like this should be a separate function

::: gfx/2d/DrawTargetD2D.h
@@ +218,5 @@
>      D2D1_MATRIX_3X2_F mTransform;
>      RefPtr<PathD2D> mPath;
>    };
>    std::vector<PushedClip> mPushedClips;
> +  std::vector<RefPtr<ID2D1Layer>> mCachedLayers;

It looks like you're just using a std::vector here so that you raii destruction. If so, add a comment and we can switch it to something like what I'm using in this patch:
https://bug763119.bugzilla.mozilla.org/attachment.cgi?id=631822

That will save us the heap allocation.

@@ +219,5 @@
>      RefPtr<PathD2D> mPath;
>    };
>    std::vector<PushedClip> mPushedClips;
> +  std::vector<RefPtr<ID2D1Layer>> mCachedLayers;
> +  uint32_t mCurrentCachedLayer;

Add a comment about the rationale behind caching layers.
Comment 4 Joe Drew (not getting mail) 2012-06-13 14:12:47 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> > +  std::vector<RefPtr<ID2D1Layer>> mCachedLayers;
                                    ^^
That's going to fail to build on non-C++11 compilers.
Comment 5 Bas Schouten (:bas.schouten) 2012-06-13 14:58:48 PDT
Created attachment 632896 [details] [diff] [review]
Part 1: Cache ID2D1Layers for D2D Azure backend v2
Comment 6 Bas Schouten (:bas.schouten) 2012-06-13 17:51:49 PDT
Created attachment 632979 [details] [diff] [review]
Part 1: Cache ID2D1Layers for D2D Azure backend v3

Better function and this also uses the cache for masking, providing further performance improvements.
Comment 7 Jeff Muizelaar [:jrmuizel] 2012-06-14 00:06:09 PDT
Comment on attachment 632979 [details] [diff] [review]
Part 1: Cache ID2D1Layers for D2D Azure backend v3

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

::: gfx/2d/DrawTargetD2D.cpp
@@ +939,5 @@
>    rt->PopLayer();
>  
>    FinalizeRTForOperation(aOptions.mCompositionOp, aSource, Rect(0, 0, (Float)mSize.width, (Float)mSize.height));
> +
> +  mCurrentCachedLayer--;

It feels like you should also move PopLayer(); mCurrentCacheLayer-- into a helper to make sure they always match.
Comment 8 Bas Schouten (:bas.schouten) 2012-06-20 23:31:52 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f328118d86b9
Comment 9 Ed Morley [:emorley] 2012-06-21 04:04:19 PDT
https://hg.mozilla.org/mozilla-central/rev/f328118d86b9

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