Use variable length key (rather than accumulated matrix) as save layer hoisting key

Use variable length key (rather than accumulated matrix) as save layer hoisting key

Adding the rendering canvas' CTM to the layer hoisting key (i.e., Add support for hoisting layers in pictures drawn with a matrix - https://codereview.chromium.org/748853002/) has increased the cache miss rate due to accumulated floating point error. This CL fixes part of the issue by using the chain of operation indices leading to each saveLayer as the key. The canvas' CTM must still form part of the key but should be less subject to accumulated error.

BUG=skia:2315
Committed: https://skia.googlesource.com/skia/+/01d6e5f462d1d52203ee1a6660415877e4cf2dde

[robertphillips, 2014-11-24 17:03:29]

robertphillips@google.com changed reviewers:
+ bsalomon@google.com

[bsalomon, 2014-11-25 13:52:05]

I found this in my list of incoming CLs... wasn't sure if you were ready for me
to look at it yet.

https://codereview.chromium.org/753253002/diff/40001/src/core/SkLayerInfo.h
File src/core/SkLayerInfo.h (right):

https://codereview.chromium.org/753253002/diff/40001/src/core/SkLayerInfo.h#n...
src/core/SkLayerInfo.h:55: int*    fKey;
Did you consider using SkAutoSTArray here? (It's a fixed -ized dyn-allocated
array, unlike similarly named SkSTArray).

[robertphillips, 2014-11-25 14:04:50]

It's not ready yet.

[robertphillips, 2014-11-25 16:36:47]

https://codereview.chromium.org/753253002/diff/40001/src/core/SkLayerInfo.h
File src/core/SkLayerInfo.h (right):

https://codereview.chromium.org/753253002/diff/40001/src/core/SkLayerInfo.h#n...
src/core/SkLayerInfo.h:55: int*    fKey;
SkAutoTArray would work if it had a count in Release. In this instance I always
know the size of fKey when allocating it, it doesn't change and the contents are
POD so SkAutoSTArray is somewhat overkill.

[robertphillips, 2014-12-01 16:26:28]

PTAL - finally ready for review.

[bsalomon, 2014-12-01 16:40:47]

https://codereview.chromium.org/753253002/diff/280001/src/gpu/GrLayerCache.h
File src/gpu/GrLayerCache.h (right):

https://codereview.chromium.org/753253002/diff/280001/src/gpu/GrLayerCache.h#...
src/gpu/GrLayerCache.h:79: Key(uint32_t pictureID, const SkMatrix& initialMat,
Is the not copy optimization worth the risk of getting it wrong? Would we get
most of the same benefit from a SkAutoSTMalloc? Otherwise, lgtm.

[robertphillips, 2014-12-01 16:59:08]

https://codereview.chromium.org/753253002/diff/280001/src/gpu/GrLayerCache.h
File src/gpu/GrLayerCache.h (right):

https://codereview.chromium.org/753253002/diff/280001/src/gpu/GrLayerCache.h#...
src/gpu/GrLayerCache.h:79: Key(uint32_t pictureID, const SkMatrix& initialMat,
On 2014/12/01 16:40:47, bsalomon wrote:
> Is the not copy optimization worth the risk of getting it wrong? Would we get
> most of the same benefit from a SkAutoSTMalloc? Otherwise, lgtm.

The not-copy option is used in the lookup case in which we never want to copy.
The copy option is used in the cache creation case where we always want to copy.
So the two use cases are very clearly delineated. In both cases we also know the
exact size of the key so the storage aspects of SkAutoSTMalloc are just going to
get in the way.

[robertphillips, 2014-12-01 16:59:14]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-12-01 16:59:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/753253002/280001

[commit-bot: I haz the power, 2014-12-01 17:09:30]

Committed patchset #15 (id:280001) as
https://skia.googlesource.com/skia/+/01d6e5f462d1d52203ee1a6660415877e4cf2dde