Ganesh resource cache changes (reverse sense of parameter to internalDetach & attachToHead and add …

Ganesh resource cache changes (reverse sense of parameter to internalDetach & attachToHead and add …

[robertphillips, 2012-10-26 19:44:17]

This is setting up to have the uncached textures pass through the resource cache
but not impact the budget.

[bsalomon, 2012-10-26 19:54:21]

A couple Qs:

1) Why can't the makeExclusive be done automatically rather than requiring the
client to do it? It feels fragile to trust them to do it.

2) Should we use enums for exclusive and impactsBudget? The resource cache code
is pretty hard to read and replacing "true" and "false" with understandable enum
values might help some.

3) Does it make sense now or in the future to record the impactsBudget info on
the ResourceEntry rather than passing it around as a param?

[robertphillips, 2012-10-29 13:21:24]

> 1) Why can't the makeExclusive be done automatically rather than requiring the
> client to do it? It feels fragile to trust them to do it.

I did it that way (having find make the resource exclusive) the first go round
and this way seemed better. Because of how GrContext::createUncachedTexture is
structured, having find automatically call make exclusive sort of forced
createTexture to do the same thing. Having the makeExclusive call as an
orthogonal operation allowed find & createTexture do their thing and then just
have one makeExclusive on the result. 
 
> 2) Should we use enums for exclusive and impactsBudget? The resource cache
code
> is pretty hard to read and replacing "true" and "false" with understandable
enum
> values might help some.

Done - PTAL.

> 3) Does it make sense now or in the future to record the impactsBudget info on
> the ResourceEntry rather than passing it around as a param?

The full-on CL adds a flag to Texture that captures that information (since when
a texture is 'freed' we need to know if it was a scratch texture or an uncached
texture). I will try out moving that flag to GrResource and see how that plays
out in the 'controversial' CL. I hesitate to add the flag to ResourceEntry since
I would still like to remove that some day.

[robertphillips, 2012-10-29 15:35:52]

> 1) Why can't the makeExclusive be done automatically rather than requiring the
> client to do it? It feels fragile to trust them to do it.

Done. PTAL.

[bsalomon, 2012-10-29 16:17:18]

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

https://codereview.appspot.com/6784051/diff/8001/src/gpu/GrResourceCache.h#ne...
src/gpu/GrResourceCache.h:225: enum OwnershipBits {
Can we call this Flags rather than Bits? I think that's a bit more consistent
with the rest of Skia.

Also, shouldn't these be explicitly 0x1 and 0x2? I think as written
kNoOtherOwners_OwnershipBit doesn't actually do anything.

https://codereview.appspot.com/6784051/diff/8001/src/gpu/GrResourceCache.h#ne...
src/gpu/GrResourceCache.h:236: *  resource will not be returned from future
'find' calls.
Until the existing owners unref it?

https://codereview.appspot.com/6784051/diff/8001/src/gpu/GrResourceCache.h#ne...
src/gpu/GrResourceCache.h:241: int ownershipFlags = 0);
usually use uint32_t for bitfields.

[robertphillips, 2012-10-29 16:47:16]

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

https://codereview.appspot.com/6784051/diff/8001/src/gpu/GrResourceCache.h#ne...
src/gpu/GrResourceCache.h:225: enum OwnershipBits {
On 2012/10/29 16:17:18, bsalomon wrote:
> Can we call this Flags rather than Bits? I think that's a bit more consistent
> with the rest of Skia.

Done. Should I update the coding style guidelines?

> 
> Also, shouldn't these be explicitly 0x1 and 0x2? I think as written
> kNoOtherOwners_OwnershipBit doesn't actually do anything.
> 

Doh - done.

https://codereview.appspot.com/6784051/diff/8001/src/gpu/GrResourceCache.h#ne...
src/gpu/GrResourceCache.h:236: *  resource will not be returned from future
'find' calls.
On 2012/10/29 16:17:18, bsalomon wrote:
> Until the existing owners unref it?

Done.

https://codereview.appspot.com/6784051/diff/8001/src/gpu/GrResourceCache.h#ne...
src/gpu/GrResourceCache.h:241: int ownershipFlags = 0);
On 2012/10/29 16:17:18, bsalomon wrote:
> usually use uint32_t for bitfields.

Done.

[bsalomon, 2012-10-29 17:29:41]

LGTM

On 2012/10/29 16:47:16, robertphillips wrote:
> https://codereview.appspot.com/6784051/diff/8001/src/gpu/GrResourceCache.h
> File src/gpu/GrResourceCache.h (right):
> 
>
https://codereview.appspot.com/6784051/diff/8001/src/gpu/GrResourceCache.h#ne...
> src/gpu/GrResourceCache.h:225: enum OwnershipBits {
> On 2012/10/29 16:17:18, bsalomon wrote:
> > Can we call this Flags rather than Bits? I think that's a bit more
consistent
> > with the rest of Skia.
> 
> Done. Should I update the coding style guidelines?
> 

Sure, SGTM.

> > 
> > Also, shouldn't these be explicitly 0x1 and 0x2? I think as written
> > kNoOtherOwners_OwnershipBit doesn't actually do anything.
> > 
> 
> Doh - done.
> 
>
https://codereview.appspot.com/6784051/diff/8001/src/gpu/GrResourceCache.h#ne...
> src/gpu/GrResourceCache.h:236: *  resource will not be returned from future
> 'find' calls.
> On 2012/10/29 16:17:18, bsalomon wrote:
> > Until the existing owners unref it?
> 
> Done.
> 
>
https://codereview.appspot.com/6784051/diff/8001/src/gpu/GrResourceCache.h#ne...
> src/gpu/GrResourceCache.h:241: int ownershipFlags = 0);
> On 2012/10/29 16:17:18, bsalomon wrote:
> > usually use uint32_t for bitfields.
> 
> Done.

[robertphillips, 2012-10-31 12:26:46]

committed as r6211

Apparently I do not have the necessary permissions to edit the coding standard
:(