Tighten the matrix usage in cc/layer_tree_host_common.cpp

Tighten the matrix usage in cc/layer_tree_host_common.cpp

As calculateDrawTransforms has evolved, the math has become substantially more
overhead. This patch refactors the matrix math to reduce the number of matrix
copies and matrix multiplications. Changes that would drastically affect
readability or robustness are avoided.

With this patch, cc_perftests improves by approximately 6-8%.
SevenTabSwitcher: 2030 runs/s --> 2220 runs/s  (9.3%)
tenTabSwitcher: 757 runs/s --> 807 runs/s  (6.6%)

BUG=163769
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171051

[shawnsingh, 2012-12-02 06:43:16]

PTAL thanks!

[danakj, 2012-12-03 18:24:40]

https://codereview.chromium.org/11280271/diff/1/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (left):

https://codereview.chromium.org/11280271/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:526: // start of the combinedTransform, but we
don't want it to affect the contentsScale.
implTransform should not affect the contentsScale. That's why it's below
updateLayerContentsScale, and what this comment was saying.

[shawnsingh, 2012-12-03 19:37:06]

Thanks for review, we need to discuss this point before new patch comes =)

https://codereview.chromium.org/11280271/diff/1/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (left):

https://codereview.chromium.org/11280271/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:526: // start of the combinedTransform, but we
don't want it to affect the contentsScale.
On 2012/12/03 18:24:40, danakj wrote:
> implTransform should not affect the contentsScale. That's why it's below
> updateLayerContentsScale, and what this comment was saying.

Yes, I was planning to ask you about this before landing the patch.   All I did
was provable math substitutions.  I'm pretty sure I didn't change what would be
computed here.  In particular, ignoring the comment for a moment, this code
tacks implTransform to the left side of the composite sequence of matrix
multiplications, unconditionally.   So it is equivalent to move it up to the
initialization of combinedTransform (by matrix associativity), as long as the
sequence of multiplications itself doesn't change order.

So, I feel that the comment + original code were not quite adding up clearly...
one of them may need to be fixed.  As far as I could reason, multiplying
layer->implTransform() on the left side does correctly _not_ affect
contentsScale.  So either the left-side multiplication here is not quite in the
correct place (but avoids content scale), or the comment itself should be
updated and I can re-instate the comment in the new code...

[danakj, 2012-12-03 19:56:28]

https://codereview.chromium.org/11280271/diff/1/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (left):

https://codereview.chromium.org/11280271/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:526: // start of the combinedTransform, but we
don't want it to affect the contentsScale.
On 2012/12/03 19:37:06, shawnsingh wrote:
> On 2012/12/03 18:24:40, danakj wrote:
> > implTransform should not affect the contentsScale. That's why it's below
> > updateLayerContentsScale, and what this comment was saying.
> 
> Yes, I was planning to ask you about this before landing the patch.   All I
did
> was provable math substitutions.  I'm pretty sure I didn't change what would
be
> computed here.  In particular, ignoring the comment for a moment, this code
> tacks implTransform to the left side of the composite sequence of matrix
> multiplications, unconditionally.   So it is equivalent to move it up to the
> initialization of combinedTransform (by matrix associativity), as long as the
> sequence of multiplications itself doesn't change order.
> 
> So, I feel that the comment + original code were not quite adding up
clearly...
> one of them may need to be fixed.  As far as I could reason, multiplying
> layer->implTransform() on the left side does correctly _not_ affect
> contentsScale.  So either the left-side multiplication here is not quite in
the
> correct place (but avoids content scale), or the comment itself should be
> updated and I can re-instate the comment in the new code...

updateLayerContentsScale wants the combinedTransform without the implTransform
included.

You're right the combinedTransform ends up being the same here. But the
transform passed to updateLayerContentsScale is not the same.

[shawnsingh, 2012-12-03 20:30:46]

Got it =)  will fix.

[shawnsingh, 2012-12-04 03:28:42]

On 2012/12/03 20:30:46, shawnsingh wrote:
> Got it =)  will fix.

New patch uploaded.  PTAL, thanks!

[danakj, 2012-12-04 18:27:42]

LGTM with nits

https://codereview.chromium.org/11280271/diff/7001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11280271/diff/7001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:525: // Note Carefully: this is Concat, not
Preconcat (implTransform * combinedTransform).
nit: carefully lowercase

https://codereview.chromium.org/11280271/diff/7001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:532: // Note Carefully: this is Concat, not
Preconcat (currentScrollCompensation * combinedTransform).
same

https://codereview.chromium.org/11280271/diff/7001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:570: // space which do not want; so here we use the
combinedTransform
nit: "which do not want"

https://codereview.chromium.org/11280271/diff/7001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:573: combinedTransform.Scale(1 /
renderSurfaceSublayerScale.x(), 1 / renderSurfaceSublayerScale.y());
changing the combinedTransform makes me a bit uncomfortable for some reason. i
see it'll work ok for now though, as the other use is only in the else clause.

[shawnsingh, 2012-12-04 18:31:39]

Thanks for review =)

https://codereview.chromium.org/11280271/diff/7001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11280271/diff/7001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:573: combinedTransform.Scale(1 /
renderSurfaceSublayerScale.x(), 1 / renderSurfaceSublayerScale.y());
I think this is most likely going to be a non-issue anyway after the
drawProperties patch which will compute the drawTransform in-place.

[danakj, 2012-12-04 18:32:48]

https://codereview.chromium.org/11280271/diff/7001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11280271/diff/7001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:573: combinedTransform.Scale(1 /
renderSurfaceSublayerScale.x(), 1 / renderSurfaceSublayerScale.y());
On 2012/12/04 18:31:39, shawnsingh wrote:
> I think this is most likely going to be a non-issue anyway after the
> drawProperties patch which will compute the drawTransform in-place.

Thanks for the reassurance. :)

[shawnsingh, 2012-12-05 00:52:09]

So it turns out I was wrong, looking at the wrong thing.  The drawProperties
patch does not affect this code.

I'm happy to discuss if you would like to change this somehow, as long as we are
sensitive to the number of unnecessary copies we make.   Conceptually, it is
completely correct that combinedTransform should no longer be used after setting
surface/layer drawTransforms.  If there's some way to enforce that
combinedTransform remains un-used after that point in code, I think that would
be what you want.

We could do this by introducing additional scoping.... I'm not sure how other
people feel about that, but it could actually be a nice trick.  In some
alternate universe, it might even help performance slightly by keeping the stack
shorter overall.

[danakj, 2012-12-05 18:27:28]

On 2012/12/05 00:52:09, shawnsingh wrote:
> So it turns out I was wrong, looking at the wrong thing.  The drawProperties
> patch does not affect this code.
> 
> I'm happy to discuss if you would like to change this somehow, as long as we
are
> sensitive to the number of unnecessary copies we make.   Conceptually, it is
> completely correct that combinedTransform should no longer be used after
setting
> surface/layer drawTransforms.  If there's some way to enforce that
> combinedTransform remains un-used after that point in code, I think that would
> be what you want.
> 
> We could do this by introducing additional scoping.... I'm not sure how other
> people feel about that, but it could actually be a nice trick.  In some
> alternate universe, it might even help performance slightly by keeping the
stack
> shorter overall.

Or maybe set the transform to identity after consuming it?

[shawnsingh, 2012-12-05 19:00:10]

On 2012/12/05 18:27:28, danakj wrote:
> Or maybe set the transform to identity after consuming it?


We would like to avoid such unneeded computations, that's the point of this
tightening...

Last night, I analyzed the sequence of operations inside of draw transforms and
screen space transforms for layers and surfaces, and it looks like there is an
elegant way to rewrite all this math that will further reduce the number of
multiplications and copies.  Let's continue discussing this after seeing whether
that patch helps or not =)