Use SkFlatDictionary in SkGPipe to take advantage of its new features.

Use SkFlatDictionary in SkGPipe to take advantage of its new features.

Add a controller class to perform the allocation/unallocation for the dictionary and to provide an entry to be replaced, if replacements are allowed.

TODO:
Use LRU caching in my custom controller so replacements will be done less often.
More refactoring on SkFlatDictionary so picture recording's use of the dictionary does not require going through the path to replace.

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

[Leon, 2012-07-12 18:02:02]

https://codereview.appspot.com/6345102/diff/1/tools/bench_pictures_main.cpp
File tools/bench_pictures_main.cpp (right):

https://codereview.appspot.com/6345102/diff/1/tools/bench_pictures_main.cpp#n...
tools/bench_pictures_main.cpp:144: printf("%i_tiles_%ix%i: cmsecs = %6.2f\n",
tiles.count(), options.fTileWidth,
This wasn't actually my change. I accidentally did a git commit --amend to the
change before mine, so git cl dcommit included it. I have not actually changed
the file.

Teach me to use git... :P

[reed1, 2012-07-13 18:09:34]

still complex, but maybe that's unavoidable. What are bench results for this
change (-match picture_record)?

https://codereview.appspot.com/6345102/diff/5001/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (left):

https://codereview.appspot.com/6345102/diff/5001/src/core/SkPictureFlat.h#old...
src/core/SkPictureFlat.h:260: SkFlatDictionary(SkChunkAlloc* heap) {
// The caller guarantees that the controller will out-live the dictionary, else
// we need to make FlatController refcounted

https://codereview.appspot.com/6345102/diff/5001/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (right):

https://codereview.appspot.com/6345102/diff/5001/src/core/SkPictureFlat.h#new...
src/core/SkPictureFlat.h:217: // Returns the size of the flattened data.
size_t or uint32_t ?

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

https://codereview.appspot.com/6345102/diff/5001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:95: int indexToRemove = fPointers.count() - 1;
would a for-loop be clearer?
can you use fPointers.rfind(...)?

[Leon, 2012-07-13 21:04:22]

Re: complexity

I'll think about it some more...

Re: timing

The picture_record tests took around .5-2 ms longer.

https://codereview.appspot.com/6345102/diff/5001/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (left):

https://codereview.appspot.com/6345102/diff/5001/src/core/SkPictureFlat.h#old...
src/core/SkPictureFlat.h:260: SkFlatDictionary(SkChunkAlloc* heap) {
On 2012/07/13 18:09:35, reed1 wrote:
> // The caller guarantees that the controller will out-live the dictionary,
else
> // we need to make FlatController refcounted

Done.

https://codereview.appspot.com/6345102/diff/5001/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (right):

https://codereview.appspot.com/6345102/diff/5001/src/core/SkPictureFlat.h#new...
src/core/SkPictureFlat.h:217: // Returns the size of the flattened data.
On 2012/07/13 18:09:35, reed1 wrote:
> size_t or uint32_t ?

Done.

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

https://codereview.appspot.com/6345102/diff/5001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:95: int indexToRemove = fPointers.count() - 1;
On 2012/07/13 18:09:35, reed1 wrote:
> would a for-loop be clearer?
> can you use fPointers.rfind(...)?
> 

Done.

[reed1, 2012-07-16 13:51:13]

Might add a small todo list in the CL, so we can see some of the motivation for
these structural changes:

e.g.
- LRU smarts in the controller
- ...

https://codereview.appspot.com/6345102/diff/12001/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (right):

https://codereview.appspot.com/6345102/diff/12001/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:164: public:
For the future, I find this comment-style harder to read, and it doesn't
automatically turn into doxygen dox. How about

/**
 * THis is my
 * block comment
 */
virtual void ...;

/**
 * Note the space between functions
 */
void int ...;

https://codereview.appspot.com/6345102/diff/12001/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:248: 
// When we purge an entry, we want to reuse an old index for the new entry, so
we expose this setter (or some other comment to this effect)

https://codereview.appspot.com/6345102/diff/12001/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:363: int indexToReplace = -1;
Can you use fData.find(toReplace) instead of this for-loop?

https://codereview.appspot.com/6345102/diff/12001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.appspot.com/6345102/diff/12001/src/pipe/SkGPipeRead.cpp#ne...
src/pipe/SkGPipeRead.cpp:110: if(fBitmaps.count() == index) {
minor:

bm = SkNEW(SkBitmap);

[Leon, 2012-07-16 15:27:06]

https://codereview.appspot.com/6345102/diff/12001/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (right):

https://codereview.appspot.com/6345102/diff/12001/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:164: public:
On 2012/07/16 13:51:13, reed1 wrote:
> For the future, I find this comment-style harder to read, and it doesn't
> automatically turn into doxygen dox. How about
> 
> /**
>  * THis is my
>  * block comment
>  */
> virtual void ...;
> 
> /**
>  * Note the space between functions
>  */
> void int ...;
> 

Done.

https://codereview.appspot.com/6345102/diff/12001/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:248: 
On 2012/07/16 13:51:13, reed1 wrote:
> // When we purge an entry, we want to reuse an old index for the new entry, so
> we expose this setter (or some other comment to this effect)

Done.

https://codereview.appspot.com/6345102/diff/12001/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:363: int indexToReplace = -1;
On 2012/07/16 13:51:13, reed1 wrote:
> Can you use fData.find(toReplace) instead of this for-loop?

Done.

https://codereview.appspot.com/6345102/diff/12001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.appspot.com/6345102/diff/12001/src/pipe/SkGPipeRead.cpp#ne...
src/pipe/SkGPipeRead.cpp:110: if(fBitmaps.count() == index) {
On 2012/07/16 13:51:13, reed1 wrote:
> minor:
> 
> bm = SkNEW(SkBitmap);

Done.

[reed1, 2012-07-16 15:31:07]

lgtm

[Leon, 2012-07-16 20:39:01]

Latest patch set refactors find so that Pipe calls a different version which
does the Pipe specific stuff and the Picture version ignores it. It also moves
the RefCntSet and FactorySet assignment into the constructor.

I had hoped to eliminate the (slight) slowdown in picture recording introduced
by my earlier patches, but did not achieve that.

[Leon, 2012-07-16 20:50:23]

When I used the profiler to compare with and without my changes to see why my
version is slower, my version ended up being faster, so I may not have
introduced any (or much) slow down.

[Leon, 2012-07-16 20:50:56]

When I used the profiler to compare with and without my changes to see why my
version is slower, my version ended up being faster, so I may not have
introduced any (or much) slow down.

[reed1, 2012-07-17 12:23:55]

lgtm for my part (w/ question about Sets and the controller)

https://codereview.appspot.com/6345102/diff/14002/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (right):

https://codereview.appspot.com/6345102/diff/14002/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:290: public:
Does it make sense to have the Controller own the various sets (refcnounts,
typefaces, etc.)? I ask mostly just seeing all of these parameters and
refs/unrefs. If it doesn't make sense, ignore it.

[Leon, 2012-07-17 15:51:46]

Derek, any thoughts?

https://codereview.appspot.com/6345102/diff/14002/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (right):

https://codereview.appspot.com/6345102/diff/14002/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:290: public:
On 2012/07/17 12:23:55, reed1 wrote:
> Does it make sense to have the Controller own the various sets (refcnounts,
> typefaces, etc.)? I ask mostly just seeing all of these parameters and
> refs/unrefs. If it doesn't make sense, ignore it.

Maybe it does. Certainly the Pipe has no other need of the FactorySet, and the
SkRefCntSets are only used by SkRefCntPlaybacks passed to unflattenDictionary
(which now has access to the Controller).

One downside I see is that currently all the dictionaries used by SkPicture
share a Controller, but they do not have the same needs in terms of ref sets.
That said, assigning the refset when it's not needed (i.e. for Matrices) only
has the downside of calling SkRefCnt_SafeAssign and unref for each buffer that's
created.

[DerekS, 2012-07-17 16:21:40]

I'm in favor of moving those into the controller, since it will make the code
more readable.  I can't think of any scenario that will need them separately and
even if we did we could always pull it back out.

lgtm