Adding API to SkGPipe for controlling memory usage externally

Adding API to SkGPipe and SkDeferredCanvas for controlling memory usage externally

BUG=http://code.google.com/p/chromium/issues/detail?id=136828
Committed: https://code.google.com/p/skia/source/detail?r=4971

[reed1, 2012-08-06 14:57:43]

Where are we heading with this API? We have a budget setter, and now a purge.
Can we write some code that tests/exercises these, so we can see if/that the
API's make sense together?

What will the caller do with the bool result. Is that important?

https://codereview.appspot.com/6454102/diff/1/include/pipe/SkGPipe.h
File include/pipe/SkGPipe.h (right):

https://codereview.appspot.com/6454102/diff/1/include/pipe/SkGPipe.h#newcode133
include/pipe/SkGPipe.h:133: *   be freed. 0 means to attempt to free as much as
possible.
Why not just pass ~0 if you want to free as much as possible? Do we use the
0-means-all convension in other places in skia?

[Leon, 2012-08-06 15:02:50]

Will this fix the bug and get DeferredCanvas and Pipe into M22?

CC'ing Derek, who is working on a change that pulls in common behavior from
Picture and Pipe for storing bitmaps.

https://codereview.appspot.com/6454102/diff/1/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.appspot.com/6454102/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
src/pipe/SkGPipeWrite.cpp:324: return 0 != bytesToFree ? ((origBytesAllocated -
fBytesAllocated) >= bytesToFree) : true;
If the caller passes 0, this will return true even if nothing was purged, but
according to the description in the header, that should return false.

[junov1, 2012-08-06 15:12:10]

On 2012/08/06 14:57:43, reed1 wrote:
> Where are we heading with this API? We have a budget setter, and now a purge.
> Can we write some code that tests/exercises these, so we can see if/that the
> API's make sense together?

I have that in another CL, where SkDeferedCanvas makes use of this API. I am
still working on a test that emulates how I intended to integrates this in
webkit.

The global strategy I am going for is to have a deferred canvas manager in
webkit that will enforce a global memory consumption limit along with a set of
rules that will favor animated canvases, and aggressively purge the resources
held by stale canvases.

> 
> What will the caller do with the bool result. Is that important?

The deferred canvas manager will used the bool result as an exit condition in an
eviction that may visit multiple canvases.


> 
> https://codereview.appspot.com/6454102/diff/1/include/pipe/SkGPipe.h
> File include/pipe/SkGPipe.h (right):
> 
>
https://codereview.appspot.com/6454102/diff/1/include/pipe/SkGPipe.h#newcode133
> include/pipe/SkGPipe.h:133: *   be freed. 0 means to attempt to free as much
as
> possible.
> Why not just pass ~0 if you want to free as much as possible? Do we use the
> 0-means-all convension in other places in skia?

sgtm, will remove 0 case.

[junov1, 2012-08-06 15:13:23]

On 2012/08/06 15:02:50, scroggo-work wrote:
> Will this fix the bug and get DeferredCanvas and Pipe into M22?

To late for that.  Aiming for M23 at this point :-(

[junov1, 2012-08-06 15:18:23]

New patch

[reed1, 2012-08-06 15:23:33]

can we add code in this CL that exercises it? tests?

https://codereview.appspot.com/6454102/diff/6002/include/pipe/SkGPipe.h
File include/pipe/SkGPipe.h (right):

https://codereview.appspot.com/6454102/diff/6002/include/pipe/SkGPipe.h#newco...
include/pipe/SkGPipe.h:134: * @return true if at least bytesToFree bytes were
freed
Why not just return the number of bytes freed?

[junov1, 2012-08-06 17:35:23]

New patch.

Did the following:
- Changed freeMemoryIfPossible to return the number of freed bytes
- Added wrappers in SkDeferredCanvas to expose freeMemoryIfPossible
- Modified behavior of SkDeferredCanvas when it reaches its memory limit: it
will first try to freeMemoryIfPossible, then flush only if necessary.
- Added unit test that verifies bitmap cache behavior in SkGPipe via the
deferred canvas interface.

[Leon, 2012-08-06 18:15:10]

https://codereview.appspot.com/6454102/diff/1004/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (right):

https://codereview.appspot.com/6454102/diff/1004/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:168: return
this->getDeferredDevice()->storageAllocatedForRecording();
Isn't this method only defined on SkDeferredDevice if
SK_DEFERRED_CANVAS_USES_GPIPE?

https://codereview.appspot.com/6454102/diff/1004/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:175: return false;
0

https://codereview.appspot.com/6454102/diff/1004/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:718: if (storageAllocatedForRecording() >
fMaxRecordingStorageBytes) {
this->storageAllocatedForRecording()

https://codereview.appspot.com/6454102/diff/1004/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:720: size_t tryFree =
storageAllocatedForRecording() - fMaxRecordingStorageBytes;
Ditto

https://codereview.appspot.com/6454102/diff/1004/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:726: this->freeMemoryIfPossible(~0);
Doesn't it cause a warning to ignore the return value? Should we add "(void)" to
the beginning of this line to avoid the warning?

https://codereview.appspot.com/6454102/diff/1004/tests/DeferredCanvasTest.cpp
File tests/DeferredCanvasTest.cpp (right):

https://codereview.appspot.com/6454102/diff/1004/tests/DeferredCanvasTest.cpp...
tests/DeferredCanvasTest.cpp:264: // Verfiy that partial purge works, image zero
is in cache but not reffed by
Verify*

[junov1, 2012-08-06 18:49:59]

https://codereview.appspot.com/6454102/diff/1004/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (right):

https://codereview.appspot.com/6454102/diff/1004/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:726: this->freeMemoryIfPossible(~0);
On 2012/08/06 18:15:10, scroggo-work wrote:
> Doesn't it cause a warning to ignore the return value? Should we add "(void)"
to
> the beginning of this line to avoid the warning?
Really? On what platform do this cause a warning?

[junov1, 2012-08-06 18:52:43]

New patch. -> Response to comment + made sure all builds and passes tests with
SkPicture backend.

[Leon, 2012-08-06 18:53:24]

On 2012/08/06 18:49:59, junov1 wrote:
>
https://codereview.appspot.com/6454102/diff/1004/src/utils/SkDeferredCanvas.cpp
> File src/utils/SkDeferredCanvas.cpp (right):
> 
>
https://codereview.appspot.com/6454102/diff/1004/src/utils/SkDeferredCanvas.c...
> src/utils/SkDeferredCanvas.cpp:726: this->freeMemoryIfPossible(~0);
> On 2012/08/06 18:15:10, scroggo-work wrote:
> > Doesn't it cause a warning to ignore the return value? Should we add
"(void)"
> to
> > the beginning of this line to avoid the warning?
> Really? On what platform do this cause a warning?

I could be wrong here. I just know we follow this convention in numerous places
in Skia, and I thought that was the reasoning. I'm sure we don't follow it
everywhere, and it is not mentioned in the docs. Feel free to ignore my comment.

[junov1, 2012-08-06 18:56:19]

On 2012/08/06 18:53:24, scroggo-work wrote:
> I could be wrong here. I just know we follow this convention in numerous
places
> in Skia, and I thought that was the reasoning. I'm sure we don't follow it
> everywhere, and it is not mentioned in the docs. Feel free to ignore my
comment.

I should do it if it helps someone, but I don't want to uglify for nothing.
Maybe it's an android thing?  If it is useful, maybe it should be in the docs.

[Leon, 2012-08-06 19:04:43]

https://codereview.appspot.com/6454102/diff/13001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.appspot.com/6454102/diff/13001/src/pipe/SkGPipeWrite.cpp#n...
src/pipe/SkGPipeWrite.cpp:1366: return false;
0

https://codereview.appspot.com/6454102/diff/13001/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (right):

https://codereview.appspot.com/6454102/diff/13001/src/utils/SkDeferredCanvas....
src/utils/SkDeferredCanvas.cpp:716: SkASSERT(0); // Not implemented for
SkPicture backend
I think this is a moot point because a) you're the only client at the moment and
b) the picture path is unused, but it seems wrong to me that calling a public
method always asserts with a particular compile flag.

[junov1, 2012-08-06 19:11:45]

New patch.

https://codereview.appspot.com/6454102/diff/13001/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (right):

https://codereview.appspot.com/6454102/diff/13001/src/utils/SkDeferredCanvas....
src/utils/SkDeferredCanvas.cpp:716: SkASSERT(0); // Not implemented for
SkPicture backend
On 2012/08/06 19:04:44, scroggo-work wrote:
> I think this is a moot point because a) you're the only client at the moment
and
> b) the picture path is unused, but it seems wrong to me that calling a public
> method always asserts with a particular compile flag.
Yeah, I'm thinking of ripping it out real soon.  No way we're going back to
SkPicture at this point.

[Leon, 2012-08-06 19:18:01]

looks good, although I think we're still in a "no checkin" state.

[Leon, 2012-08-07 13:52:19]

On 2012/08/06 19:18:01, scroggo-work wrote:
> looks good, although I think we're still in a "no checkin" state.

lgtm