Notify resource caches when pixelref genID goes stale

Notify resource caches when pixelref genID goes stale

patch from issue 954443002 at patchset 40001 (http://crrev.com/954443002#ps40001)

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/7eeba2587760a0802fd2b90765b4fd0e5e895375

[reed2, 2015-02-24 15:55:37]

reed@chromium.org changed reviewers:
+ mtklein@google.com

[reed2, 2015-02-24 15:55:38]

using sharedID to simplify the purge. Will look at messagebus next.

[reed2, 2015-02-24 15:55:43]

The CQ bit was checked by reed@chromium.org

[commit-bot: I haz the power, 2015-02-24 15:56:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/950363002/20001

[commit-bot: I haz the power, 2015-02-24 15:56:21]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-02-24 21:56 UTC

[commit-bot: I haz the power, 2015-02-24 15:57:51]

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

[commit-bot: I haz the power, 2015-02-24 15:58:23]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...)

[reed2, 2015-02-24 16:43:36]

The CQ bit was checked by reed@chromium.org

[mtklein, 2015-02-24 16:43:40]

https://codereview.chromium.org/950363002/diff/20001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.chromium.org/950363002/diff/20001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:231: SkNotifyBitmapGenIDIsStale(fGenerationID);
Just noticed.... this should probably go inside the if (fUniqueGenerationID) {
}, which implies and is stronger than an non-zero fGenerationID.  It's safe to
invalidate the gen ID where you're doing it (it's safe to do it anywhere), but
you may end up purging resources too early unless you're guarded by
fUniqueGenerationID.

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

https://codereview.chromium.org/950363002/diff/20001/src/core/SkResourceCache...
src/core/SkResourceCache.h:37: void init(void* nameSpace, uint64_t sharedID,
size_t length);
Why don't we call this uniqueID?  I hate to add another fooID name to think
about.

Let's put the note about 0 being special up here too?

https://codereview.chromium.org/950363002/diff/20001/src/core/SkResourceCache...
src/core/SkResourceCache.h:136: static void PurgeSharedID(uint64_t sharedID);
Seems like we don't need both PurgeSharedID and PostPurgeSharedID?

https://codereview.chromium.org/950363002/diff/20001/src/core/SkResourceCache...
src/core/SkResourceCache.h:213: void purgeSharedID(uint64_t sharedID);
Probably can be private?

[commit-bot: I haz the power, 2015-02-24 16:44:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/950363002/40001

[commit-bot: I haz the power, 2015-02-24 16:44:15]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-02-24 22:44 UTC

[reed2, 2015-02-24 16:47:53]

The CQ bit was unchecked by reed@chromium.org

[reed2, 2015-02-24 17:44:31]

The CQ bit was checked by reed@chromium.org

[reed2, 2015-02-24 17:44:52]

ptal, now w/ messagebus

https://codereview.chromium.org/950363002/diff/20001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.chromium.org/950363002/diff/20001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:231: SkNotifyBitmapGenIDIsStale(fGenerationID);
On 2015/02/24 16:43:39, mtklein wrote:
> Just noticed.... this should probably go inside the if (fUniqueGenerationID) {
> }, which implies and is stronger than an non-zero fGenerationID.  It's safe to
> invalidate the gen ID where you're doing it (it's safe to do it anywhere), but
> you may end up purging resources too early unless you're guarded by
> fUniqueGenerationID.

Ah, thanks. I didn't really know what fUniqueGenerationID meant...

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

https://codereview.chromium.org/950363002/diff/20001/src/core/SkResourceCache...
src/core/SkResourceCache.h:37: void init(void* nameSpace, uint64_t sharedID,
size_t length);
On 2015/02/24 16:43:39, mtklein wrote:
> Why don't we call this uniqueID?  I hate to add another fooID name to think
> about.
> 
> Let's put the note about 0 being special up here too?

I don't think its unique, since it may appear in several entries. I thought
"shared" was an important term here.

https://codereview.chromium.org/950363002/diff/20001/src/core/SkResourceCache...
src/core/SkResourceCache.h:136: static void PurgeSharedID(uint64_t sharedID);
On 2015/02/24 16:43:39, mtklein wrote:
> Seems like we don't need both PurgeSharedID and PostPurgeSharedID?

Ah, good point. Don't need the purger, just the poster.

https://codereview.chromium.org/950363002/diff/20001/src/core/SkResourceCache...
src/core/SkResourceCache.h:213: void purgeSharedID(uint64_t sharedID);
On 2015/02/24 16:43:39, mtklein wrote:
> Probably can be private?

public like the others for testing. (which I need to add :)

[commit-bot: I haz the power, 2015-02-24 17:45:04]

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

[commit-bot: I haz the power, 2015-02-24 17:45:05]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-02-24 23:45 UTC

[reed2, 2015-02-24 17:46:35]

reed@chromium.org changed reviewers:
+ fmalita@chromium.org

[reed2, 2015-02-24 17:46:35]

florin, perhaps we can extend this (separate CL?) to also listen for pictureID
"has been deleted" messages, so we can preemptively purge pictureshader
caches...

[f(malita), 2015-02-24 20:31:37]

On 2015/02/24 17:46:35, reed2 wrote:
> florin, perhaps we can extend this (separate CL?) to also listen for pictureID
> "has been deleted" messages, so we can preemptively purge pictureshader
> caches...

I like the general idea, but not familiar enough with ResCache/PixelRefs for an
authoritative review. Deferring to mtklein for l-g-t-m.

https://codereview.chromium.org/950363002/diff/100001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.chromium.org/950363002/diff/100001/src/core/SkPixelRef.cpp...
src/core/SkPixelRef.cpp:225: SkNotifyBitmapGenIDIsStale(fGenerationID);
IIUC, we're going to bombard the resource cache with notifications for every
single genid change - whether the resource is actually cached or not. Isn't that
going to have a significant pref impact?

https://codereview.chromium.org/950363002/diff/100001/src/core/SkResourceCach...
File src/core/SkResourceCache.cpp (right):

https://codereview.chromium.org/950363002/diff/100001/src/core/SkResourceCach...
src/core/SkResourceCache.cpp:486: SkTArray<PurgeSharedIDMessage> msgs;
SkSTArray<16, PurgeSharedIDMessage, true>?

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

https://codereview.chromium.org/950363002/diff/100001/src/core/SkResourceCach...
src/core/SkResourceCache.h:128: *  do not call the VisitorProc. If a match is
found, return whatever the visitor returns.
Doc needs name update.

[mtklein, 2015-02-24 20:48:13]

The CQ bit was unchecked by mtklein@google.com

[mtklein, 2015-02-24 20:48:37]

Just one question on the test.  Otherwise this LGTM.

https://codereview.chromium.org/950363002/diff/100001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.chromium.org/950363002/diff/100001/src/core/SkPixelRef.cpp...
src/core/SkPixelRef.cpp:225: SkNotifyBitmapGenIDIsStale(fGenerationID);
On 2015/02/24 20:31:36, f(malita) wrote:
> IIUC, we're going to bombard the resource cache with notifications for every
> single genid change - whether the resource is actually cached or not. Isn't
that
> going to have a significant pref impact?

Sort of.  It's only visible changes.   It's pretty weird.  Here's an example
flow marked up:

 SkBitmap bm;
 bm.notifyPixelsChanged();
 // Does not trigger.  No one's read the gen ID, so there's no point.
 bm.getGenerationID();
 bm.notifyPixelsChanged();
 // Triggers.
 bm.notifyPixelsChanged();
 // Does not trigger.  No one's read the new gen ID.
 bm.notifyPixelsChanged();
 bm.notifyPixelsChanged();
 bm.notifyPixelsChanged();
 bm.notifyPixelsChanged();
 // Ditto.
 bm.getGenerationID();
 bm.notifyPixelsChanged();
 // Triggers.

I feel fairly confident about the perf impact being minimal, in that we're
already sending the same signal under the same conditions to Ganesh caches (via
fGenIDChangeListeners).  But, as always, we'll find out.  It might be fun to
have an excuse to make the underlying notification bus more efficient too.

https://codereview.chromium.org/950363002/diff/100001/src/core/SkResourceCach...
File src/core/SkResourceCache.cpp (right):

https://codereview.chromium.org/950363002/diff/100001/src/core/SkResourceCach...
src/core/SkResourceCache.cpp:486: SkTArray<PurgeSharedIDMessage> msgs;
On 2015/02/24 20:31:37, f(malita) wrote:
> SkSTArray<16, PurgeSharedIDMessage, true>?

At the moment, users must use SkTArray<Message>.  I wouldn't worry about it here
though... the current impl of poll is a pointer swap.

https://codereview.chromium.org/950363002/diff/100001/tests/SkResourceCacheTe...
File tests/SkResourceCacheTest.cpp (right):

https://codereview.chromium.org/950363002/diff/100001/tests/SkResourceCacheTe...
tests/SkResourceCacheTest.cpp:161: const SkMipMap* mm =
SkMipMapCache::FindAndRef(src, cache);
Doesn't this need to be single-threaded guarded just like the ones below?

[f(malita), 2015-02-24 20:54:55]

https://codereview.chromium.org/950363002/diff/100001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.chromium.org/950363002/diff/100001/src/core/SkPixelRef.cpp...
src/core/SkPixelRef.cpp:225: SkNotifyBitmapGenIDIsStale(fGenerationID);
On 2015/02/24 20:48:37, mtklein wrote:
> On 2015/02/24 20:31:36, f(malita) wrote:
> > IIUC, we're going to bombard the resource cache with notifications for every
> > single genid change - whether the resource is actually cached or not. Isn't
> that
> > going to have a significant pref impact?
> 
> Sort of.  It's only visible changes.   It's pretty weird.  Here's an example
> flow marked up:
> 
>  SkBitmap bm;
>  bm.notifyPixelsChanged();
>  // Does not trigger.  No one's read the gen ID, so there's no point.
>  bm.getGenerationID();
>  bm.notifyPixelsChanged();
>  // Triggers.
>  bm.notifyPixelsChanged();
>  // Does not trigger.  No one's read the new gen ID.
>  bm.notifyPixelsChanged();
>  bm.notifyPixelsChanged();
>  bm.notifyPixelsChanged();
>  bm.notifyPixelsChanged();
>  // Ditto.
>  bm.getGenerationID();
>  bm.notifyPixelsChanged();
>  // Triggers.
> 
> I feel fairly confident about the perf impact being minimal, in that we're
> already sending the same signal under the same conditions to Ganesh caches
(via
> fGenIDChangeListeners).  But, as always, we'll find out.  It might be fun to
> have an excuse to make the underlying notification bus more efficient too.

Ah, I did not notice the lazy impl - thanks for explaining!

[reed2, 2015-02-24 20:56:29]

https://codereview.chromium.org/950363002/diff/100001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.chromium.org/950363002/diff/100001/src/core/SkPixelRef.cpp...
src/core/SkPixelRef.cpp:225: SkNotifyBitmapGenIDIsStale(fGenerationID);
On 2015/02/24 20:31:36, f(malita) wrote:
> IIUC, we're going to bombard the resource cache with notifications for every
> single genid change - whether the resource is actually cached or not. Isn't
that
> going to have a significant pref impact?

Good thought. Perhaps we can have a flag on pixelref that announces that it has
been cached (ever)?

https://codereview.chromium.org/950363002/diff/100001/src/core/SkResourceCach...
File src/core/SkResourceCache.cpp (right):

https://codereview.chromium.org/950363002/diff/100001/src/core/SkResourceCach...
src/core/SkResourceCache.cpp:486: SkTArray<PurgeSharedIDMessage> msgs;
On 2015/02/24 20:31:37, f(malita) wrote:
> SkSTArray<16, PurgeSharedIDMessage, true>?

poll() takes SkTArray, not SkSTArray. SkTArray has prealloc'd some space already
(8 I think). Passing true seems like a good idea (is there no way for the
template to deduce that?)

https://codereview.chromium.org/950363002/diff/100001/src/core/SkResourceCach...
src/core/SkResourceCache.cpp:486: SkTArray<PurgeSharedIDMessage> msgs;
On 2015/02/24 20:48:37, mtklein wrote:
> On 2015/02/24 20:31:37, f(malita) wrote:
> > SkSTArray<16, PurgeSharedIDMessage, true>?
> 
> At the moment, users must use SkTArray<Message>.  I wouldn't worry about it
here
> though... the current impl of poll is a pointer swap.

... yea, just figured that out when I tried to compile :)

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

https://codereview.chromium.org/950363002/diff/100001/src/core/SkResourceCach...
src/core/SkResourceCache.h:128: *  do not call the VisitorProc. If a match is
found, return whatever the visitor returns.
On 2015/02/24 20:31:37, f(malita) wrote:
> Doc needs name update.

Hmmm, I think it is still accurate. If a find-visitor returns false, we purge
the rec. (don't we?)

[f(malita), 2015-02-24 20:59:39]

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

https://codereview.chromium.org/950363002/diff/100001/src/core/SkResourceCach...
src/core/SkResourceCache.h:128: *  do not call the VisitorProc. If a match is
found, return whatever the visitor returns.
On 2015/02/24 20:56:28, reed2 wrote:
> On 2015/02/24 20:31:37, f(malita) wrote:
> > Doc needs name update.
> 
> Hmmm, I think it is still accurate. If a find-visitor returns false, we purge
> the rec. (don't we?)

I meant just the param name: s/VisitorProc/FindVisitor/

[mtklein, 2015-02-24 21:24:59]

https://codereview.chromium.org/950363002/diff/100001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.chromium.org/950363002/diff/100001/src/core/SkPixelRef.cpp...
src/core/SkPixelRef.cpp:225: SkNotifyBitmapGenIDIsStale(fGenerationID);
On 2015/02/24 20:56:28, reed2 wrote:
> On 2015/02/24 20:31:36, f(malita) wrote:
> > IIUC, we're going to bombard the resource cache with notifications for every
> > single genid change - whether the resource is actually cached or not. Isn't
> that
> > going to have a significant pref impact?
> 
> Good thought. Perhaps we can have a flag on pixelref that announces that it
has
> been cached (ever)?

Oh actually, this is a really good point, probably a problem, and we should
probably fix it preemptively.  The calls we're making to Ganesh are also
throttled by the fact that Ganesh has registered a listener, while we're calling
blind.  We'll be sending messages at a higher rate than we do today.

This might be a good case for using the GenIDChangeListener interface after all.
 Is there something we can have implement SkPixelRef::GenIDChangeListener
cleanly?  Maybe the cache, or the cache entry itself?

https://codereview.chromium.org/950363002/diff/110012/src/core/SkResourceCach...
File src/core/SkResourceCache.cpp (right):

https://codereview.chromium.org/950363002/diff/110012/src/core/SkResourceCach...
src/core/SkResourceCache.cpp:486: SkTArray<PurgeSharedIDMessage, true> msgs;
You'd better check with Brian if you switch this over.  He switched this API
from SkTDArray to SkTArray because he's got non-POD messages.  I'd assume
they're relocatable, but you'd better check.

It's a little moot though.  Nothing will ever copy elements to or from msgs.  We
swap the array pointers.

[reed2, 2015-02-24 21:30:08]

Running through all gms (in DM) we basically only trigger the SkNotify call when
we're called by the destructor.

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

https://codereview.chromium.org/950363002/diff/100001/src/core/SkResourceCach...
src/core/SkResourceCache.h:128: *  do not call the VisitorProc. If a match is
found, return whatever the visitor returns.
On 2015/02/24 20:59:38, f(malita) wrote:
> On 2015/02/24 20:56:28, reed2 wrote:
> > On 2015/02/24 20:31:37, f(malita) wrote:
> > > Doc needs name update.
> > 
> > Hmmm, I think it is still accurate. If a find-visitor returns false, we
purge
> > the rec. (don't we?)
> 
> I meant just the param name: s/VisitorProc/FindVisitor/

Doh. Done.

https://codereview.chromium.org/950363002/diff/100001/tests/SkResourceCacheTe...
File tests/SkResourceCacheTest.cpp (right):

https://codereview.chromium.org/950363002/diff/100001/tests/SkResourceCacheTe...
tests/SkResourceCacheTest.cpp:161: const SkMipMap* mm =
SkMipMapCache::FindAndRef(src, cache);
On 2015/02/24 20:48:37, mtklein wrote:
> Doesn't this need to be single-threaded guarded just like the ones below?

This test explicitly has a local cache (cache is not null), so this check is
safe.

https://codereview.chromium.org/950363002/diff/110012/src/core/SkResourceCach...
File src/core/SkResourceCache.cpp (right):

https://codereview.chromium.org/950363002/diff/110012/src/core/SkResourceCach...
src/core/SkResourceCache.cpp:486: SkTArray<PurgeSharedIDMessage, true> msgs;
On 2015/02/24 21:24:59, mtklein wrote:
> You'd better check with Brian if you switch this over.  He switched this API
> from SkTDArray to SkTArray because he's got non-POD messages.  I'd assume
> they're relocatable, but you'd better check.
> 
> It's a little moot though.  Nothing will ever copy elements to or from msgs. 
We
> swap the array pointers.

Already undid this change.

[mtklein, 2015-02-24 21:31:42]

On 2015/02/24 21:30:08, reed2 wrote:
> Running through all gms (in DM) we basically only trigger the SkNotify call
when
> we're called by the destructor.

Ah good.  That makes me feel better.  Ignore my panic about change listeners. 
Let's land this thing.

[reed2, 2015-02-24 21:36:55]

addGenIDChangeListener() requires that I "new" a subclass each time I add
something to the cache. In my case, I wonder if it is reasonable to just add a
SkTRacy bool to track this...

[mtklein, 2015-02-24 21:40:13]

On 2015/02/24 21:36:55, reed2 wrote:
> addGenIDChangeListener() requires that I "new" a subclass each time I add
> something to the cache. In my case, I wonder if it is reasonable to just add a
> SkTRacy bool to track this...

Can we make something like the cache key implement it?  Something we're already
newing anyway?

Please don't much with SkTRacy for now.  Every SkTRacy is just suppressing a
real race condition.  I'm trying to fix the last ones in SkPixelRef now.

Once I've fixed them, we certainly can add another new
whatever-safe-flag-I-come-up-with to indicate that it's in a cache.  That
is-a-clone flag seems to valuable to give up for now, so I'll need to have a
mechanism to track these things somehow.

[reed2, 2015-02-24 21:47:47]

The CQ bit was checked by reed@chromium.org

[reed2, 2015-02-24 21:47:47]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/950363002/#ps130001
(title: "update dox")

[commit-bot: I haz the power, 2015-02-24 21:48:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/950363002/130001

[commit-bot: I haz the power, 2015-02-24 21:54:28]

Committed patchset #8 (id:130001) as
https://skia.googlesource.com/skia/+/7eeba2587760a0802fd2b90765b4fd0e5e895375