Keep track of identity SkMatrix44s

Keep track of identity SkMatrix44s

This keeps a bool on SkMatrix44 indicating if we know for sure that the matrix
is identity, similar to the TypeMask on SkMatrix. This is useful to early-out
of potentially expensive matrix math at the cost of some memory.
Committed: https://code.google.com/p/skia/source/detail?r=6620

[jamesr, 2012-11-29 11:28:31]

Hey Mike, what do you think of this?  In combination with some chromium-side
patches this speeds up an end-to-end compositor benchmark by about 2% on its
own, which is not too shabby.  It would probably show up a lot hotter on a more
targetting matrix math microbenchmark.  I'd like to also extend the caching to
store if the matrix is a translate/scale/perspective/etc, like SkMatrix, but
want to check with you on approach first.  We could keep extending the caching
here in Skia or I could keep this interface small+tight and do state tracking on
the chromium wrapper for this type.  The only downside there is I'd have to be
very careful about anything that accesses the underlying SkMatrix44 type - which
is solvable but a bit of a pain.

[reed1, 2012-11-29 12:59:52]

I like the direction, and we could land this for now, but I agree with you that
a fuller characterization of the matrix will almost certainly be useful. We get
lots of milage out of the bit field we have on SkMatrix.

Do we have a feel for common special-cases matrices in your uses? e.g. is it
common that the 44 is really 2D, or 3D w/ no perspective, or just scale or
translate? My swag for fields would be

translate = (1 << 0)
scale = (1 << 1)
affine = (1 <<2)    // no perspective

That was my plan anyway. That said, if you get a win out of the current CL, lets
land it and we can add more bits later.

[danakj, 2012-11-29 18:16:18]

Can we then also make methods like multiply faster based on this flag and
others, by avoiding reading/writing matrix positions that are redundant based on
the flags.

For instance with identity if you multiply two matrices, you don't need to read
the identity matrix or do any multiplication operations at all.

[reed1, 2012-11-29 18:57:29]

On 2012/11/29 18:16:18, danakj wrote:
> Can we then also make methods like multiply faster based on this flag and
> others, by avoiding reading/writing matrix positions that are redundant based
on
> the flags.
> 
> For instance with identity if you multiply two matrices, you don't need to
read
> the identity matrix or do any multiplication operations at all.

This is precisely what SkMatrix does with its flags. concat is faster, and
matrix*point is faster.

[shawnsingh, 2012-11-29 19:09:43]

James and I just talked;   James said he would like to land the current patch
as-is (assuming it's correct)

and then I will take over and get more optimizations similar to SkMatrix in
place soon, including (priority 1) translation/scaling parameters rather than
matrices just like danakj@ and reed@ are saying, and (priority 2)  reducing
copies in the APIs

does that sound OK to everyone?

[shawnsingh, 2012-11-29 19:11:51]

By the way, reducing copies may be painfully invasive to a lot of code, but I
think it will be worth it in the end.  We should make sure everyone is on board
for changing the API that way.  I know vollick@ and I had discussed before, and
we were both thinking of moving the gfx::Transform API in that direction.  Now
we have a reason to do this sooner than later...

[danakj, 2012-11-29 19:12:37]

On 2012/11/29 19:11:51, shawnsingh wrote:
> By the way, reducing copies may be painfully invasive to a lot of code, but I
> think it will be worth it in the end.  We should make sure everyone is on
board
> for changing the API that way.  I know vollick@ and I had discussed before,
and
> we were both thinking of moving the gfx::Transform API in that direction.  Now
> we have a reason to do this sooner than later...

I'm not clear what API changes would be needed, but I guess I'll see in code
review :)

I'm fine with landing this, it's an improvement, and building on it.

[shawnsingh, 2012-11-29 19:36:09]

API changes could happen on gfx::transform or Skia... I think they will be
useful either way.  There may be some kinks doing it in gfx::transform, but we
can address those as-needed?

API changes that would be required for removing unnecessary copies:

- remove operators like *

- any functions that have return values of matrices or other data types....
should instead pass them by reference.   This will remove a lot of unnecessary
copies from within the transform library, and will force users of the API to
explicitly instantiate matrices, so they will be more aware of the cost, and are
incentivized to reduce matrix instances.

- possibly remove / privatize some constructors to disallow them.

- possibly add more constructors that allow us to initialize translations,
scales, etc. directly.


On the internal side:

- implementations of various functions should not be condensed by instantiating
matrices and using operators.  They should try to do computations in-place, or
at least use floats directly that the compiler can take advantage of them being
separate pieces of data rather than an object.    This would probably fall out
of changing the API in the first place.

[shawnsingh_google.com, 2012-11-29 19:36:38]

James and I just talked;   James said he would like to land the current
patch as-is (assuming it's correct)

and then I will take over and get more optimizations in place soon,
including (priority 1) translation/scaling parameters rather than matrices,
and (priority 2)  reducing copies in the APIs

~Shawn


On Thu, Nov 29, 2012 at 10:57 AM, <reed@google.com> wrote:

> On 2012/11/29 18:16:18, danakj wrote:
>
>> Can we then also make methods like multiply faster based on this flag
>>
> and
>
>> others, by avoiding reading/writing matrix positions that are
>>
> redundant based on
>
>> the flags.
>>
>
>  For instance with identity if you multiply two matrices, you don't
>>
> need to read
>
>> the identity matrix or do any multiplication operations at all.
>>
>
> This is precisely what SkMatrix does with its flags. concat is faster,
> and matrix*point is faster.
>
>
https://codereview.appspot.**com/6854113/<https://codereview.appspot.com/6854...
>

[Stephen White, 2012-11-29 19:40:59]

On 2012/11/29 19:36:09, shawnsingh wrote:
> API changes that would be required for removing unnecessary copies:
> 
> - remove operators like *
> 
> - any functions that have return values of matrices or other data types....
> should instead pass them by reference.   This will remove a lot of unnecessary
> copies from within the transform library, and will force users of the API to
> explicitly instantiate matrices, so they will be more aware of the cost, and
are
> incentivized to reduce matrix instances.

Does the Return Value Optimization not take care of this?

> - possibly remove / privatize some constructors to disallow them.
> 
> - possibly add more constructors that allow us to initialize translations,
> scales, etc. directly.
> 
> 
> On the internal side:
> 
> - implementations of various functions should not be condensed by
instantiating
> matrices and using operators.  They should try to do computations in-place, or
> at least use floats directly that the compiler can take advantage of them
being
> separate pieces of data rather than an object.    This would probably fall out
> of changing the API in the first place.

[shawnsingh, 2012-11-29 19:46:26]

In my personal experience, return value optimization doesn't always kick-in as
hoped.  It probably depends on which compiler and which platform, i'm not deeply
familiar enough with which platforms/compilers RVO would work.  If someone else
knows more about this, it's worth a try.   But given the large variety of
platforms and compilers to support, maybe we shouldn't bank on it?

[jamesr, 2012-11-29 20:33:47]

On 2012/11/29 18:57:29, reed1 wrote:
> On 2012/11/29 18:16:18, danakj wrote:
> > Can we then also make methods like multiply faster based on this flag and
> > others, by avoiding reading/writing matrix positions that are redundant
based
> on
> > the flags.
> > 
> > For instance with identity if you multiply two matrices, you don't need to
> read
> > the identity matrix or do any multiplication operations at all.
> 
> This is precisely what SkMatrix does with its flags. concat is faster, and
> matrix*point is faster.

OK.  Currently gfx::Transform is doing optimizations like this, but we could
move those up to SkMatrix44.  This does mean adding branches even in cases where
the caller might already know that the matrix isn't identity, but that seems
pretty minor compared to the cost of doing a full matrix op.  As a follow-up to
this we could move these opts up to SKM44 if you'd prefer.

[reed1, 2012-11-29 20:52:09]

How about this.

1. land this CL that catches identity
2. mike will take a pass at replicating the SkMatrix trans/scale/affine checks
3. we re-profile

If there is general cleanup in gfx::Transform, that can happen in parallel (I
hope)

[jamesr, 2012-11-29 20:57:23]

On 2012/11/29 20:52:09, reed1 wrote:
> How about this.
> 
> 1. land this CL that catches identity

Sounds good!  Can you take care of that (or I could ask Stephen who's here) - I
don't know how to land skia patches and am sure I don't have permissions.

> 2. mike will take a pass at replicating the SkMatrix trans/scale/affine checks

OK - Shawn expressed interest in doing these as well, could y'all coordinate
offline about who's doing what when?  I think most of this work is pretty simple
so it's just a matter of what's on each person's plate.

> 3. we re-profile

Yes!

> 
> If there is general cleanup in gfx::Transform, that can happen in parallel (I
> hope)

Sounds like a plan.  Shawn has volunteered to look at this side.  I think we
just need to decide which side of the interface is responsible for which
optimizations, then make sure they happen.

[reed1, 2012-11-29 20:59:51]

I am/will coordinate with Shawn

[shawnsingh, 2012-11-29 21:01:13]

OK so if Mike works on the SkMatrix trans/scale/affine checks, I'll work on
reformulating the gfx::transform API to reduce copies where possible.

[reed1, 2012-11-29 21:06:02]

lgtm