Add SkCachedData and use it for SkMipMap

Add SkCachedData and use it for SkMipMap

Committed: https://skia.googlesource.com/skia/+/92561a0b99ad6c08ab7a11dd1872f028199392e9

[reed1, 2014-09-22 20:48:44]

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

[reed1, 2014-09-23 13:46:31]

ptal -- though more tests to come

[reed1, 2014-09-23 20:30:42]

PTAL

[mtklein, 2014-09-23 22:16:55]

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

https://codereview.chromium.org/592843003/diff/40001/src/core/SkCachedData.cp...
src/core/SkCachedData.cpp:27: if (in_cache) {
So, if I add SkASSERT(in_cache) just before each if (in_cache) { ...},
everything seems to run fine.  This makes me think we've got more power encoded
into fRefCnt than we need.

[reed2, 2014-09-24 01:23:57]

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

[reed2, 2014-09-24 01:23:58]

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

https://codereview.chromium.org/592843003/diff/40001/src/core/SkCachedData.cp...
src/core/SkCachedData.cpp:27: if (in_cache) {
On 2014/09/23 22:16:55, mtklein wrote:
> So, if I add SkASSERT(in_cache) just before each if (in_cache) { ...},
> everything seems to run fine.  This makes me think we've got more power
encoded
> into fRefCnt than we need.

Sounds like I need to expand my tests to (try to) exercise the case when we're
not in the cache...

[reed2, 2014-09-24 01:24:19]

The CQ bit was checked by reed@chromium.org

[commit-bot: I haz the power, 2014-09-24 01:24:48]

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

[commit-bot: I haz the power, 2014-09-24 01:24:49]

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 a valid reviewer till 2014-09-24 07:24 UTC

[reed2, 2014-09-24 01:26:10]

The CQ bit was unchecked by reed@chromium.org

[mtklein, 2014-09-24 01:26:24]

On 2014/09/24 01:23:58, reed2 wrote:
> https://codereview.chromium.org/592843003/diff/40001/src/core/SkCachedData.cpp
> File src/core/SkCachedData.cpp (right):
> 
>
https://codereview.chromium.org/592843003/diff/40001/src/core/SkCachedData.cp...
> src/core/SkCachedData.cpp:27: if (in_cache) {
> On 2014/09/23 22:16:55, mtklein wrote:
> > So, if I add SkASSERT(in_cache) just before each if (in_cache) { ...},
> > everything seems to run fine.  This makes me think we've got more power
> encoded
> > into fRefCnt than we need.
> 
> Sounds like I need to expand my tests to (try to) exercise the case when we're
> not in the cache...

Why, if the non-test code doesn't?  Why wouldn't SkCachedData want to be cached?

[reed1, 2014-09-24 16:04:07]

PTAL

[reed1, 2014-09-24 16:04:10]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-24 16:04:37]

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

[commit-bot: I haz the power, 2014-09-24 16:04:38]

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 a valid reviewer till 2014-09-24 22:04 UTC

[reed1, 2014-09-24 16:06:02]

latest test exercises all branches in ref/unref.

[qiankun, 2014-09-25 07:15:11]

qiankun.miao@intel.com changed reviewers:
+ qiankun.miao@intel.com

[qiankun, 2014-09-25 07:15:12]

https://codereview.chromium.org/592843003/diff/80001/src/core/SkCachedData.cpp
File src/core/SkCachedData.cpp (right):

https://codereview.chromium.org/592843003/diff/80001/src/core/SkCachedData.cp...
src/core/SkCachedData.cpp:56: if (2 == owners) {
In some cases, owners may exceed 2 before SkCachedData is attached. Is it
correct for such case?

[reed1, 2014-09-25 18:57:46]

On 2014/09/25 07:15:12, qiankun wrote:
> https://codereview.chromium.org/592843003/diff/80001/src/core/SkCachedData.cpp
> File src/core/SkCachedData.cpp (right):
> 
>
https://codereview.chromium.org/592843003/diff/80001/src/core/SkCachedData.cp...
> src/core/SkCachedData.cpp:56: if (2 == owners) {
> In some cases, owners may exceed 2 before SkCachedData is attached. Is it
> correct for such case?

It should be fine.

[reed1, 2014-09-25 18:57:54]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-25 18:58:48]

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

[commit-bot: I haz the power, 2014-09-25 18:58:50]

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 a valid reviewer till 2014-09-26 00:58 UTC

[commit-bot: I haz the power, 2014-09-25 19:02:36]

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

[commit-bot: I haz the power, 2014-09-25 19:02:37]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)

[reed1, 2014-09-25 19:59:06]

ptal

[reed1, 2014-09-25 20:00:04]

BTW -- it is possible (likely?) that after this lands, we may want to retool how
we cache SkMask, and use subclassing instead (as this CL does for SkMipMap).
However, not sure till we really try it.

[reed1, 2014-09-26 14:40:02]

removed MaskCache work (too soon since I didn't try to use it in the mask call
sites), but added stand-alone cachedata test (and fixed a bug now that I'm
exercising discardables directly).

PTAL

[reed1, 2014-09-26 14:40:08]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-26 14:40:36]

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

[commit-bot: I haz the power, 2014-09-26 14:40:37]

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 a valid reviewer till 2014-09-26 20:40 UTC

[commit-bot: I haz the power, 2014-09-26 14:44:29]

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

[commit-bot: I haz the power, 2014-09-26 14:44:30]

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...)

[reed1, 2014-09-26 17:39:19]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-26 17:39:28]

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

[commit-bot: I haz the power, 2014-09-26 17:39:29]

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 a valid reviewer till 2014-09-26 23:39 UTC

[commit-bot: I haz the power, 2014-09-26 17:46:02]

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

[commit-bot: I haz the power, 2014-09-26 17:46:08]

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

[reed1, 2014-09-26 17:49:07]

The CQ bit was checked by reed@google.com

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

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

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

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 a valid reviewer till 2014-09-26 23:49 UTC

[commit-bot: I haz the power, 2014-09-26 17:54:14]

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

[commit-bot: I haz the power, 2014-09-26 17:54:15]

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...)

[reed1, 2014-09-26 17:59:08]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-26 17:59:36]

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

[commit-bot: I haz the power, 2014-09-26 17:59:39]

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 a valid reviewer till 2014-09-26 23:59 UTC

[mtklein, 2014-09-26 19:06:33]

https://codereview.chromium.org/592843003/diff/200002/src/core/SkBitmapCache.h
File src/core/SkBitmapCache.h (right):

https://codereview.chromium.org/592843003/diff/200002/src/core/SkBitmapCache....
src/core/SkBitmapCache.h:55: static SkMipMap* FindAndRef(const SkBitmap& src,
SkResourceCache* localCache = NULL);
const both these guys?

https://codereview.chromium.org/592843003/diff/200002/src/core/SkCachedData.h
File src/core/SkCachedData.h (right):

https://codereview.chromium.org/592843003/diff/200002/src/core/SkCachedData.h...
src/core/SkCachedData.h:50: int32_t                 fRefCnt;    // low-bit means
we're owned by the cache
Now that you've got a mutex, why don't we go back to something basic, e.g.
  bool fInCache;
  int  fActiveClientCount;
?

[reed1, 2014-09-26 21:12:21]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-09-26 21:13:20]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-26 21:13:37]

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

[commit-bot: I haz the power, 2014-09-26 21:13:40]

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 a valid reviewer till 2014-09-27 03:13 UTC

[reed1, 2014-09-26 21:33:51]

https://codereview.chromium.org/592843003/diff/200002/src/core/SkBitmapCache.h
File src/core/SkBitmapCache.h (right):

https://codereview.chromium.org/592843003/diff/200002/src/core/SkBitmapCache....
src/core/SkBitmapCache.h:55: static SkMipMap* FindAndRef(const SkBitmap& src,
SkResourceCache* localCache = NULL);
On 2014/09/26 19:06:33, mtklein wrote:
> const both these guys?

Done.

https://codereview.chromium.org/592843003/diff/200002/src/core/SkCachedData.h
File src/core/SkCachedData.h (right):

https://codereview.chromium.org/592843003/diff/200002/src/core/SkCachedData.h...
src/core/SkCachedData.h:50: int32_t                 fRefCnt;    // low-bit means
we're owned by the cache
On 2014/09/26 19:06:33, mtklein wrote:
> Now that you've got a mutex, why don't we go back to something basic, e.g.
>   bool fInCache;
>   int  fActiveClientCount;
> ?

Done.

[commit-bot: I haz the power, 2014-09-27 03:13:45]

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

[commit-bot: I haz the power, 2014-09-27 03:13:47]

No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer from
https://skia.googlesource.com/skia/+/master/CQ_COMMITTERS

[mtklein, 2014-09-27 14:47:50]

A few nits and things but the meat lgtm.

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData.cpp
File src/core/SkCachedData.cpp (right):

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData.c...
src/core/SkCachedData.cpp:64: AutoMutexWritable writable(this);
I really like this pattern.

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData.c...
src/core/SkCachedData.cpp:73: writable->fInCache |= fromCache;
Just a nit, but I'd go with ||= here.  It really is logical bools, not bits.

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData.c...
src/core/SkCachedData.cpp:79: switch (writable->fRefCnt) {
These guys might read better if you decrement first. 

switch (--writable->fRefCnt) {
  case 0:  // we're going to be deleted
  case 1:  // if we're down to one owner
}
return 0 == writable->fRefCnt;

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData.h
File src/core/SkCachedData.h (right):

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData.h...
src/core/SkCachedData.h:60: SkMutex& get_mutex() { return fMutex; }
breaking your own naming rules for these next 4 methods...

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData.h...
src/core/SkCachedData.h:78: friend class AutoMutexWritable;
I _think_ you can drop this?  The aren't nested classes already friends of the
parent (or is it the other way around)?

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData_p...
File src/core/SkCachedData_priv.h (right):

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData_p...
src/core/SkCachedData_priv.h:15: static void UnrefFromCache(SkCachedData* data)
{
Is this file dead code?

[reed1, 2014-09-27 17:09:06]

grrr, I don't like this. When we're not using discardable, I still have a mutex
etc. (grumble)

[reed1, 2014-09-27 17:25:59]

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData.cpp
File src/core/SkCachedData.cpp (right):

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData.c...
src/core/SkCachedData.cpp:73: writable->fInCache |= fromCache;
On 2014/09/27 14:47:49, mtklein wrote:
> Just a nit, but I'd go with ||= here.  It really is logical bools, not bits.

||= isn't legal afaik

However, I did rewrite to use an "if"

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData.c...
src/core/SkCachedData.cpp:79: switch (writable->fRefCnt) {
On 2014/09/27 14:47:49, mtklein wrote:
> These guys might read better if you decrement first. 
> 
> switch (--writable->fRefCnt) {
>   case 0:  // we're going to be deleted
>   case 1:  // if we're down to one owner
> }
> return 0 == writable->fRefCnt;

Done.

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData.h
File src/core/SkCachedData.h (right):

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData.h...
src/core/SkCachedData.h:60: SkMutex& get_mutex() { return fMutex; }
On 2014/09/27 14:47:49, mtklein wrote:
> breaking your own naming rules for these next 4 methods...

Done.

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData.h...
src/core/SkCachedData.h:78: friend class AutoMutexWritable;
On 2014/09/27 14:47:49, mtklein wrote:
> I _think_ you can drop this?  The aren't nested classes already friends of the
> parent (or is it the other way around)?

Done.

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData_p...
File src/core/SkCachedData_priv.h (right):

https://codereview.chromium.org/592843003/diff/270001/src/core/SkCachedData_p...
src/core/SkCachedData_priv.h:15: static void UnrefFromCache(SkCachedData* data)
{
On 2014/09/27 14:47:50, mtklein wrote:
> Is this file dead code?

Done.

[reed1, 2014-10-02 20:24:38]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-10-02 20:25:28]

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

[commit-bot: I haz the power, 2014-10-02 20:47:12]

Committed patchset #17 (id:310001) as 92561a0b99ad6c08ab7a11dd1872f028199392e9