Purge cached resources on SkImage destruction.

Purge cached resources on SkImage destruction.

BUG=532981
R=reed@google.com,mtklein@google.com

Committed: https://skia.googlesource.com/skia/+/3b0d532df72db806c255cad98538fcbb4d9678a8

[f(malita), 2015-09-17 21:28:13]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-17 21:28:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1352883004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1352883004/1

[f(malita), 2015-09-17 23:01:30]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-17 23:01:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1352883004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1352883004/40001

[commit-bot: I haz the power, 2015-09-17 23:04:04]

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

[commit-bot: I haz the power, 2015-09-17 23:04:05]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[f(malita), 2015-09-17 23:10:50]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-17 23:11:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1352883004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1352883004/60001

[f(malita), 2015-09-17 23:15:12]

Updated, PTAL.

[commit-bot: I haz the power, 2015-09-18 03:37:57]

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

[commit-bot: I haz the power, 2015-09-18 03:37:58]

Dry run: This issue passed the CQ dry run.

[f(malita), 2015-09-18 13:19:06]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-18 13:19:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1352883004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1352883004/80001

[f(malita), 2015-09-18 13:20:09]

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

[commit-bot: I haz the power, 2015-09-18 13:25:02]

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

[commit-bot: I haz the power, 2015-09-18 13:25:03]

Dry run: This issue passed the CQ dry run.

[tomhudson, 2015-09-18 13:30:53]

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

[tomhudson, 2015-09-18 13:30:54]

https://codereview.chromium.org/1352883004/diff/80001/src/core/SkBitmapProvid...
File src/core/SkBitmapProvider.cpp (left):

https://codereview.chromium.org/1352883004/diff/80001/src/core/SkBitmapProvid...
src/core/SkBitmapProvider.cpp:65: // TODO
Looks like maybe this mattered but we let it fall through the cracks?

https://codereview.chromium.org/1352883004/diff/80001/src/core/SkBitmapProvid...
File src/core/SkBitmapProvider.cpp (right):

https://codereview.chromium.org/1352883004/diff/80001/src/core/SkBitmapProvid...
src/core/SkBitmapProvider.cpp:65: as_IB(fImage)->notifyAddedToCache();
We're comfortable that this upcast is safe, that there aren't any other classes
derived from SkImage?

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

https://codereview.chromium.org/1352883004/diff/80001/tests/SkResourceCacheTe...
tests/SkResourceCacheTest.cpp:231: // to mitigate this risk.
Not sure how this is safe from flake?

[f(malita), 2015-09-18 13:44:24]

https://codereview.chromium.org/1352883004/diff/80001/src/core/SkBitmapProvid...
File src/core/SkBitmapProvider.cpp (right):

https://codereview.chromium.org/1352883004/diff/80001/src/core/SkBitmapProvid...
src/core/SkBitmapProvider.cpp:65: as_IB(fImage)->notifyAddedToCache();
On 2015/09/18 13:30:54, tomhudson wrote:
> We're comfortable that this upcast is safe, that there aren't any other
classes
> derived from SkImage?

Right:  SkImage_Base is the only derived class, and the motivation (IIUC) is to
hide internal implementation details from the public facing SkImage.  Within
Skia, we use as_IB(fImage) all over the place to access the private API.

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

https://codereview.chromium.org/1352883004/diff/80001/tests/SkResourceCacheTe...
tests/SkResourceCacheTest.cpp:231: // to mitigate this risk.
On 2015/09/18 13:30:54, tomhudson wrote:
> Not sure how this is safe from flake?

Note that we don't assert image pixels are cached, but only that they're no
longer cached after image destruction.

So flakes would err on the side of passing (the danger is for some other thread
to evict our data instead of the SkImage destructor).

Not sure how to improve this given multithreaded DM + global cache, beyond "the
risk is low, let's repeat a few time to increase our chances of detecting
breakage".  Maybe add a DEF_SERIAL_TEST DM option?

[mtklein, 2015-09-18 13:56:15]

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

https://codereview.chromium.org/1352883004/diff/80001/tests/SkResourceCacheTe...
tests/SkResourceCacheTest.cpp:231: // to mitigate this risk.
On 2015/09/18 13:44:24, f(malita) wrote:
> On 2015/09/18 13:30:54, tomhudson wrote:
> > Not sure how this is safe from flake?
> 
> Note that we don't assert image pixels are cached, but only that they're no
> longer cached after image destruction.
> 
> So flakes would err on the side of passing (the danger is for some other
thread
> to evict our data instead of the SkImage destructor).
> 
> Not sure how to improve this given multithreaded DM + global cache, beyond
"the
> risk is low, let's repeat a few time to increase our chances of detecting
> breakage".  Maybe add a DEF_SERIAL_TEST DM option?
> 

DM has a way to group tasks that cannot run concurrently together into an
enclave.  The enclave itself runs in parallel with other enclaves and with
thread-safe tasks (sort of little enclaves-of-1), but the tasks inside it run
serially.  This is how we do GPU testing, and how we used to do PDF testing back
when it wasn't thread safe.

If we can identify all the tasks (unit tests, GMs, skps, etc) that might
interfere here, we can put them all in an enclave together.  But if that's a
significant chunk of everything, it's probably better to do nothing instead,
rather than serialize DM.

Another approach is to have the bots run DM twice, once with --match
NotThreadSafe, the other time with --match ~NotThreadSafe, and just name
non-thread-safe tests something with "NotThreadSafe" in it.

Another approach is to add test-only canvas APIs to plumb down a separate
resource cache to use and make this thread safe.

[mtklein, 2015-09-18 14:32:36]

https://codereview.chromium.org/1352883004/diff/80001/src/core/SkBitmapProvid...
File src/core/SkBitmapProvider.cpp (left):

https://codereview.chromium.org/1352883004/diff/80001/src/core/SkBitmapProvid...
src/core/SkBitmapProvider.cpp:65: // TODO
On 2015/09/18 13:30:54, tomhudson wrote:
> Looks like maybe this mattered but we let it fall through the cracks?

Are we not DOing the TODO now?

https://codereview.chromium.org/1352883004/diff/80001/src/core/SkImageCachera...
File src/core/SkImageCacherator.h (right):

https://codereview.chromium.org/1352883004/diff/80001/src/core/SkImageCachera...
src/core/SkImageCacherator.h:37: bool lockAsBitmap(SkBitmap*, const SkImage*
client);
I think I'm having trouble catching up on our new toys.  We have an
SkImageGenerator, which presumably generates bitmaps and/or textures, and an
SkImageCacherator that wraps an SkImageGenerator to make lockAsFoo() faster
after the first call.  Is that roughly right?

If so, how does this client SkImage fit into this?  Is it a source that the
bitmap / texture are derived from?  (Is it somehow also pointed to by the
SkImageGenerator?)

https://codereview.chromium.org/1352883004/diff/80001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/1352883004/diff/80001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:217: return props ? *props : SkSurfaceProps(0,
kUnknown_SkPixelGeometry);
We might even think about adding SkSurfaceProps::SafeDefaults().  I was just
puzzling through this the other day.

https://codereview.chromium.org/1352883004/diff/80001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:222: , fProps(copy_or_safe_defaults(props))
You ought to be able to do fAddedToCache(false) here.  It'll be a bit faster,
but probably nothing life-changing.  That'd start it off initialized rather than
start it uninitialized then atomically initialize it.  (There's rarely a need to
do anything atomic in constructors.)

https://codereview.chromium.org/1352883004/diff/80001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:229: SkNotifyBitmapGenIDIsStale(this->uniqueID());
Are bitmap genids and skimage genids in the same namespace?

[reed1, 2015-09-18 14:32:53]

I think we can drop the destructor on SkImage (since it inherits from SkRefCnt),
but I'm also ok to leave it in.

LGTM w/ a suggestion for commenting the new tests.

https://codereview.chromium.org/1352883004/diff/80001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/1352883004/diff/80001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:37: SkImage::~SkImage() { }
nit: I think we can put this impl ( {} ) in the header.

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

https://codereview.chromium.org/1352883004/diff/80001/tests/SkResourceCacheTe...
tests/SkResourceCacheTest.cpp:225: static void
test_discarded_image(skiatest::Reporter* reporter, const SkMatrix& transform,
How about a block-comment here or on the main test function, describing the
architecture (briefly) and why we want to exercise different parts. e.g.
(generated images have caches, HQ-scaled images have caches, etc.)

[f(malita), 2015-09-18 15:00:14]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-18 15:00:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1352883004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1352883004/100001

[f(malita), 2015-09-18 15:00:40]

https://codereview.chromium.org/1352883004/diff/80001/src/core/SkImageCachera...
File src/core/SkImageCacherator.h (right):

https://codereview.chromium.org/1352883004/diff/80001/src/core/SkImageCachera...
src/core/SkImageCacherator.h:37: bool lockAsBitmap(SkBitmap*, const SkImage*
client);
On 2015/09/18 14:32:36, mtklein wrote:
> I think I'm having trouble catching up on our new toys.  We have an
> SkImageGenerator, which presumably generates bitmaps and/or textures, and an
> SkImageCacherator that wraps an SkImageGenerator to make lockAsFoo() faster
> after the first call.  Is that roughly right?

Sounds about right to me.

> 
> If so, how does this client SkImage fit into this?  Is it a source that the
> bitmap / texture are derived from?  (Is it somehow also pointed to by the
> SkImageGenerator?)

We have a SkImage_Generator specialization, which is backed by a
cacherator+generator.  The genID is shared by the whole gang.

https://codereview.chromium.org/1352883004/diff/80001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/1352883004/diff/80001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:37: SkImage::~SkImage() { }
On 2015/09/18 14:32:53, reed1 wrote:
> nit: I think we can put this impl ( {} ) in the header.

Dropped the SkImage dtor completely.

https://codereview.chromium.org/1352883004/diff/80001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:222: , fProps(copy_or_safe_defaults(props))
On 2015/09/18 14:32:36, mtklein wrote:
> You ought to be able to do fAddedToCache(false) here.  It'll be a bit faster,
> but probably nothing life-changing.  That'd start it off initialized rather
than
> start it uninitialized then atomically initialize it.  (There's rarely a need
to
> do anything atomic in constructors.)

Thanks, done.

https://codereview.chromium.org/1352883004/diff/80001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:229: SkNotifyBitmapGenIDIsStale(this->uniqueID());
On 2015/09/18 14:32:36, mtklein wrote:
> Are bitmap genids and skimage genids in the same namespace?

Yes.

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

https://codereview.chromium.org/1352883004/diff/80001/tests/SkResourceCacheTe...
tests/SkResourceCacheTest.cpp:225: static void
test_discarded_image(skiatest::Reporter* reporter, const SkMatrix& transform,
On 2015/09/18 14:32:53, reed1 wrote:
> How about a block-comment here or on the main test function, describing the
> architecture (briefly) and why we want to exercise different parts. e.g.
> (generated images have caches, HQ-scaled images have caches, etc.)

Done.

[mtklein, 2015-09-18 15:03:30]

gotcha, lgtm

[commit-bot: I haz the power, 2015-09-18 15:06:19]

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

[commit-bot: I haz the power, 2015-09-18 15:06:20]

Dry run: This issue passed the CQ dry run.

[f(malita), 2015-09-18 15:06:50]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2015-09-18 15:06:50]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1352883004/#ps100001
(title: "review comments")

[commit-bot: I haz the power, 2015-09-18 15:06:57]

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

[commit-bot: I haz the power, 2015-09-18 15:07:36]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/3b0d532df72db806c255cad98538fcbb4d9678a8