Add size change notifications to GrResourceCache

Adds a mechanism for GrCacheable objects to notify the resource cache
when their size has changed. GrResourceCacheEntry now holds a
reference to the cache, and a cached value of the resource's most
recent size.

Also utilizes this new functionality for mipmaps, and adds a test for
changing resource sizes.

Committed: http://code.google.com/p/skia/source/detail?r=14576

[bsalomon, 2014-04-28 21:08:33]

Some minor style/naming comments. Otherwise looking good to me but I'd like Rob
to look as well.

https://codereview.chromium.org/257093002/diff/20001/include/gpu/GrCacheable.h
File include/gpu/GrCacheable.h (right):

https://codereview.chromium.org/257093002/diff/20001/include/gpu/GrCacheable....
include/gpu/GrCacheable.h:49: void onResourceSizeChanged() const;
We usually use the onSuchAndSuch() naming style for this pattern:

public:
   /* non-virtual */ void suchAndSuch() {
        // some common code here to assert, canonicalize inputs, early out, etc
here
        this->onSuchAndSuch();
   }

private:
   virtual void onSuchAndSuch() { // do the real subclass-specific meaty work
here }



Maybe we should call this just resourceSizeDidChange() or
sendResourceSizeChangeNotification() or something like that.

https://codereview.chromium.org/257093002/diff/20001/src/gpu/GrResourceCache.cpp
File src/gpu/GrResourceCache.cpp (right):

https://codereview.chromium.org/257093002/diff/20001/src/gpu/GrResourceCache....
src/gpu/GrResourceCache.cpp:20: fCacheEntry->onResourceSizeChanged();
{ }

[Chris Dalton, 2014-04-28 22:30:57]

Set 3 should address all the comments thus far

https://codereview.chromium.org/257093002/diff/20001/include/gpu/GrCacheable.h
File include/gpu/GrCacheable.h (right):

https://codereview.chromium.org/257093002/diff/20001/include/gpu/GrCacheable....
include/gpu/GrCacheable.h:49: void onResourceSizeChanged() const;
On 2014/04/28 21:08:33, bsalomon wrote:
> We usually use the onSuchAndSuch() naming style for this pattern:
> 
> public:
>    /* non-virtual */ void suchAndSuch() {
>         // some common code here to assert, canonicalize inputs, early out,
etc
> here
>         this->onSuchAndSuch();
>    }
> 
> private:
>    virtual void onSuchAndSuch() { // do the real subclass-specific meaty work
> here }
> 
> 
> 
> Maybe we should call this just resourceSizeDidChange() or
> sendResourceSizeChangeNotification() or something like that.

Done.

https://codereview.chromium.org/257093002/diff/20001/src/gpu/GrResourceCache.cpp
File src/gpu/GrResourceCache.cpp (right):

https://codereview.chromium.org/257093002/diff/20001/src/gpu/GrResourceCache....
src/gpu/GrResourceCache.cpp:20: fCacheEntry->onResourceSizeChanged();
On 2014/04/28 21:08:33, bsalomon wrote:
> { }

Done.

[robertphillips, 2014-04-29 13:46:25]

https://codereview.chromium.org/257093002/diff/40001/include/gpu/GrCacheable.h
File include/gpu/GrCacheable.h (right):

https://codereview.chromium.org/257093002/diff/40001/include/gpu/GrCacheable....
include/gpu/GrCacheable.h:48: 
// This entry point should be called whenever gpuMemorySize will report a
different size. This information will ultimately be communicated to the resource
cache.
?

https://codereview.chromium.org/257093002/diff/40001/include/gpu/GrTexture.h
File include/gpu/GrTexture.h (right):

https://codereview.chromium.org/257093002/diff/40001/include/gpu/GrTexture.h#...
include/gpu/GrTexture.h:48: 
I don't think we need to change this in this CL but this does now conflate dirty
with unallocated.

https://codereview.chromium.org/257093002/diff/40001/include/gpu/GrTexture.h#...
include/gpu/GrTexture.h:157: kNotAllocated_MipMapsStatus,
// allocated but dirty

https://codereview.chromium.org/257093002/diff/40001/src/gpu/GrResourceCache.cpp
File src/gpu/GrResourceCache.cpp (right):

https://codereview.chromium.org/257093002/diff/40001/src/gpu/GrResourceCache....
src/gpu/GrResourceCache.cpp:15: 
Thoughts on having GrCacheable.cpp and/or GrResourceCacheEntry.cpp?

https://codereview.chromium.org/257093002/diff/40001/src/gpu/GrResourceCache.h
File src/gpu/GrResourceCache.h (right):

https://codereview.chromium.org/257093002/diff/40001/src/gpu/GrResourceCache....
src/gpu/GrResourceCache.h:131: 
// Inform the resource cache that this entry's size has changed. Usually invoked
from GrCacheable::didChangeGpuMemorySize, not directly from here.
?

https://codereview.chromium.org/257093002/diff/40001/src/gpu/GrResourceCache....
src/gpu/GrResourceCache.h:284: */
amountInc?

https://codereview.chromium.org/257093002/diff/40001/src/gpu/GrResourceCache....
src/gpu/GrResourceCache.h:286: void didDecreaseResourceSize(const
GrResourceCacheEntry*, size_t amount);
amountDec?

https://codereview.chromium.org/257093002/diff/40001/src/gpu/GrTexture.cpp
File src/gpu/GrTexture.cpp (right):

https://codereview.chromium.org/257093002/diff/40001/src/gpu/GrTexture.cpp#ne...
src/gpu/GrTexture.cpp:61: 
no SK_OVERRIDE here

https://codereview.chromium.org/257093002/diff/40001/tests/ResourceCacheTest.cpp
File tests/ResourceCacheTest.cpp (right):

https://codereview.chromium.org/257093002/diff/40001/tests/ResourceCacheTest....
tests/ResourceCacheTest.cpp:61: SK_DECLARE_INST_COUNT(TestResource);
TestResouce(size_t size = kDefaultSize) ?

https://codereview.chromium.org/257093002/diff/40001/tests/ResourceCacheTest....
tests/ResourceCacheTest.cpp:185: GrContext* context) {
rm '\' ?

https://codereview.chromium.org/257093002/diff/40001/tests/ResourceCacheTest....
tests/ResourceCacheTest.cpp:198: 
// Test changing resources sizes when they are nonExclusive (both increase &
decrease)
?

https://codereview.chromium.org/257093002/diff/40001/tests/ResourceCacheTest....
tests/ResourceCacheTest.cpp:220: }
// Test changing resource size of exclusively held resource

https://codereview.chromium.org/257093002/diff/40001/tests/ResourceCacheTest....
tests/ResourceCacheTest.cpp:237:
static_cast<TestResource*>(cache.find(key2))->setSize(201);
What am I missing that makes key1 exclusively held?

https://codereview.chromium.org/257093002/diff/40001/tests/ResourceCacheTest....
tests/ResourceCacheTest.cpp:241: REPORTER_ASSERT(reporter, 1 ==
cache.getCachedResourceCount());
Is there a way to check the detached size here? If not, that might be very
useful to add.

[Chris Dalton, 2014-04-30 16:47:38]

I forgot, is it possoble to update the commit message with another patchset?

https://codereview.chromium.org/257093002/diff/40001/include/gpu/GrTexture.h
File include/gpu/GrTexture.h (right):

https://codereview.chromium.org/257093002/diff/40001/include/gpu/GrTexture.h#...
include/gpu/GrTexture.h:48: 
On 2014/04/29 13:46:25, robertphillips wrote:
> I don't think we need to change this in this CL but this does now conflate
dirty
> with unallocated.

Corect. I was going for minimal change -- keep the external api the same, but
internally track if 'dirty' actually means not allocate' or outdated.

Should I back this texture stuff out and let it get resolved in a future change?

https://codereview.chromium.org/257093002/diff/40001/src/gpu/GrResourceCache.cpp
File src/gpu/GrResourceCache.cpp (right):

https://codereview.chromium.org/257093002/diff/40001/src/gpu/GrResourceCache....
src/gpu/GrResourceCache.cpp:15: 
On 2014/04/29 13:46:25, robertphillips wrote:
> Thoughts on having GrCacheable.cpp and/or GrResourceCacheEntry.cpp?

I thought about it. It seemed like a lot to make a new file for such a small
function. I put it here since GrResourceCacheEntry also defines a few methods in
this file.

... But GrResourceCacheEntry and GrResourceCache also get defined in the same
header.

Another option is to just define this method right in GrCacheable.h, excepth
then we'd have to include GrResourceCache.h from there.

I'm open to whatever you guys think is best.

https://codereview.chromium.org/257093002/diff/40001/tests/ResourceCacheTest.cpp
File tests/ResourceCacheTest.cpp (right):

https://codereview.chromium.org/257093002/diff/40001/tests/ResourceCacheTest....
tests/ResourceCacheTest.cpp:237:
static_cast<TestResource*>(cache.find(key2))->setSize(201);
On 2014/04/29 13:46:25, robertphillips wrote:
> What am I missing that makes key1 exclusively held?

I should have put a comment :) This is testing that key1 gets purged when key2
is resized beyond the cache budget.

[robertphillips, 2014-04-30 17:31:49]

You can always edit the commit message separate. I would edit it first and then
post the new patch.

https://codereview.chromium.org/257093002/diff/40001/include/gpu/GrTexture.h
File include/gpu/GrTexture.h (right):

https://codereview.chromium.org/257093002/diff/40001/include/gpu/GrTexture.h#...
include/gpu/GrTexture.h:48: 
Nope - I think this is moving in the right direction. But just wanted to point
out that we will have to address this at some point.

https://codereview.chromium.org/257093002/diff/40001/src/gpu/GrResourceCache.cpp
File src/gpu/GrResourceCache.cpp (right):

https://codereview.chromium.org/257093002/diff/40001/src/gpu/GrResourceCache....
src/gpu/GrResourceCache.cpp:15: 
Let's just leave them here for now (Should probably be a separate refactoring
anyway).

https://codereview.chromium.org/257093002/diff/40001/tests/ResourceCacheTest.cpp
File tests/ResourceCacheTest.cpp (right):

https://codereview.chromium.org/257093002/diff/40001/tests/ResourceCacheTest....
tests/ResourceCacheTest.cpp:237:
static_cast<TestResource*>(cache.find(key2))->setSize(201);
Ah okay - that needs a comment! :)

[bsalomon, 2014-04-30 17:54:37]

On 2014/04/30 16:47:38, Chris Dalton wrote:
> I forgot, is it possoble to update the commit message with another patchset?
> 

You can either edit it with the Edit Issue button on the left of the web
interface or do "git cl description" from the branch associated with the issue.

[Chris Dalton, 2014-04-30 18:43:29]

I think that's everything

https://codereview.chromium.org/257093002/diff/60001/include/gpu/GrTexture.h
File include/gpu/GrTexture.h (right):

https://codereview.chromium.org/257093002/diff/60001/include/gpu/GrTexture.h#...
include/gpu/GrTexture.h:158: kAllocated_MipMapsStatus,
NotAllocated/Allocated/Valid is what I came up with. Let me know if that sounds
like it will work.

[robertphillips, 2014-04-30 19:06:53]

lgtm & a question

https://codereview.chromium.org/257093002/diff/60001/tests/ResourceCacheTest.cpp
File tests/ResourceCacheTest.cpp (right):

https://codereview.chromium.org/257093002/diff/60001/tests/ResourceCacheTest....
tests/ResourceCacheTest.cpp:205: TestResource* a = new TestResource();
Does this actually change the resource size with the new default parameter
active?

[Chris Dalton, 2014-04-30 19:08:31]

https://codereview.chromium.org/257093002/diff/60001/tests/ResourceCacheTest.cpp
File tests/ResourceCacheTest.cpp (right):

https://codereview.chromium.org/257093002/diff/60001/tests/ResourceCacheTest....
tests/ResourceCacheTest.cpp:205: TestResource* a = new TestResource();
On 2014/04/30 19:06:54, robertphillips wrote:
> Does this actually change the resource size with the new default parameter
> active?

Good point. I can upload one more patchset.

[Chris Dalton, 2014-05-05 16:54:58]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-05-05 16:55:07]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/257093002/120001

[commit-bot: I haz the power, 2014-05-05 16:55:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-05 16:55:12]

Presubmit check for 257093002-120001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com')

Was the presubmit check useful? If not, run "git cl presubmit -v"
to figure out which PRESUBMIT.py was run, then run git blame
on the file to figure out who to ask for help.

[bsalomon, 2014-05-05 17:33:53]

lgtm

[Chris Dalton, 2014-05-05 19:02:53]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-05-05 19:03:11]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/257093002/120001

[commit-bot: I haz the power, 2014-05-05 19:09:28]

Change committed as 14576