Optimize QuadF's IsCounterClockwise function

Optimize QuadF's IsCounterClockwise function

IsCounterClockwise is a busy function,See performance and discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/graphics-dev/aSvAbzcgbos

This patch reduce IsCounterClockwise() from 8mul+3add+4sub to 4mul+2add+2sub,
and performance doubled in real micro-bench.

R=danakj@chromium.org

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

[danakj, 2014-08-11 14:16:35]

On Mon, Aug 11, 2014 at 3:03 AM, <kui.zheng@arm.com> wrote:

> Signed-off-by: kui zheng <kui.zheng@arm.com>
>

BTW While we use git (or pretend to at the moment), we don't need to
include Signed-off-by headers in the commit logs, at least I never see this
in Chromium at this time.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-08-11 14:18:49]

+vollick for algebra

https://codereview.chromium.org/458113002/diff/1/ui/gfx/geometry/quad_f.cc
File ui/gfx/geometry/quad_f.cc (right):

https://codereview.chromium.org/458113002/diff/1/ui/gfx/geometry/quad_f.cc#ne...
ui/gfx/geometry/quad_f.cc:48: float tmp1 = p2_.y() - p4_.y();
how about naming this p24 or something more descriptive than "tmp"?

https://codereview.chromium.org/458113002/diff/1/ui/gfx/geometry/quad_f.cc#ne...
ui/gfx/geometry/quad_f.cc:49: float tmp2 = p3_.y() - p1_.y();
ditto

[Ian Vollick, 2014-08-11 14:39:09]

Neat! LGTM.

Just one minor naming nit.

https://codereview.chromium.org/458113002/diff/1/ui/gfx/geometry/quad_f.cc
File ui/gfx/geometry/quad_f.cc (right):

https://codereview.chromium.org/458113002/diff/1/ui/gfx/geometry/quad_f.cc#ne...
ui/gfx/geometry/quad_f.cc:52: double determinant1 = static_cast<double>(p1_.x())
* tmp1;
If I'm understanding things, determinant1 here is no longer a determinant, but
rather computes some of the terms involved in the old code's sum. If that's
true, I'd vote for something like what Dana suggested above for tmp1 and tmp2
rather than "determinant" and maybe add a comment explaining that what's here is
equivalent to computing the sum of the determinants (i.e., that if you flip the
sign of determinant3 and determinant4 and add them to determinant1 and
determinant2 here, you get the same value as before).

[kui.zheng, 2014-08-12 04:20:55]

https://codereview.chromium.org/458113002/diff/1/ui/gfx/geometry/quad_f.cc
File ui/gfx/geometry/quad_f.cc (right):

https://codereview.chromium.org/458113002/diff/1/ui/gfx/geometry/quad_f.cc#ne...
ui/gfx/geometry/quad_f.cc:48: float tmp1 = p2_.y() - p4_.y();
On 2014/08/11 14:18:49, danakj wrote:
> how about naming this p24 or something more descriptive than "tmp"?

Done.

https://codereview.chromium.org/458113002/diff/1/ui/gfx/geometry/quad_f.cc#ne...
ui/gfx/geometry/quad_f.cc:49: float tmp2 = p3_.y() - p1_.y();
On 2014/08/11 14:18:49, danakj wrote:
> ditto

Done.

https://codereview.chromium.org/458113002/diff/1/ui/gfx/geometry/quad_f.cc#ne...
ui/gfx/geometry/quad_f.cc:52: double determinant1 = static_cast<double>(p1_.x())
* tmp1;
On 2014/08/11 14:39:09, Ian Vollick wrote:
> If I'm understanding things, determinant1 here is no longer a determinant, but
> rather computes some of the terms involved in the old code's sum. If that's
> true, I'd vote for something like what Dana suggested above for tmp1 and tmp2
> rather than "determinant" and maybe add a comment explaining that what's here
is
> equivalent to computing the sum of the determinants (i.e., that if you flip
the
> sign of determinant3 and determinant4 and add them to determinant1 and
> determinant2 here, you get the same value as before).

Right, This patch just refactoring the computation of adding determinants so
that reducing the number of operations. So, "determinant" is meaningless here.

[kui.zheng, 2014-08-12 04:21:24]

On 2014/08/11 14:16:35, danakj wrote:
> On Mon, Aug 11, 2014 at 3:03 AM, <mailto:kui.zheng@arm.com> wrote:
> 
> > Signed-off-by: kui zheng <mailto:kui.zheng@arm.com>
> >
> 
> BTW While we use git (or pretend to at the moment), we don't need to
> include Signed-off-by headers in the commit logs, at least I never see this
> in Chromium at this time.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Got it, Thanks for your remind

[danakj, 2014-08-12 13:56:00]

LGTM

[Ian Vollick, 2014-08-12 13:58:31]

On 2014/08/12 13:56:00, danakj wrote:
> LGTM

lgtm2

[Ian Vollick, 2014-08-12 13:58:34]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-08-12 14:01:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kui.zheng@arm.com/458113002/40001

[commit-bot: I haz the power, 2014-08-13 00:42:26]

Change committed as 289138