Simplify cache IDs and keys.

Simplify cache IDs and keys.

R=robertphillips@google.com

[bsalomon, 2012-12-19 17:03:42]

I still have to check whether this would break WK at all.

[robertphillips, 2012-12-19 18:49:58]

I'm a bit unsure about including the match criteria in the key itself.

https://codereview.appspot.com/6954047/diff/2001/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.appspot.com/6954047/diff/2001/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:466: *
It _is_

https://codereview.appspot.com/6954047/diff/2001/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:504: */
pass key by ref?

https://codereview.appspot.com/6954047/diff/2001/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:511: */
pass key by ref?

https://codereview.appspot.com/6954047/diff/2001/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:554: */
Maybe just kScratch_Key?

https://codereview.appspot.com/6954047/diff/2001/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:558: */
Maybe just kInvalid_Key?

https://codereview.appspot.com/6954047/diff/2001/include/gpu/SkGr.h
File include/gpu/SkGr.h (right):

https://codereview.appspot.com/6954047/diff/2001/include/gpu/SkGr.h#newcode77
include/gpu/SkGr.h:77: 
const GrContext?

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrBinHashKey.h
File src/gpu/GrBinHashKey.h (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrBinHashKey.h#newco...
src/gpu/GrBinHashKey.h:92: 
Should we have a keySize entry point?

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrCacheID.cpp
File src/gpu/GrCacheID.cpp (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrCacheID.cpp#newcode24
src/gpu/GrCacheID.cpp:24: 
put kAssertKey in a debug block?

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrCacheID.cpp#newcode29
src/gpu/GrCacheID.cpp:29: 
return a Domain?

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrResourceCache.h
File src/gpu/GrResourceCache.h (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrResourceCache.h#ne...
src/gpu/GrResourceCache.h:49: because we're indentifying the dimensions of the
resource not its contents. */
take a reference to the Key?

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrResourceCache.h#ne...
src/gpu/GrResourceCache.h:120: void init(const GrCacheID::Domain domain,
Key by ref?

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrStencilBuffer.cpp
File src/gpu/GrStencilBuffer.cpp (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrStencilBuffer.cpp#...
src/gpu/GrStencilBuffer.cpp:25: void gen_cache_id(int width, int height, int
sampleCnt, GrCacheID* cacheID) {
gDomain -> gStencilBufferDomain?

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrStencilBuffer.cpp#...
src/gpu/GrStencilBuffer.cpp:40: // All SBs are created internally to attach to
RTs so they all use the same domain.
gResourceType -> gStencilBufferResourceType?

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrTexture.cpp#newcod...
src/gpu/GrTexture.cpp:154: const GrCacheID& cacheID) {
gType -> gTextureResourceType?

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrTexture.cpp#newcod...
src/gpu/GrTexture.cpp:173: bool GrTexture::NeedsResizing(const GrResourceKey&
key) {
use SkToBool?

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrTexture.cpp#newcod...
src/gpu/GrTexture.cpp:177: bool GrTexture::NeedsFiltering(const GrResourceKey&
key) {
same here?

https://codereview.appspot.com/6954047/diff/2001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/SkGr.cpp#newcode72
src/gpu/SkGr.cpp:72: GR_STATIC_ASSERT(sizeof(key) >= 8 + sizeof(size_t));
gDomain -> gBitmapTextureDomain?

https://codereview.appspot.com/6954047/diff/2001/src/gpu/effects/GrTextureStr...
File src/gpu/effects/GrTextureStripAtlas.cpp (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/effects/GrTextureStr...
src/gpu/effects/GrTextureStripAtlas.cpp:198: 
gDomain -> gTextureStripAtlasDomain?

[bsalomon, 2012-12-19 19:33:21]

Uploaded a version to address smaller comments. I'm going to try to rewrite the
scratch criteria outside of cache ID and hide the scratch domain.

https://codereview.appspot.com/6954047/diff/2001/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.appspot.com/6954047/diff/2001/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:466: *
On 2012/12/19 18:49:58, robertphillips wrote:
> It _is_

Done.

https://codereview.appspot.com/6954047/diff/2001/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:504: */
On 2012/12/19 18:49:58, robertphillips wrote:
> pass key by ref?

Done.

https://codereview.appspot.com/6954047/diff/2001/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:511: */
On 2012/12/19 18:49:58, robertphillips wrote:
> pass key by ref?

Done.

https://codereview.appspot.com/6954047/diff/2001/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:554: */
On 2012/12/19 18:49:58, robertphillips wrote:
> Maybe just kScratch_Key?

leaving for now since I may refactor this.

https://codereview.appspot.com/6954047/diff/2001/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:558: */
On 2012/12/19 18:49:58, robertphillips wrote:
> Maybe just kInvalid_Key?

ditto

https://codereview.appspot.com/6954047/diff/2001/include/gpu/SkGr.h
File include/gpu/SkGr.h (right):

https://codereview.appspot.com/6954047/diff/2001/include/gpu/SkGr.h#newcode77
include/gpu/SkGr.h:77: 
On 2012/12/19 18:49:58, robertphillips wrote:
> const GrContext?

Done.

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrBinHashKey.h
File src/gpu/GrBinHashKey.h (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrBinHashKey.h#newco...
src/gpu/GrBinHashKey.h:92: 
On 2012/12/19 18:49:58, robertphillips wrote:
> Should we have a keySize entry point?

It's a template param, so can expose using a const.

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrCacheID.cpp
File src/gpu/GrCacheID.cpp (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrCacheID.cpp#newcode24
src/gpu/GrCacheID.cpp:24: 
On 2012/12/19 18:49:58, robertphillips wrote:
> put kAssertKey in a debug block?

structs can be packed differently in release and debug builds. these static
asserts are evaluated at build time in both release and debug builds.

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrCacheID.cpp#newcode29
src/gpu/GrCacheID.cpp:29: 
On 2012/12/19 18:49:58, robertphillips wrote:
> return a Domain?

Done.

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrResourceCache.h
File src/gpu/GrResourceCache.h (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrResourceCache.h#ne...
src/gpu/GrResourceCache.h:49: because we're indentifying the dimensions of the
resource not its contents. */
On 2012/12/19 18:49:58, robertphillips wrote:
> take a reference to the Key?

Done.

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrStencilBuffer.cpp
File src/gpu/GrStencilBuffer.cpp (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrStencilBuffer.cpp#...
src/gpu/GrStencilBuffer.cpp:25: void gen_cache_id(int width, int height, int
sampleCnt, GrCacheID* cacheID) {
On 2012/12/19 18:49:58, robertphillips wrote:
> gDomain -> gStencilBufferDomain?

Done.

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrStencilBuffer.cpp#...
src/gpu/GrStencilBuffer.cpp:40: // All SBs are created internally to attach to
RTs so they all use the same domain.
On 2012/12/19 18:49:58, robertphillips wrote:
> gResourceType -> gStencilBufferResourceType?

Done.

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrTexture.cpp#newcod...
src/gpu/GrTexture.cpp:154: const GrCacheID& cacheID) {
On 2012/12/19 18:49:58, robertphillips wrote:
> gType -> gTextureResourceType?

Done.

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrTexture.cpp#newcod...
src/gpu/GrTexture.cpp:173: bool GrTexture::NeedsResizing(const GrResourceKey&
key) {
On 2012/12/19 18:49:58, robertphillips wrote:
> use SkToBool?

Done.

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrTexture.cpp#newcod...
src/gpu/GrTexture.cpp:177: bool GrTexture::NeedsFiltering(const GrResourceKey&
key) {
On 2012/12/19 18:49:58, robertphillips wrote:
> same here?

Done.

https://codereview.appspot.com/6954047/diff/2001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/SkGr.cpp#newcode72
src/gpu/SkGr.cpp:72: GR_STATIC_ASSERT(sizeof(key) >= 8 + sizeof(size_t));
On 2012/12/19 18:49:58, robertphillips wrote:
> gDomain -> gBitmapTextureDomain?

Done.

https://codereview.appspot.com/6954047/diff/2001/src/gpu/effects/GrTextureStr...
File src/gpu/effects/GrTextureStripAtlas.cpp (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/effects/GrTextureStr...
src/gpu/effects/GrTextureStripAtlas.cpp:198: 
On 2012/12/19 18:49:58, robertphillips wrote:
> gDomain -> gTextureStripAtlasDomain?

Done.

[bsalomon, 2012-12-19 20:57:36]

Refactored to remove knowledge of scratch domain and match criteria from
GrCacheID.

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrResourceCache.h
File src/gpu/GrResourceCache.h (right):

https://codereview.appspot.com/6954047/diff/2001/src/gpu/GrResourceCache.h#ne...
src/gpu/GrResourceCache.h:120: void init(const GrCacheID::Domain domain,
On 2012/12/19 18:49:58, robertphillips wrote:
> Key by ref?

Done.

[robertphillips, 2012-12-20 14:02:42]

LGTM + some questions & suggestions/nits

https://codereview.appspot.com/6954047/diff/1023/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.appspot.com/6954047/diff/1023/include/gpu/GrContext.h#newc...
include/gpu/GrContext.h:120: /**
returns -> return

https://codereview.appspot.com/6954047/diff/1023/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.appspot.com/6954047/diff/1023/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:481: */
Add an kInvalid_Key and init here? We could then call
"this->reset(kInvalid_Domain, kInvalid_Key)".

https://codereview.appspot.com/6954047/diff/1023/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:507: 
Why this friend? If needed add a comment like "// needs access to XXX in order
to YYY"?

https://codereview.appspot.com/6954047/diff/1023/src/gpu/GrBinHashKey.h
File src/gpu/GrBinHashKey.h (right):

https://codereview.appspot.com/6954047/diff/1023/src/gpu/GrBinHashKey.h#newco...
src/gpu/GrBinHashKey.h:95: const uint8_t* getData() const {
GrAssert(fIsValid)?

https://codereview.appspot.com/6954047/diff/1023/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/6954047/diff/1023/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:435: if (NULL != texture) {
Is it kosher to switch from texture->desc to desc in this call?

https://codereview.appspot.com/6954047/diff/1023/src/gpu/GrResourceCache.cpp
File src/gpu/GrResourceCache.cpp (right):

https://codereview.appspot.com/6954047/diff/1023/src/gpu/GrResourceCache.cpp#...
src/gpu/GrResourceCache.cpp:15: GrResourceKey::ResourceType
GrResourceKey::GenerateResourceType() {
Ignore this if you like: Do we want to proactively reserve an invalid resource
type (e.g., ID 0)?

[bsalomon, 2012-12-20 14:21:09]

https://codereview.appspot.com/6954047/diff/1023/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.appspot.com/6954047/diff/1023/include/gpu/GrContext.h#newc...
include/gpu/GrContext.h:120: /**
On 2012/12/20 14:02:43, robertphillips wrote:
> returns -> return

Done.

https://codereview.appspot.com/6954047/diff/1023/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.appspot.com/6954047/diff/1023/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:481: */
On 2012/12/20 14:02:43, robertphillips wrote:
> Add an kInvalid_Key and init here? We could then call
> "this->reset(kInvalid_Domain, kInvalid_Key)".

The key's meaning is domain-specific. If we say some value is illegal then all
domains have to respect that restriction. It seems like it adds little value and
a burden on domains.

I added some asserts to the getters that the domain is valid.

https://codereview.appspot.com/6954047/diff/1023/include/gpu/GrTypes.h#newcod...
include/gpu/GrTypes.h:507: 
On 2012/12/20 14:02:43, robertphillips wrote:
> Why this friend? If needed add a comment like "// needs access to XXX in order
> to YYY"?

No longer necessary after refactor of match criteria, removed.

https://codereview.appspot.com/6954047/diff/1023/src/gpu/GrBinHashKey.h
File src/gpu/GrBinHashKey.h (right):

https://codereview.appspot.com/6954047/diff/1023/src/gpu/GrBinHashKey.h#newco...
src/gpu/GrBinHashKey.h:95: const uint8_t* getData() const {
On 2012/12/20 14:02:43, robertphillips wrote:
> GrAssert(fIsValid)?

Done.

https://codereview.appspot.com/6954047/diff/1023/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/6954047/diff/1023/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:435: if (NULL != texture) {
On 2012/12/20 14:02:43, robertphillips wrote:
> Is it kosher to switch from texture->desc to desc in this call?
Hmm.. should be, but will change back.

https://codereview.appspot.com/6954047/diff/1023/src/gpu/GrResourceCache.cpp
File src/gpu/GrResourceCache.cpp (right):

https://codereview.appspot.com/6954047/diff/1023/src/gpu/GrResourceCache.cpp#...
src/gpu/GrResourceCache.cpp:15: GrResourceKey::ResourceType
GrResourceKey::GenerateResourceType() {
On 2012/12/20 14:02:43, robertphillips wrote:
> Ignore this if you like: Do we want to proactively reserve an invalid resource
> type (e.g., ID 0)?

It seems like we just add such a thing when we need it... the IDs are runtime
assigned so no other code would need to change if we were to reserve a value.