Purge bitmaps from SkGPipe's shared heap.

Purge bitmaps from SkGPipe's shared heap.

BitmapInfo:
Now in SkGPipePriv so it can be accessed by SkGPipeRead.

Add the ability to essentially ref count BitmapInfos so that they can
be purged to make room in the shared heap for a new one.

SkGPipeWrite:
Purge the least recently used bitmap if it has already been drawn by
all readers.

SkGPipeRead:
Read the BitmapInfo (instead of the SkBitmap) and decrement its count
after drawing.

SkGPipeController:
Added a method to tell how many readers will be used, so that when
purging bitmaps each reader can be accounted for.

Committed: https://code.google.com/p/skia/source/detail?r=4638

[Leon, 2012-07-12 23:02:04]

https://codereview.appspot.com/6374065/diff/1/include/core/SkThread_platform.h
File include/core/SkThread_platform.h (right):

https://codereview.appspot.com/6374065/diff/1/include/core/SkThread_platform....
include/core/SkThread_platform.h:58: #define sk_atomic_inc(addr, inc)   
android_atomic_add(inc, addr)
Found this online, but I'll test to make sure it works.

https://codereview.appspot.com/6374065/diff/1/src/ports/SkThread_win.cpp
File src/ports/SkThread_win.cpp (right):

https://codereview.appspot.com/6374065/diff/1/src/ports/SkThread_win.cpp#newc...
src/ports/SkThread_win.cpp:25: return
_InterlockedAdd(reinterpret_cast<LONG*>(addr), value) - value;
I still need to try this on a Windows machine. Is _InterlockedAdd as fast as
_InterlockedIncrement? Might it be better to have a separate method for
sk_atomic_add that takes a parameter?

[reed1, 2012-07-13 14:55:50]

Not sure we should remove atomic_inc, and cast it as atomic_add(ptr, inc = 1).
Why not have both? inc seems so very common . Are we making the callers to any
extra work at all with them calling add?

Lets try to land a separate CL for the atomic changes by itself (first).

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

https://codereview.appspot.com/6374065/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
src/pipe/SkGPipeWrite.cpp:204: BitmapInfo* Heap::bitmapToReplace(const SkBitmap&
bm) const {
5?

[Leon, 2012-07-13 19:15:18]

On 2012/07/13 14:55:50, reed1 wrote:
> Not sure we should remove atomic_inc, and cast it as atomic_add(ptr, inc = 1).
> Why not have both? inc seems so very common . Are we making the callers to any
> extra work at all with them calling add?
> 
> Lets try to land a separate CL for the atomic changes by itself (first).

I have uploaded a change at https://codereview.appspot.com/6350106/
> 
> https://codereview.appspot.com/6374065/diff/1/src/pipe/SkGPipeWrite.cpp
> File src/pipe/SkGPipeWrite.cpp (right):
> 
>
https://codereview.appspot.com/6374065/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
> src/pipe/SkGPipeWrite.cpp:204: BitmapInfo* Heap::bitmapToReplace(const
SkBitmap&
> bm) const {
> 5?

[Leon, 2012-07-13 19:15:33]

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

https://codereview.appspot.com/6374065/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
src/pipe/SkGPipeWrite.cpp:204: BitmapInfo* Heap::bitmapToReplace(const SkBitmap&
bm) const {
On 2012/07/13 14:55:50, reed1 wrote:
> 5?

Completely arbitrary. We should do some testing to figure out what the best
value is.

[reed1, 2012-07-13 19:17:25]

On 2012/07/13 19:15:33, scroggo-work wrote:
> https://codereview.appspot.com/6374065/diff/1/src/pipe/SkGPipeWrite.cpp
> File src/pipe/SkGPipeWrite.cpp (right):
> 
>
https://codereview.appspot.com/6374065/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
> src/pipe/SkGPipeWrite.cpp:204: BitmapInfo* Heap::bitmapToReplace(const
SkBitmap&
> bm) const {
> On 2012/07/13 14:55:50, reed1 wrote:
> > 5?
> 
> Completely arbitrary. We should do some testing to figure out what the best
> value is.

I'm OK with 5, if you can document in the code what you just said...

[Leon, 2012-07-16 17:29:23]

Now uses sk_atomic_add, which was committed at
https://code.google.com/p/skia/source/detail?r=4623#

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

https://codereview.appspot.com/6374065/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
src/pipe/SkGPipeWrite.cpp:204: BitmapInfo* Heap::bitmapToReplace(const SkBitmap&
bm) const {
Added a comment explaining why 5 is used as the limit.

[reed1, 2012-07-16 18:19:13]

Still a little opaque to me. Just looking for more dox (either explaining or as
TODOs).

Given how hard it is for a new reader to understand flattening or pictures (both
my fault), I'm pushing to make this new infrastructure more obvious...

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

https://codereview.appspot.com/6374065/diff/12002/src/pipe/SkGPipeWrite.cpp#n...
src/pipe/SkGPipeWrite.cpp:165: 
// Is this array sorted in any way?

https://codereview.appspot.com/6374065/diff/12002/src/pipe/SkGPipeWrite.cpp#n...
src/pipe/SkGPipeWrite.cpp:172: 
// We just "used" info. Update our LRU accordingly

https://codereview.appspot.com/6374065/diff/12002/src/pipe/SkGPipeWrite.cpp#n...
src/pipe/SkGPipeWrite.cpp:203: 
/**
 * Given a new bitmap to be added to the cache, return an existing one that
should be removed to make room, or NULL if there is already room.
(or some other high-level comment document this methods use/constraints).
*/

https://codereview.appspot.com/6374065/diff/12002/src/pipe/SkGPipeWrite.cpp#n...
src/pipe/SkGPipeWrite.cpp:205: // Arbitrarily set a limit of 5. We should test
to find the optimum limit.
// What are the trade-offs? Why isn't bigger always better? Do you expect that
above some value we won't get faster? Will we run out of memory? Will we
slow-down searching too much?

[Leon, 2012-07-16 21:28:33]

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

https://codereview.appspot.com/6374065/diff/12002/src/pipe/SkGPipeWrite.cpp#n...
src/pipe/SkGPipeWrite.cpp:165: 
On 2012/07/16 18:19:13, reed1 wrote:
> // Is this array sorted in any way?

No. It's actually an artifact of my old method of doing things. Now that I use a
linked list, the only thing I use it for is to check the count, which I can do
without it. I have removed it in my latest patch

https://codereview.appspot.com/6374065/diff/12002/src/pipe/SkGPipeWrite.cpp#n...
src/pipe/SkGPipeWrite.cpp:172: 
On 2012/07/16 18:19:13, reed1 wrote:
> // We just "used" info. Update our LRU accordingly

Done.

https://codereview.appspot.com/6374065/diff/12002/src/pipe/SkGPipeWrite.cpp#n...
src/pipe/SkGPipeWrite.cpp:203: 
On 2012/07/16 18:19:13, reed1 wrote:
> /**
>  * Given a new bitmap to be added to the cache, return an existing one that
> should be removed to make room, or NULL if there is already room.
> (or some other high-level comment document this methods use/constraints).
> */

Done.

https://codereview.appspot.com/6374065/diff/12002/src/pipe/SkGPipeWrite.cpp#n...
src/pipe/SkGPipeWrite.cpp:205: // Arbitrarily set a limit of 5. We should test
to find the optimum limit.
On 2012/07/16 18:19:13, reed1 wrote:
> // What are the trade-offs? Why isn't bigger always better? Do you expect that
> above some value we won't get faster? Will we run out of memory? Will we
> slow-down searching too much?

Added a comment.

[reed1, 2012-07-17 12:20:03]

lgtm w/ override comment

https://codereview.appspot.com/6374065/diff/17001/src/pipe/utils/SamplePipeCo...
File src/pipe/utils/SamplePipeControllers.h (left):

https://codereview.appspot.com/6374065/diff/17001/src/pipe/utils/SamplePipeCo...
src/pipe/utils/SamplePipeControllers.h:36: virtual void notifyWritten(size_t
bytes) SK_OVERRIDE;
SK_OVERRIDE

[Leon, 2012-07-17 15:59:19]

https://codereview.appspot.com/6374065/diff/17001/src/pipe/utils/SamplePipeCo...
File src/pipe/utils/SamplePipeControllers.h (left):

https://codereview.appspot.com/6374065/diff/17001/src/pipe/utils/SamplePipeCo...
src/pipe/utils/SamplePipeControllers.h:36: virtual void notifyWritten(size_t
bytes) SK_OVERRIDE;
On 2012/07/17 12:20:03, reed1 wrote:
> SK_OVERRIDE

Done.