Make SkBitmapCache remove invalid bitmaps from the SkResourceCache.

Make SkBitmapCache remove invalid bitmaps from the SkResourceCache.

This adds SkResourceCache::Remove() which will remove a resource from
its cache. The resource is required to be unlocked at the time Remove()
is called.

Then SkBitmapCache::Find() makes use of this to Remove() bitmaps from
the cache whose pixels have been evicted. This allows the bitmap to be
re-added to the cache with pixels again.

After this change, background a tab (and discarding all the bitmaps'
contents) no longer disables image caching for those discarded images
once the tab is visible again.

BUG=skia:2926
NOTRY=true

Committed: https://skia.googlesource.com/skia/+/790ffe3feb90370318f42b28eb9c6af6e38cd4f9

[danakj, 2014-09-10 22:20:31]

danakj@chromium.org changed reviewers:
+ reed@android.com

[danakj, 2014-09-10 22:30:44]

I found SkResouceCacheTest.cpp, and will try to figure out how to discard a
bitmap's pixels and write a test tomorrow.

[tomhudson, 2014-09-11 00:37:30]

Hmm, what's with all the purple trybots?

[reed1, 2014-09-11 13:40:30]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-09-11 13:40:31]

good catch.

https://codereview.chromium.org/561953002/diff/1/src/core/SkResourceCache.cpp
File src/core/SkResourceCache.cpp (right):

https://codereview.chromium.org/561953002/diff/1/src/core/SkResourceCache.cpp...
src/core/SkResourceCache.cpp:289: void
SkResourceCache::remove(SkResourceCache::ID id) {
I think we can just have remove(), and move the impl into there.

https://codereview.chromium.org/561953002/diff/1/src/core/SkResourceCache.h
File src/core/SkResourceCache.h (right):

https://codereview.chromium.org/561953002/diff/1/src/core/SkResourceCache.h#n...
src/core/SkResourceCache.h:144: void remove(ID);
(Rec*)

[reed1, 2014-09-11 14:33:33]

I realize this is a good fix as is, even though I would reorg parts of it. If
you like you can land this now and I can apply my refactoring ideas to it later.
lgtm

https://codereview.chromium.org/561953002/diff/1/src/core/SkResourceCache.cpp
File src/core/SkResourceCache.cpp (right):

https://codereview.chromium.org/561953002/diff/1/src/core/SkResourceCache.cpp...
src/core/SkResourceCache.cpp:289: void
SkResourceCache::remove(SkResourceCache::ID id) {
On 2014/09/11 13:40:30, reed1 wrote:
> I think we can just have remove(), and move the impl into there.

Additionally, we probably need to actually check that the lock-count is 0,
rather than assert.

Since this can be called arbitrarily by the client, they may decide to remove
it, but its possible another client could also have a "lock" on the record at
the time.

[danakj, 2014-09-11 14:42:30]

https://codereview.chromium.org/561953002/diff/1/src/core/SkResourceCache.cpp
File src/core/SkResourceCache.cpp (right):

https://codereview.chromium.org/561953002/diff/1/src/core/SkResourceCache.cpp...
src/core/SkResourceCache.cpp:289: void
SkResourceCache::remove(SkResourceCache::ID id) {
On 2014/09/11 14:33:32, reed1 wrote:
> On 2014/09/11 13:40:30, reed1 wrote:
> > I think we can just have remove(), and move the impl into there.
> 
> Additionally, we probably need to actually check that the lock-count is 0,
> rather than assert.
> 
> Since this can be called arbitrarily by the client, they may decide to remove
> it, but its possible another client could also have a "lock" on the record at
> the time.

I considered that, but then I ended up with "marking it for delete" and then
every time purgeIfNeeded() ran it would have to look through the whole list
looking for the marked one (or keep a secondary list).

It seemed like a lot of complication with no existing use case?

https://codereview.chromium.org/561953002/diff/1/src/core/SkResourceCache.cpp...
src/core/SkResourceCache.cpp:289: void
SkResourceCache::remove(SkResourceCache::ID id) {
On 2014/09/11 13:40:30, reed1 wrote:
> I think we can just have remove(), and move the impl into there.

IIUC you're suggesting that Remove() do the cast from ID to Rec* and it call
remove() with the Rec*. Sure I will do that.

[danakj, 2014-09-11 15:00:07]

PTAL - renamed purgeRec() to remove() and moved the old contents of remove()
into Remove()

[reed1, 2014-09-11 15:11:17]

thanks, the new version is simpler / easier to read.

I still want to think about the case where lockcount isn't 0, but I can tackle
that later on. For now the assert will at least catch this if it happens before
that.

lgtm

https://codereview.chromium.org/561953002/diff/60001/src/core/SkResourceCache...
File src/core/SkResourceCache.cpp (right):

https://codereview.chromium.org/561953002/diff/60001/src/core/SkResourceCache...
src/core/SkResourceCache.cpp:326: remove(rec);
nit: this->remove(rec)

[danakj, 2014-09-11 17:01:32]

I also added a unit test, though it's behind #if 0 :(

https://codereview.chromium.org/561953002/diff/60001/src/core/SkResourceCache...
File src/core/SkResourceCache.cpp (right):

https://codereview.chromium.org/561953002/diff/60001/src/core/SkResourceCache...
src/core/SkResourceCache.cpp:326: remove(rec);
On 2014/09/11 15:11:17, reed1 wrote:
> nit: this->remove(rec)

Done.

[danakj, 2014-09-11 17:03:53]

If you're happy with this can you please commit for me? I'm not a committer.
Thanks!

[tomhudson, 2014-09-11 17:18:17]

The CQ bit was checked by tomhudson@google.com

[commit-bot: I haz the power, 2014-09-11 17:18:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561953002/100001

[danakj, 2014-09-11 17:23:32]

Hm..

[17:20:14.097038] In file included from ../../src/core/SkResourceCache.cpp:9:
[17:20:14.097193] ../../src/core/SkResourceCache.h:79:17: error: private field
'fPad' is not used [-Werror,-Wunused-private-field]
[17:20:14.097242]         int32_t fPad;
[17:20:14.097282]                 ^
[17:20:14.097319] 1 error generated.

This variable appears unused before and after my change tho?

[danakj, 2014-09-11 17:24:06]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-09-11 17:24:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561953002/120001

[danakj, 2014-09-11 17:24:37]

On 2014/09/11 17:23:32, danakj wrote:
> This variable appears unused before and after my change tho?

Anyhow I've uploaded a new version that removes the unused fPad.

[danakj, 2014-09-11 17:25:04]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-09-11 17:33:59]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-09-11 17:34:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561953002/120001

[commit-bot: I haz the power, 2014-09-11 17:49:25]

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

[commit-bot: I haz the power, 2014-09-11 17:49:26]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)

[danakj, 2014-09-11 17:49:35]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-09-11 17:49:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561953002/120001

[commit-bot: I haz the power, 2014-09-11 17:49:58]

Committed patchset #7 (id:120001) as 790ffe3feb90370318f42b28eb9c6af6e38cd4f9