Refactored cacheID in preparation for clip mask caching changes

This CL adds more structure to the cacheID. The resource type is now explicitly represented (instead of being a bit flag) and a new domain field has been added.

[robertphillips, 2012-08-07 12:55:38]

This patch makes the resource type & domain explicitly extensible. It also
separates the GrCacheID class from GrTextureDesc and moves it to its own files.
It also makes greater use of the GrCacheID class in the GrTexture and
GrStencilBuffer classes.

It looks like the GrCacheID class may replace the GrResourceKey class.

[bsalomon, 2012-08-07 13:30:00]

This is looking like a big step in the right direction for the future of the
cache.

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.cpp
File src/gpu/GrCacheID.cpp (right):

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.cpp#newcode16
src/gpu/GrCacheID.cpp:16: if (domain < 0 || domain >= 256) {
domain could never be < 0, right?

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.cpp#newcode38
src/gpu/GrCacheID.cpp:38: v[0] = (uint32_t) (fPublicID & 0xffffffffUL);
Should we make the raw be uint64_t[2]? It seems like we're doing work to fit a
64 bit thing into two 32 bit words which might be a waste on a 64 bit machine.

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.h
File src/gpu/GrCacheID.h (right):

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.h#newcode21
src/gpu/GrCacheID.h:21: *
Overall this is a really nice comment, which is great since I anticipate
re-familiarizing myself with this several times :)

But it might be clearer to define the public / private ID distinction before the
paragraph below since you use the term "private ID" without having said what it
is.

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.h#newcode37
src/gpu/GrCacheID.h:37: class GrCacheID {
If everything on this is going to be public should we call it struct?  Or do you
expect to add private stuff later?

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrClipMaskManager.cpp...
src/gpu/GrClipMaskManager.cpp:21: const int8_t
GrClipMaskManager::kAlphaMaskDomainID = GrCacheID::GetNextDomain();
I think we'll get yelled at by chrome for this. There is a define
SK_ALLOW_GLOBAL_STATIC_INITIALIZERS (or something very close to that) that you
can use to know if static initializers are banned or not (and if so we'll have
to pay the 0 check every time)

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (left):

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrContext.cpp#oldcode182
src/gpu/GrContext.cpp:182: void gen_stencil_key_values(int width, int height,
yay!

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrTexture.cpp#newcode129
src/gpu/GrTexture.cpp:129: kFilter_TextureBit          = kNPOT_TextureBit << 1,
Might as well say the other two bits are 0x2 and 0x4 to make this a bit easier
to read.

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrTexture.cpp#newcode141
src/gpu/GrTexture.cpp:141: bool scratch,
Should we be passing scratch anymore? Isn't it redundant?

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrTexture.cpp#newcode154
src/gpu/GrTexture.cpp:154: cacheID->fDomain =
GrCacheID::kUnrestricted_ResourceDomain;
Should SkBitmap's get their own domain? I guess right now we don't have a way of
them communicating it but it seems like they should eventually.

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrTexture.cpp#newcode180
src/gpu/GrTexture.cpp:180: cacheID->fResourceSpecific16 |= kScratch_TextureBit;
Do we need this bit anymore or is it known by the public ID?

[robertphillips, 2012-08-07 15:12:35]

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.cpp
File src/gpu/GrCacheID.cpp (right):

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.cpp#newcode16
src/gpu/GrCacheID.cpp:16: if (domain < 0 || domain >= 256) {
On 2012/08/07 13:30:00, bsalomon wrote:
> domain could never be < 0, right?

Done.

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.cpp#newcode38
src/gpu/GrCacheID.cpp:38: v[0] = (uint32_t) (fPublicID & 0xffffffffUL);
If I'm correct and this class replaces GrResourceKey then this conversion to
uint32_t's will just go away. If that happens I intend to just point the hash
computation at the beginning of the GrCacheID as a uint32_t* and let it go. The
size assert above is laying the groundwork for that cast.

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.h
File src/gpu/GrCacheID.h (right):

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.h#newcode21
src/gpu/GrCacheID.h:21: *
On 2012/08/07 13:30:01, bsalomon wrote:
> Overall this is a really nice comment, which is great since I anticipate
> re-familiarizing myself with this several times :)
> 
> But it might be clearer to define the public / private ID distinction before
the
> paragraph below since you use the term "private ID" without having said what
it
> is.

Done.

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.h#newcode37
src/gpu/GrCacheID.h:37: class GrCacheID {
Done - the domain may change but the resource type remains the same.

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrClipMaskManager.cpp...
src/gpu/GrClipMaskManager.cpp:21: const int8_t
GrClipMaskManager::kAlphaMaskDomainID = GrCacheID::GetNextDomain();
Done - ouch.

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrTexture.cpp#newcode129
src/gpu/GrTexture.cpp:129: kFilter_TextureBit          = kNPOT_TextureBit << 1,
On 2012/08/07 13:30:01, bsalomon wrote:
> Might as well say the other two bits are 0x2 and 0x4 to make this a bit easier
> to read.

Done.

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrTexture.cpp#newcode141
src/gpu/GrTexture.cpp:141: bool scratch,
See later comment - I would like to leave the existing scratch tracking alone in
this CL.

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrTexture.cpp#newcode154
src/gpu/GrTexture.cpp:154: cacheID->fDomain =
GrCacheID::kUnrestricted_ResourceDomain;
I agree (that it should) but I would like to hold off until we are actually
using them. (I added the GrClipMaskManager domain just to show how it is done).

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrTexture.cpp#newcode180
src/gpu/GrTexture.cpp:180: cacheID->fResourceSpecific16 |= kScratch_TextureBit;
Ultimately it will go away. I would like to do a bit more work (and a lot more
testing) before removing it.

[bsalomon, 2012-08-07 15:26:25]

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.cpp
File src/gpu/GrCacheID.cpp (right):

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrCacheID.cpp#newcode38
src/gpu/GrCacheID.cpp:38: v[0] = (uint32_t) (fPublicID & 0xffffffffUL);
On 2012/08/07 15:12:35, robertphillips wrote:
> If I'm correct and this class replaces GrResourceKey then this conversion to
> uint32_t's will just go away. If that happens I intend to just point the hash
> computation at the beginning of the GrCacheID as a uint32_t* and let it go.
The
> size assert above is laying the groundwork for that cast.

SGTM

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrClipMaskManager.cpp...
src/gpu/GrClipMaskManager.cpp:21: const int8_t
GrClipMaskManager::kAlphaMaskDomainID = GrCacheID::GetNextDomain();
On 2012/08/07 15:12:35, robertphillips wrote:
> Done - ouch.

:(

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrTexture.cpp#newcode154
src/gpu/GrTexture.cpp:154: cacheID->fDomain =
GrCacheID::kUnrestricted_ResourceDomain;
On 2012/08/07 15:12:35, robertphillips wrote:
> I agree (that it should) but I would like to hold off until we are actually
> using them. (I added the GrClipMaskManager domain just to show how it is
done).

SGTM

http://codereview.appspot.com/6458089/diff/4002/src/gpu/GrTexture.cpp#newcode180
src/gpu/GrTexture.cpp:180: cacheID->fResourceSpecific16 |= kScratch_TextureBit;
On 2012/08/07 15:12:35, robertphillips wrote:
> Ultimately it will go away. I would like to do a bit more work (and a lot more
> testing) before removing it.

Makes sense to stage it.

http://codereview.appspot.com/6458089/diff/8001/src/gpu/GrCacheID.h
File src/gpu/GrCacheID.h (right):

http://codereview.appspot.com/6458089/diff/8001/src/gpu/GrCacheID.h#newcode14
src/gpu/GrCacheID.h:14: #define SK_DECLARE_RESOURCE_TYPE()                      
        \
I think we should use the GR_ prefix since these macros only make sense in
GR-land.

Also, maybe we should use the word cache, esp since we use the word "domain" in
at least one other context (texture domain).

GR_DECLARE_RESOURCE_CACHE_TYPE
GR_DECLARE_RESOURCE_CACHE_DOMAIN

?


I don't love saying public: and private: in macros. But it may not be easily
avoided here. Do we need the types/domains in the headers? It looks like the
current changes could work with non-member static functions in the cpp files
without declaring the type/domain ids in their headers.

http://codereview.appspot.com/6458089/diff/8001/src/gpu/GrCacheID.h#newcode47
src/gpu/GrCacheID.h:47: *      the private portion - which is used by the cache
(domain & type)
nice!

[robertphillips, 2012-08-07 17:58:49]

The private/public switch was to protect the member variable from inadvertent
changes. Moving the static var into the accessor functions is much safer.

http://codereview.appspot.com/6458089/diff/8001/src/gpu/GrCacheID.h
File src/gpu/GrCacheID.h (right):

http://codereview.appspot.com/6458089/diff/8001/src/gpu/GrCacheID.h#newcode14
src/gpu/GrCacheID.h:14: #define SK_DECLARE_RESOURCE_TYPE()                      
        \
On 2012/08/07 15:26:25, bsalomon wrote:
> I think we should use the GR_ prefix since these macros only make sense in
> GR-land.
> 
> Also, maybe we should use the word cache, esp since we use the word "domain"
in
> at least one other context (texture domain).
> 
> GR_DECLARE_RESOURCE_CACHE_TYPE
> GR_DECLARE_RESOURCE_CACHE_DOMAIN
> 
> ?
> 
> 
> I don't love saying public: and private: in macros. But it may not be easily
> avoided here. Do we need the types/domains in the headers? It looks like the
> current changes could work with non-member static functions in the cpp files
> without declaring the type/domain ids in their headers.

Done.

[robertphillips, 2012-08-07 19:46:20]

Publish moved static function's body to .cpp to prevent any problems with static
local variable.

[bsalomon, 2012-08-07 20:00:49]

LGTM.

Soon the macros will write the code for us and we can quit!

[robertphillips, 2012-08-08 10:43:09]

committed as r5002