First pass at refactoring flatted dictionaries.

Refactor dictionaries for use by entities other than just SkPicture
Committed: https://code.google.com/p/skia/source/detail?r=4077

[Leon, 2012-04-30 20:29:45]

https://codereview.appspot.com/6101043/diff/5022/include/core/SkRegion.h
File include/core/SkRegion.h (right):

https://codereview.appspot.com/6101043/diff/5022/include/core/SkRegion.h#newc...
include/core/SkRegion.h:360: uint32_t unflatten32(SkReader32*);
Should this parameter be

const SkReader32*

?

The other classes also changed their parameters from "const void*" to
"SkReader32*"; I assume the answer should be the same for all of them.

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

https://codereview.appspot.com/6101043/diff/5022/src/core/SkPictureFlat.h#new...
src/core/SkPictureFlat.h:152: template <class T>
Should we enforce that T is a class that has a flatten and unflatten method? The
classes we use currently have no superclass; should they all derive from a base
class that has flatten and unflatten?

[DerekS, 2012-04-30 20:38:53]

https://codereview.appspot.com/6101043/diff/5022/include/core/SkRegion.h
File include/core/SkRegion.h (right):

https://codereview.appspot.com/6101043/diff/5022/include/core/SkRegion.h#newc...
include/core/SkRegion.h:360: uint32_t unflatten32(SkReader32*);
While the reader's buffer is const the reader itself is not. The reader updates
it's position within the buffer and number of bytes read so that is why I didn't
mark it const.

On 2012/04/30 20:29:45, scroggo1 wrote:
> Should this parameter be
> 
> const SkReader32*
> 
> ?
> 
> The other classes also changed their parameters from "const void*" to
> "SkReader32*"; I assume the answer should be the same for all of them.

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

https://codereview.appspot.com/6101043/diff/5022/src/core/SkPictureFlat.h#new...
src/core/SkPictureFlat.h:152: template <class T>
I was a little torn about this.  I didn't want those methods to have a
superclass as they are simple structs and I didn't want to introduce a vtable.
The template does do a compile time check to ensure the methods are there so
this won't be a runtime problem.

However, I should add some documentation that we require a flattenable item to
support those 2 methods.  Any suggestion on the best place to add it for future
readers?

On 2012/04/30 20:29:45, scroggo1 wrote:
> Should we enforce that T is a class that has a flatten and unflatten method?
The
> classes we use currently have no superclass; should they all derive from a
base
> class that has flatten and unflatten?

[Leon, 2012-04-30 20:46:37]

https://codereview.appspot.com/6101043/diff/5022/include/core/SkRegion.h
File include/core/SkRegion.h (right):

https://codereview.appspot.com/6101043/diff/5022/include/core/SkRegion.h#newc...
include/core/SkRegion.h:360: uint32_t unflatten32(SkReader32*);
Ah yes. Nvm.

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

https://codereview.appspot.com/6101043/diff/5022/src/core/SkPictureFlat.h#new...
src/core/SkPictureFlat.h:152: template <class T>
I think right above the template would be a good place for such a comment.

[Leon, 2012-05-01 17:35:16]

LGTM

[reed1, 2012-05-07 20:45:25]

I like the code sharing.

Do we have sufficient benches for recording pictures to know if we've affected
that?

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

https://codereview.appspot.com/6101043/diff/14001/src/core/SkPictureFlat.h#ol...
src/core/SkPictureFlat.h:144: 
Should these 2 be lined still, since they are no longer templated?

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

https://codereview.appspot.com/6101043/diff/14001/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:225: fHeap = heap;
is there a clear comment for why we init fNextIndex to 1 (just curious)?

https://codereview.appspot.com/6101043/diff/14001/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:236: 
Add comment block for what it does, and what it returns?

[junov1, 2012-05-07 21:06:28]

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

https://codereview.appspot.com/6101043/diff/14001/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:229: void reset() { fData.reset(); fNextIndex = 1; }
Don't we need to reset fHeap as well? Since the heap is a constructor argument,
I guess it is possible for multiple SkFlatDictionary objects to share the same
SkChunkAlloc.  Is that your intention?  If so, the the reset method should
probably explicitly deallocate (fHeap->unalloc) each item in fData before
calling fData.reset().  If you do not intend for multiple dictionaries to share
the same heap, then fHeap->reset is fine, but maybe it would be safer to have
SkFlatDictionary create its heap internally and fully own it.

[DerekS, 2012-05-08 13:11:18]

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

https://codereview.appspot.com/6101043/diff/14001/src/core/SkPictureFlat.h#ol...
src/core/SkPictureFlat.h:144: 
On 2012/05/07 20:45:25, reed1 wrote:
> Should these 2 be lined still, since they are no longer templated?

Done.

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

https://codereview.appspot.com/6101043/diff/14001/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:225: fHeap = heap;
On 2012/05/07 20:45:25, reed1 wrote:
> is there a clear comment for why we init fNextIndex to 1 (just curious)?

Done.

https://codereview.appspot.com/6101043/diff/14001/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:229: void reset() { fData.reset(); fNextIndex = 1; }
Yes, the intention is that multiple dictionaries can use the same SkChunkAlloc. 
The allocator doesn't provide any means for individual allocations to be freed
so it is up to the caller to reset() or reuse() the allocator.  I've documented
that assumption in my latest patch set.

FWIW, this is the current behavior for SkPicture.

On 2012/05/07 21:06:29, junov1 wrote:
> Don't we need to reset fHeap as well? Since the heap is a constructor
argument,
> I guess it is possible for multiple SkFlatDictionary objects to share the same
> SkChunkAlloc.  Is that your intention?  If so, the the reset method should
> probably explicitly deallocate (fHeap->unalloc) each item in fData before
> calling fData.reset().  If you do not intend for multiple dictionaries to
share
> the same heap, then fHeap->reset is fine, but maybe it would be safer to have
> SkFlatDictionary create its heap internally and fully own it.

https://codereview.appspot.com/6101043/diff/14001/src/core/SkPictureFlat.h#ne...
src/core/SkPictureFlat.h:236: 
On 2012/05/07 20:45:25, reed1 wrote:
> Add comment block for what it does, and what it returns?

Done.

[reed1, 2012-05-08 15:35:34]

Seems good. I'd like to see a bench before we commit this.

[DerekS, 2012-05-30 16:01:17]

I've tested this CL with the following bench and the results 
show that the new CL is slightly faster at around 11.75ms on average compared to
the old version which was 12.5ms.

So in the end we not only cleaned up the code during the refactoring we also
made is slightly faster.

http://codereview.appspot.com/6258062/