BSP Tree perf tests to match LayerSorter perf tests

BSP Tree perf tests to match LayerSorter perf tests

The BspTree perf tests should ideally perform equally or better than the
current LayerSorter tests do in the same tests (i.e. "rubik" vs.
"rubik").

Performance results on Z600:
layer_sort_cubes= 3.547ms
layer_sort_rubik= 0.634ms
bsp_tree_cubes= 1.485ms
bsp_tree_rubik= 0.136ms
bsp_tree_cubes_2= 2.915ms (2x duplicated layers)
bsp_tree_cubes_4= 5.667ms (4x duplicated layers)

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

[enne (OOO), 2014-07-28 20:48:53]

Can you upload a patch that doesn't contain the DrawPolygon changes? I am happy
to show you how if you need pointers.

[enne (OOO), 2014-07-28 23:37:40]

https://codereview.chromium.org/416273002/diff/60001/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/416273002/diff/60001/cc/base/math_util.cc#new...
cc/base/math_util.cc:85: SkMScalar w = 0.01f;
Just for the record (and to jog my memory), this was a subjectively determined
value based on some 3d scenes you had looked at? In particular, I know there was
the FPS-esque one where 0.000001f caused severe clipping issues but 0.01f was
fine, right?

Why is this needed with the perf tests?

https://codereview.chromium.org/416273002/diff/60001/cc/cc_tests.gyp
File cc/cc_tests.gyp (right):

https://codereview.chromium.org/416273002/diff/60001/cc/cc_tests.gyp#newcode326
cc/cc_tests.gyp:326: 'output/bsp_controller.cc',
I think these changes shouldn't be here.

https://codereview.chromium.org/416273002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common_perftest.cc (right):

https://codereview.chromium.org/416273002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:244:
MathUtil::MapClippedQuad3d((*it)->draw_transform(),
It seems to me like maybe you just need a DrawPolygon(DrawQuad*) constructor. 
Do you have to repeat this logic inside of DirectRenderer?

https://codereview.chromium.org/416273002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:339: TEST_F(BspTreePerfTest,
BspTreeRubik) {
For completeness can you add a BspTreeCubes_2 and BspTreeCubes_4 test where the
number of quads is doubled and quadrupled? I think one quad per layer is a bit
conservative, so want to see a few other numbers.  I suggest cubes because I
think that's the one with the highest number of layers.

https://codereview.chromium.org/416273002/diff/60001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.h (right):

https://codereview.chromium.org/416273002/diff/60001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.h:95: LayerTreeHostImpl* layer_tree_host_impl()
const {
Is this change needed?

[troyhildebrandt, 2014-07-29 19:57:17]

https://codereview.chromium.org/416273002/diff/60001/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/416273002/diff/60001/cc/base/math_util.cc#new...
cc/base/math_util.cc:85: SkMScalar w = 0.01f;
On 2014/07/28 23:37:40, enne wrote:
> Just for the record (and to jog my memory), this was a subjectively determined
> value based on some 3d scenes you had looked at? In particular, I know there
was
> the FPS-esque one where 0.000001f caused severe clipping issues but 0.01f was
> fine, right?
> 
> Why is this needed with the perf tests?

It's necessary for the perf tests because without it, we get inaccuracies in the
floating point values in the vertices during clipping. The visual issues are
just a result of this, and those issues could possibly have an effect on
sorting/splitting.

https://codereview.chromium.org/416273002/diff/60001/cc/cc_tests.gyp
File cc/cc_tests.gyp (right):

https://codereview.chromium.org/416273002/diff/60001/cc/cc_tests.gyp#newcode326
cc/cc_tests.gyp:326: 'output/bsp_controller.cc',
On 2014/07/28 23:37:40, enne wrote:
> I think these changes shouldn't be here.

Removed.

https://codereview.chromium.org/416273002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common_perftest.cc (right):

https://codereview.chromium.org/416273002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:244:
MathUtil::MapClippedQuad3d((*it)->draw_transform(),
On 2014/07/28 23:37:40, enne wrote:
> It seems to me like maybe you just need a DrawPolygon(DrawQuad*) constructor. 
> Do you have to repeat this logic inside of DirectRenderer?

Made a more general purpose constructor that will work for both testing and
rendering when the time comes.

https://codereview.chromium.org/416273002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:339: TEST_F(BspTreePerfTest,
BspTreeRubik) {
On 2014/07/28 23:37:40, enne wrote:
> For completeness can you add a BspTreeCubes_2 and BspTreeCubes_4 test where
the
> number of quads is doubled and quadrupled? I think one quad per layer is a bit
> conservative, so want to see a few other numbers.  I suggest cubes because I
> think that's the one with the highest number of layers.

Done.

https://codereview.chromium.org/416273002/diff/60001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.h (right):

https://codereview.chromium.org/416273002/diff/60001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.h:95: LayerTreeHostImpl* layer_tree_host_impl()
const {
On 2014/07/28 23:37:40, enne wrote:
> Is this change needed?

Not any more, definitely an artifact of a previous approach. Removed.

[enne (OOO), 2014-07-30 18:43:57]

https://codereview.chromium.org/416273002/diff/80001/cc/quads/draw_polygon.cc
File cc/quads/draw_polygon.cc (right):

https://codereview.chromium.org/416273002/diff/80001/cc/quads/draw_polygon.cc...
cc/quads/draw_polygon.cc:57: : order_index_(draw_order_index),
original_ref_(original_ref) {
I think you need to set the normal to a default value here.  Does this change
the perf numbers?

https://codereview.chromium.org/416273002/diff/80001/cc/quads/draw_polygon.cc...
cc/quads/draw_polygon.cc:61: MathUtil::MapClippedQuad3d(
Can you just make "gfx::QuadF quad(visible_content_rect)", set points[4]
directly, and then call TransformToScreenSpace, without having to do this
mapping yourself?

https://codereview.chromium.org/416273002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common_perftest.cc (right):

https://codereview.chromium.org/416273002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:245: (*it)->visible_content_rect(),
Are the visible content rects valid in this test? Could you change it to
gfx::Rect((*it)->bounds()) so that it's much more obvious that we're testing the
entire layer as a quad (since that's what the layer sorter is sorting against,
so it's slightly more fair)?

https://codereview.chromium.org/416273002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:255: while (dupe_counter-- > 0) {
Use a for loop here.

[troyhildebrandt, 2014-07-30 23:36:57]

https://codereview.chromium.org/416273002/diff/80001/cc/quads/draw_polygon.cc
File cc/quads/draw_polygon.cc (right):

https://codereview.chromium.org/416273002/diff/80001/cc/quads/draw_polygon.cc...
cc/quads/draw_polygon.cc:57: : order_index_(draw_order_index),
original_ref_(original_ref) {
On 2014/07/30 18:43:56, enne wrote:
> I think you need to set the normal to a default value here.  Does this change
> the perf numbers?

Done.

https://codereview.chromium.org/416273002/diff/80001/cc/quads/draw_polygon.cc...
cc/quads/draw_polygon.cc:61: MathUtil::MapClippedQuad3d(
On 2014/07/30 18:43:56, enne wrote:
> Can you just make "gfx::QuadF quad(visible_content_rect)", set points[4]
> directly, and then call TransformToScreenSpace, without having to do this
> mapping yourself?

I put a comment on it.

https://codereview.chromium.org/416273002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common_perftest.cc (right):

https://codereview.chromium.org/416273002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:245: (*it)->visible_content_rect(),
On 2014/07/30 18:43:56, enne wrote:
> Are the visible content rects valid in this test? Could you change it to
> gfx::Rect((*it)->bounds()) so that it's much more obvious that we're testing
the
> entire layer as a quad (since that's what the layer sorter is sorting against,
> so it's slightly more fair)?

Using bounds() now.

https://codereview.chromium.org/416273002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:255: while (dupe_counter-- > 0) {
On 2014/07/30 18:43:56, enne wrote:
> Use a for loop here.

Done.

[enne (OOO), 2014-07-31 00:18:04]

lgtm, except for one small fix.

Thanks for the performance numbers.  I think in general layers that are 3d
sorted are not going to be very large and will probably just be 1 quad.  Knowing
that having 2 quads per layer is still faster is exciting to see.

https://codereview.chromium.org/416273002/diff/130001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_perftest.cc (right):

https://codereview.chromium.org/416273002/diff/130001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_perftest.cc:245: gfx::RectF((*it)->bounds()),
I lied.  This should be content bounds and not normal bounds, since the draw
transform operates in that space.

[troyhildebrandt, 2014-07-31 18:32:38]

https://codereview.chromium.org/416273002/diff/130001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_perftest.cc (right):

https://codereview.chromium.org/416273002/diff/130001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_perftest.cc:245: gfx::RectF((*it)->bounds()),
On 2014/07/31 00:18:04, enne wrote:
> I lied.  This should be content bounds and not normal bounds, since the draw
> transform operates in that space.

Done.

[troyhildebrandt, 2014-07-31 18:37:44]

The CQ bit was checked by thildebr@chromium.org

[commit-bot: I haz the power, 2014-07-31 18:39:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thildebr@chromium.org/416273002/150001

[troyhildebrandt, 2014-07-31 22:25:30]

The CQ bit was checked by thildebr@chromium.org

[commit-bot: I haz the power, 2014-07-31 22:27:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thildebr@chromium.org/416273002/170001

[commit-bot: I haz the power, 2014-08-01 04:28:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 05:01:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-01 05:01:42]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2014-08-01 17:08:14]

Looks like the cc_unittests issues are real, probably caused by the math_util
changes.  Can you look into that?

[troyhildebrandt, 2014-08-02 00:08:13]

The CQ bit was checked by thildebr@chromium.org

[commit-bot: I haz the power, 2014-08-02 00:10:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thildebr@chromium.org/416273002/190001

[enne (OOO), 2014-08-02 00:19:42]

I'm happy to not have to change that constant.  :)

[enne (OOO), 2014-08-02 00:19:57]

Latest patch still lgtm.

[commit-bot: I haz the power, 2014-08-02 07:53:51]

Change committed as 287182