SkPathRef: one allocation for pts+verbs, path GenID, copy-on-write

[reed1, 2012-08-31 15:50:33]

1. does it warrant new unittests or benches? (just asking)
2. what are the current bench results w/ and w/o?

[reed1, 2012-08-31 15:51:41]

https://codereview.appspot.com/6488063/diff/2004/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/6488063/diff/2004/include/core/SkPath.h#newcod...
include/core/SkPath.h:832: 
Its not wrong, but this may be the first smart-ptr we embed inside a core
class...

[bsalomon, 2012-08-31 16:08:51]

On 2012/08/31 15:50:33, reed1 wrote:
> 1. does it warrant new unittests or benches? (just asking)

Around 10 benches (path_copy, path_transform_inplace, ...) were added a while
ago with this change in mind.

The existing unit tests (and GM) proved really useful for debugging this. It
wasn't obvious to me what new unit tests should be added. I was thinking about
doing a DRT run with this applied as a preflight.

> 2. what are the current bench results w/ and w/o?

Stop by my desk and see the comparison with -repeat 200. The range is -7% to
+75%.

[bsalomon, 2012-08-31 16:19:13]

On 2012/08/31 15:51:41, reed1 wrote:
> https://codereview.appspot.com/6488063/diff/2004/include/core/SkPath.h
> File include/core/SkPath.h (right):
> 
>
https://codereview.appspot.com/6488063/diff/2004/include/core/SkPath.h#newcod...
> include/core/SkPath.h:832: 
> Its not wrong, but this may be the first smart-ptr we embed inside a core
> class...

We've started doing it in src/gpu but you're right that this is the first
instance in core.

[reed1, 2012-08-31 16:52:12]

adding cary...

- can we move SkPathRef into a sep .h file?
- consider inlining the fRefCnt field, so we have 1 alloc instead of 2
- can we simplify things by using ~ for verb index, and not have to add 1 when
we subtract ptrs?
- can we commit to verbs-go-backwards to simplify reading the code?
- I love it

[bsalomon, 2012-09-04 19:05:35]

On 2012/08/31 16:52:12, reed1 wrote:
> adding cary...
> 
> - can we move SkPathRef into a sep .h file?

Done.

> - consider inlining the fRefCnt field, so we have 1 alloc instead of 2

Good idea, I'd like to save for a followup CL.

> - can we simplify things by using ~ for verb index, and not have to add 1 when
> we subtract ptrs?

Done.

> - can we commit to verbs-go-backwards to simplify reading the code?

Done, committed to points forward, verbs backward.

[reed1, 2012-09-04 21:05:21]

looks good to me. I wonder if derek or leon want to try running w/ this patch on
their multi-threaded experiments...

https://codereview.appspot.com/6488063/diff/8002/src/core/SkPathRef.h
File src/core/SkPathRef.h (right):

https://codereview.appspot.com/6488063/diff/8002/src/core/SkPathRef.h#newcode206
src/core/SkPathRef.h:206: * Convenience methods for getting to a verb or point
by index.
I think a common name for these is at()

e.g. atVerb(index), atPoint(index)

Is it easy to add debug-range-checking in these accessors?

https://codereview.appspot.com/6488063/diff/8002/src/core/SkPathRef.h#newcode467
src/core/SkPathRef.h:467: int                 fPointCnt;
Do/can we assert that fFreeSpace is always consistent with its computed value?

[bsalomon, 2012-09-04 21:23:08]

https://codereview.appspot.com/6488063/diff/8002/src/core/SkPathRef.h
File src/core/SkPathRef.h (right):

https://codereview.appspot.com/6488063/diff/8002/src/core/SkPathRef.h#newcode206
src/core/SkPathRef.h:206: * Convenience methods for getting to a verb or point
by index.
On 2012/09/04 21:05:21, reed1 wrote:
> I think a common name for these is at()
> 
> e.g. atVerb(index), atPoint(index)
> 
> Is it easy to add debug-range-checking in these accessors?

Renamed and added checks. Also made the editor's version of atPoint return a ptr
rather than a ref.

https://codereview.appspot.com/6488063/diff/8002/src/core/SkPathRef.h#newcode467
src/core/SkPathRef.h:467: int                 fPointCnt;
On 2012/09/04 21:05:21, reed1 wrote:
> Do/can we assert that fFreeSpace is always consistent with its computed value?

validate() has this check:
SkASSERT(this->currSize() == fFreeSpace + sizeof(SkPoint) * fPointCnt +
sizeof(uint8_t) * fVerbCnt);

[Leon, 2012-09-04 21:31:14]

On 2012/09/04 21:05:21, reed1 wrote:
> looks good to me. I wonder if derek or leon want to try running w/ this patch
on
> their multi-threaded experiments...

bench_pictures has an option to run in multi-threaded mode. To try it, run:

bench_pictures -tiled W H multi

where W is either a width or a percentage of total width for the width of a
tile, and H is similar.

[Leon, 2012-09-05 12:10:42]

On 2012/09/04 21:31:14, scroggo-work wrote:
> On 2012/09/04 21:05:21, reed1 wrote:
> > looks good to me. I wonder if derek or leon want to try running w/ this
patch
> on
> > their multi-threaded experiments...
> 
> bench_pictures has an option to run in multi-threaded mode. To try it, run:
> 
> bench_pictures -tiled W H multi
> 
> where W is either a width or a percentage of total width for the width of a
> tile, and H is similar.

Oops. That's -mode tiled.

But I realized that this will probably not reveal anything interesting, since we
do not make copies of the paths when cloning a picture. We do when recording a
picture, so you could run

bench_pictures -mode record

(I think that's the correct mode; if you enter something that does not work it
will print the correct commands.)

[bsalomon, 2012-09-05 17:32:15]

On 2012/09/05 12:10:42, scroggo-work wrote:
> On 2012/09/04 21:31:14, scroggo-work wrote:
> > On 2012/09/04 21:05:21, reed1 wrote:
> > > looks good to me. I wonder if derek or leon want to try running w/ this
> patch
> > on
> > > their multi-threaded experiments...
> > 
> > bench_pictures has an option to run in multi-threaded mode. To try it, run:
> > 
> > bench_pictures -tiled W H multi
> > 
> > where W is either a width or a percentage of total width for the width of a
> > tile, and H is similar.
> 
> Oops. That's -mode tiled.
> 
> But I realized that this will probably not reveal anything interesting, since
we
> do not make copies of the paths when cloning a picture. We do when recording a
> picture, so you could run
> 
> bench_pictures -mode record
> 
> (I think that's the correct mode; if you enter something that does not work it
> will print the correct commands.)

Running bench_pictures against the files in /skp didn't reveal any interesting
performance changes. It did remind me that I changed the flattened form of
paths. I added a #define to preserve the old format for now. It will take a
follow on change to tunnel knowledge of whether the serialization is x-process
or not which will determine whether the genID or a 0 gets written. Also, if this
were to be reverted for any reason we won't have thrashed the picture format.

[bsalomon, 2012-09-11 13:48:14]

Closed with r5433