notify resource caches when pixelref genID goes stale

notify resource caches when pixelref genID goes stale

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/4675819b9dbb3ad71ec851776e5de26d342f29fe

[reed1, 2015-01-07 21:14:32]

reed@google.com changed reviewers:
+ bsalomon@google.com, mtklein@google.com

[reed1, 2015-01-07 21:14:32]

WIP -- want to sync up next week to discuss

[mtklein, 2015-01-07 21:52:43]

This seems like a good plan to me.

(Introducing SkMessageBus into the mix seems unnecessary when we have a nice
simple locked global cache to call directly.)

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

https://codereview.chromium.org/825263005/diff/1/src/core/SkPixelRef.cpp#newc...
src/core/SkPixelRef.cpp:240: SkBitmapCache::NotifyGenIDStale(fGenerationID);
Feels weird knowing both these calls really grab and release the same mutex down
under the covers.  No suggested change here though... can't think of anything
that doesn't really slice through the API barriers.

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

https://codereview.chromium.org/825263005/diff/1/src/core/SkResourceCache.h#n...
src/core/SkResourceCache.h:131: *      true  : Rec is valid
I think everything might read more clearly if we had true mean purge, false mean
keep.

[reed1, 2015-01-07 22:20:42]

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

https://codereview.chromium.org/825263005/diff/1/src/core/SkPixelRef.cpp#newc...
src/core/SkPixelRef.cpp:240: SkBitmapCache::NotifyGenIDStale(fGenerationID);
On 2015/01/07 21:52:43, mtklein wrote:
> Feels weird knowing both these calls really grab and release the same mutex
down
> under the covers.  No suggested change here though... can't think of anything
> that doesn't really slice through the API barriers.

Agree on both counts.

*if* we (later) standardize on a genID field, then this problem goes away

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

https://codereview.chromium.org/825263005/diff/1/src/core/SkResourceCache.h#n...
src/core/SkResourceCache.h:131: *      true  : Rec is valid
On 2015/01/07 21:52:43, mtklein wrote:
> I think everything might read more clearly if we had true mean purge, false
mean
> keep.

I agree 0.499999, but Find uses the same VisitorProc and it treats false as
purge so .... (can't win)

[mtklein, 2015-01-08 14:11:12]

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

https://codereview.chromium.org/825263005/diff/1/src/core/SkResourceCache.h#n...
src/core/SkResourceCache.h:131: *      true  : Rec is valid
> I agree 0.499999, but Find uses the same VisitorProc and it treats false as
> purge so .... (can't win)

Do you think we'll ever want to pass the same function to both Find and
PurgeVisitor?  If it's super confusing they both take the same signature, we can
always just typedef another bool(*PurgeProc)(const Rec&, void*) with its own
note.

[reed1, 2015-02-18 17:30:33]

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

https://codereview.chromium.org/825263005/diff/1/src/core/SkPixelRef.cpp#newc...
src/core/SkPixelRef.cpp:240: SkBitmapCache::NotifyGenIDStale(fGenerationID);
On 2015/01/07 22:20:41, reed1 wrote:
> On 2015/01/07 21:52:43, mtklein wrote:
> > Feels weird knowing both these calls really grab and release the same mutex
> down
> > under the covers.  No suggested change here though... can't think of
anything
> > that doesn't really slice through the API barriers.
> 
> Agree on both counts.
> 
> *if* we (later) standardize on a genID field, then this problem goes away

Captured this comment in a comment.

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

https://codereview.chromium.org/825263005/diff/1/src/core/SkResourceCache.h#n...
src/core/SkResourceCache.h:131: *      true  : Rec is valid
On 2015/01/08 14:11:12, mtklein wrote:
> > I agree 0.499999, but Find uses the same VisitorProc and it treats false as
> > purge so .... (can't win)
> 
> Do you think we'll ever want to pass the same function to both Find and
> PurgeVisitor?  If it's super confusing they both take the same signature, we
can
> always just typedef another bool(*PurgeProc)(const Rec&, void*) with its own
> note.

Changed to new signature.

[mtklein, 2015-02-18 18:49:48]

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

https://codereview.chromium.org/825263005/diff/40001/src/core/SkResourceCache...
src/core/SkResourceCache.cpp:306: break;
Remind me, what does this break out of?  Isn't this the same as 'return' here?

Maybe we shouldn't add the AndStop versions until we use them.

[reed1, 2015-02-18 19:30:40]

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

https://codereview.chromium.org/825263005/diff/40001/src/core/SkResourceCache...
src/core/SkResourceCache.cpp:306: break;
On 2015/02/18 18:49:48, mtklein wrote:
> Remind me, what does this break out of?  Isn't this the same as 'return' here?
> 
> Maybe we shouldn't add the AndStop versions until we use them.

break inside a switch just falls out of the switch, but will continue any outer
IF or WHILE

[mtklein, 2015-02-18 19:32:48]

lgtm

[reed1, 2015-02-19 15:22:25]

New patchsets have been uploaded after l-g-t-m from mtklein@google.com

[reed1, 2015-02-19 15:23:18]

now with unittest

[reed1, 2015-02-19 15:23:24]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-02-19 15:24:06]

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

[commit-bot: I haz the power, 2015-02-19 16:22:58]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/4675819b9dbb3ad71ec851776e5de26d342f29fe

[mtklein, 2015-02-19 16:53:02]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/936423002/ by mtklein@google.com.

The reason for reverting is: Crazy failures.

http://build.chromium.org/p/client.skia/builders/Test-Mac10.8-MacMini4.1-GeFo....