make imagecache's Key more general purpose

make imagecache's Key more general purpose

This should allow other clients with different sized keys to still use the cache.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/4f987e94319192699e7d1a86f603ac635ec0a579

[reed1, 2014-08-15 17:16:21]

This impl is not complete, as we could do more refactoring to separate out all
the code that is specific to the (private) GenWHKey, but this is my general
idea.

Other internal clients of the cache should just need an entrypoint that takes a
Key, which they can make of any size.

[f(malita), 2014-08-15 20:27:40]

On 2014/08/15 17:16:21, reed1 wrote:
> This impl is not complete, as we could do more refactoring to separate out all
> the code that is specific to the (private) GenWHKey, but this is my general
> idea.
> 
> Other internal clients of the cache should just need an entrypoint that takes
a
> Key, which they can make of any size.

Is there anything to prevent cross-domain collisions? AFAICT, different Key
subclasses can hash the same as long as they have the same size and binary
representation.

I think we need to enforce explicit domains in the base class (add a uint32_t
fDomain and make sure each subclass initializes it differently maybe?
kBitmap_Domain, kPictureShader_Domain, ...). For picture shaders that may
actually be enough (no need for a different size key IIRC), but not sure about
masks.

[qiankun, 2014-08-18 09:24:43]

On 2014/08/15 20:27:40, Florin Malita wrote:
> On 2014/08/15 17:16:21, reed1 wrote:
> > This impl is not complete, as we could do more refactoring to separate out
all
> > the code that is specific to the (private) GenWHKey, but this is my general
> > idea.
> > 
> > Other internal clients of the cache should just need an entrypoint that
takes
> a
> > Key, which they can make of any size.
> 
> Is there anything to prevent cross-domain collisions? AFAICT, different Key
> subclasses can hash the same as long as they have the same size and binary
> representation.
> 
> I think we need to enforce explicit domains in the base class (add a uint32_t
> fDomain and make sure each subclass initializes it differently maybe?
> kBitmap_Domain, kPictureShader_Domain, ...). For picture shaders that may
> actually be enough (no need for a different size key IIRC), but not sure about
> masks.

I have the same concerns. At the beginning, I use similar approach to implement
generalized Key. But It has some limits:
(1). Different types of Key may have same content fields, but they represent
different meanings. 
(2). The operator "<" and "==" may be different for different Keys. Client can
define its own rules.

[reed1, 2014-08-18 13:53:04]

As this is internal, I think any number of conventions can be used to keep keys
from different domains distinct. One way is to store a private function-ptr at
the beginning. Another could be a private enum that we keep adding to. I'll see
what the GPU guys have done for this question.

I don't understand the need for custom == or <. These are purely there to serve
the hash-search code.

[qiankun, 2014-08-18 14:39:45]

On 2014/08/18 13:53:04, reed1 wrote:
> As this is internal, I think any number of conventions can be used to keep
keys
> from different domains distinct. One way is to store a private function-ptr at
> the beginning. Another could be a private enum that we keep adding to. I'll
see
> what the GPU guys have done for this question.
> 
> I don't understand the need for custom == or <. These are purely there to
serve
> the hash-search code.

In some scenarios, we can define Keys with different content as equal. For
example, Key with only one member: int fData, we can assume Keys with (fData%2
== 0 ) as equal.

[reed1, 2014-08-18 14:41:54]

On 2014/08/18 14:39:45, qiankun wrote:
> On 2014/08/18 13:53:04, reed1 wrote:
> > As this is internal, I think any number of conventions can be used to keep
> keys
> > from different domains distinct. One way is to store a private function-ptr
at
> > the beginning. Another could be a private enum that we keep adding to. I'll
> see
> > what the <span id="abbrex-spnAbbr_9769" class="abbrex-Abbr abbrex-Abbr_GPU
abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">GPU</span> guys have done for this
question.
> > 
> > I don't understand the need for custom == or <. These are purely there to
> serve
> > the hash-search code.
> 
> In some scenarios, we can define Keys with different content as equal. For
> example, Key with only one member: int fData, we can assume Keys with (fData%2
> == 0 ) as equal.

Is that case worth making something that is inlined today into a virtual call?
Seems like the client can munge the key data to be unique (e.g. store
(intptr_t)fData >> 3)

[qiankun, 2014-08-18 15:04:11]

On 2014/08/18 14:41:54, reed1 wrote:
> On 2014/08/18 14:39:45, qiankun wrote:
> > On 2014/08/18 13:53:04, reed1 wrote:
> > > As this is internal, I think any number of conventions can be used to keep
> > keys
> > > from different domains distinct. One way is to store a private
function-ptr
> at
> > > the beginning. Another could be a private enum that we keep adding to.
I'll
> > see
> > > what the <span id="abbrex-spnAbbr_9769" class="abbrex-Abbr abbrex-Abbr_GPU
> abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">GPU</span> guys have done for
this
> question.
> > > 
> > > I don't understand the need for custom == or <. These are purely there to
> > serve
> > > the hash-search code.
> > 
> > In some scenarios, we can define Keys with different content as equal. For
> > example, Key with only one member: int fData, we can assume Keys with
(fData%2
> > == 0 ) as equal.
> 
> Is that case worth making something that is inlined today into a virtual call?
> Seems like the client can munge the key data to be unique (e.g. store
> (intptr_t)fData >> 3)

Agree with you. Yes, we'd better keep it as inline. The blur mask data
(sigma+rects/rrect) can be covered by your implementation.

[reed1, 2014-08-18 15:10:29]

I propose to land this guy, and then in subsequent iterations...

- add some shared field to distinguish different "key domains"
- consider subclassing Rec by the client, which would allow them to embedded the
"key" directly, eliminating the need to call malloc to copy/free it.
- with the above, see if we can more cleanly support different "value" types
(e.g. not require a union for SkBitmap and SkMipMap* as we do today).

[reed1, 2014-08-20 20:32:10]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-08-20 20:33:05]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/472343002/20001

[commit-bot: I haz the power, 2014-08-20 20:33:06]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-08-21 02:33 UTC

[mtklein, 2014-08-20 20:41:06]

lgtm

[commit-bot: I haz the power, 2014-08-20 20:42:03]

Committed patchset #2 (20001) as 4f987e94319192699e7d1a86f603ac635ec0a579