Use custom ListContainer to allocate DrawQuads

Use custom ListContainer to allocate DrawQuads

In RenderPass use ListContainer for generating DrawQuads and acting as
QuadList.

BUG=344962
Committed: https://crrev.com/032e929d161f8e466a55913deb748f0975c1f7de
Cr-Commit-Position: refs/heads/master@{#296855}

[weiliangc, 2014-08-20 23:30:19]

danakj: cc
kenrb: cc_messages
jamesr: mojo

Overall performance gain in Android, mostly noise on linux.
Details can be find:
https://docs.google.com/spreadsheets/d/1zyrTwQ_nd1k8M_RlnxHUIDZyJ3XYST33kEB6e...

[jamesr, 2014-08-20 23:34:13]

lgtm

[jamesr, 2014-09-15 21:48:42]

(to be clear I'm removing the mojo/ bits, not the rest (unless you want me to))

https://codereview.chromium.org/448303002/diff/80001/mojo/services/public/cpp...
File mojo/services/public/cpp/surfaces/tests/surface_unittest.cc (right):

https://codereview.chromium.org/448303002/diff/80001/mojo/services/public/cpp...
mojo/services/public/cpp/surfaces/tests/surface_unittest.cc:74: QuadPtr
mojo_quad = Quad::From(*base_ptr);
actually, a better way to do this is just one line:

QuadPtr mojo_quad = Quad::From<cc::DrawQuad>(color_quad);

etc.

[weiliangc, 2014-09-15 22:18:11]

danakj@ has been reviewing the CL this is depended on. I'll need to update
this again when I change that patch so tgis is not the urgent.

Thanks.
(to be clear I'm removing the mojo/ bits, not the rest (unless you want me
to))


https://codereview.chromium.org/448303002/diff/80001/mojo/
services/public/cpp/surfaces/tests/surface_unittest.cc
File mojo/services/public/cpp/surfaces/tests/surface_unittest.cc
(right):

https://codereview.chromium.org/448303002/diff/80001/mojo/
services/public/cpp/surfaces/tests/surface_unittest.cc#newcode74
mojo/services/public/cpp/surfaces/tests/surface_unittest.cc:74: QuadPtr
mojo_quad = Quad::From(*base_ptr);
actually, a better way to do this is just one line:

QuadPtr mojo_quad = Quad::From<cc::DrawQuad>(color_quad);

etc.

https://codereview.chromium.org/448303002/

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

[weiliangc, 2014-09-23 16:38:42]

Rebased to committed ListContianer. PTAL

[danakj, 2014-09-23 17:59:38]

https://codereview.chromium.org/448303002/diff/120001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (right):

https://codereview.chromium.org/448303002/diff/120001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl_unittest.cc:479:
EXPECT_TRANSFORMATION_MATRIX_EQ(gfx::Transform(), iter->quadTransform());
WDYT of adding ElementAt(size_t pos) to get at an element at an arbitrary
position in the structure so we don't have to iterate here? I think that'd help
a lot with readability of these test changes.

https://codereview.chromium.org/448303002/diff/120001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/448303002/diff/120001/cc/output/direct_render...
cc/output/direct_renderer.cc:385: DoDrawQuad(frame, &*it);
nit: &quad?

https://codereview.chromium.org/448303002/diff/120001/cc/quads/list_container.h
File cc/quads/list_container.h (right):

https://codereview.chromium.org/448303002/diff/120001/cc/quads/list_container...
cc/quads/list_container.h:158: DerivedElementType* AllocateAndCopyConstruct(
nit: AllocateAndCopyFrom?

https://codereview.chromium.org/448303002/diff/120001/content/common/cc_messa...
File content/common/cc_messages_unittest.cc (right):

https://codereview.chromium.org/448303002/diff/120001/content/common/cc_messa...
content/common/cc_messages_unittest.cc:494: if (in_iter_next !=
pass_in->quad_list.end()) {
Can you do this without managing 2 sets of 2 iterators? An at() method might
help here, or prev_iter = it;--prev_iter inside the loop or something.

https://codereview.chromium.org/448303002/diff/120001/mojo/services/public/cp...
File mojo/services/public/cpp/surfaces/tests/surface_unittest.cc (right):

https://codereview.chromium.org/448303002/diff/120001/mojo/services/public/cp...
mojo/services/public/cpp/surfaces/tests/surface_unittest.cc:74: QuadPtr
mojo_quad = Quad::From(*base_ptr);
james previously said you can do something like:

QuadPtr mojo_quad = Quad::From<cc::DrawQuad>(color_quad);

[kenrb, 2014-09-23 19:29:08]

ipc lgtm

[weiliangc, 2014-09-24 20:14:35]

https://codereview.chromium.org/448303002/diff/120001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (right):

https://codereview.chromium.org/448303002/diff/120001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl_unittest.cc:479:
EXPECT_TRANSFORMATION_MATRIX_EQ(gfx::Transform(), iter->quadTransform());
On 2014/09/23 17:59:38, danakj wrote:
> WDYT of adding ElementAt(size_t pos) to get at an element at an arbitrary
> position in the structure so we don't have to iterate here? I think that'd
help
> a lot with readability of these test changes.

Add at(index) function as well as unittest.

https://codereview.chromium.org/448303002/diff/120001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/448303002/diff/120001/cc/output/direct_render...
cc/output/direct_renderer.cc:385: DoDrawQuad(frame, &*it);
On 2014/09/23 17:59:38, danakj wrote:
> nit: &quad?

Done.

https://codereview.chromium.org/448303002/diff/120001/cc/quads/list_container.h
File cc/quads/list_container.h (right):

https://codereview.chromium.org/448303002/diff/120001/cc/quads/list_container...
cc/quads/list_container.h:158: DerivedElementType* AllocateAndCopyConstruct(
On 2014/09/23 17:59:38, danakj wrote:
> nit: AllocateAndCopyFrom?

Done.

https://codereview.chromium.org/448303002/diff/120001/content/common/cc_messa...
File content/common/cc_messages_unittest.cc (right):

https://codereview.chromium.org/448303002/diff/120001/content/common/cc_messa...
content/common/cc_messages_unittest.cc:494: if (in_iter_next !=
pass_in->quad_list.end()) {
On 2014/09/23 17:59:38, danakj wrote:
> Can you do this without managing 2 sets of 2 iterators? An at() method might
> help here, or prev_iter = it;--prev_iter inside the loop or something.

Done with using at().

I was thinking every time at() is called it needs to figure out which InnerList
it could be on and seems less efficient; however, I suppose it doesn't really
matter in unittest.

https://codereview.chromium.org/448303002/diff/120001/mojo/services/public/cp...
File mojo/services/public/cpp/surfaces/tests/surface_unittest.cc (right):

https://codereview.chromium.org/448303002/diff/120001/mojo/services/public/cp...
mojo/services/public/cpp/surfaces/tests/surface_unittest.cc:74: QuadPtr
mojo_quad = Quad::From(*base_ptr);
On 2014/09/23 17:59:38, danakj wrote:
> james previously said you can do something like:
> 
> QuadPtr mojo_quad = Quad::From<cc::DrawQuad>(color_quad);

Sorry, my bad.

Done.

[danakj, 2014-09-24 20:17:25]

https://codereview.chromium.org/448303002/diff/180001/cc/quads/list_container.h
File cc/quads/list_container.h (right):

https://codereview.chromium.org/448303002/diff/180001/cc/quads/list_container...
cc/quads/list_container.h:149: BaseElementType* at(size_t index);
This is an O(1) thing on std::vector, which is where you're getting this name
from, but it's not O(1) here so I think it's misleading to copy the name. That's
why I suggested ElementAt()

[weiliangc, 2014-09-24 20:26:31]

https://codereview.chromium.org/448303002/diff/180001/cc/quads/list_container.h
File cc/quads/list_container.h (right):

https://codereview.chromium.org/448303002/diff/180001/cc/quads/list_container...
cc/quads/list_container.h:149: BaseElementType* at(size_t index);
On 2014/09/24 20:17:25, danakj wrote:
> This is an O(1) thing on std::vector, which is where you're getting this name
> from, but it's not O(1) here so I think it's misleading to copy the name.
That's
> why I suggested ElementAt()

Make sense. Done.

[danakj, 2014-09-24 23:55:57]

LGTM w/ test quibbles

https://codereview.chromium.org/448303002/diff/200001/cc/quads/list_container...
File cc/quads/list_container_unittest.cc (right):

https://codereview.chromium.org/448303002/diff/200001/cc/quads/list_container...
cc/quads/list_container_unittest.cc:485: TEST(ListContainerTest,
SimpleIterationWithIndexSimpleDrawQuad) {
This test seems to be testing iterators, which are tested elsewhere and don't
iteract with ElementAt(), so I'm not sure why it was added, or at the least, I
think it should be more focused on the new thing it's trying to test.

https://codereview.chromium.org/448303002/diff/200001/cc/quads/list_container...
cc/quads/list_container_unittest.cc:548:
std::vector<SimpleDrawQuad*>::const_iterator dq_iter = dq_list.begin();
similar comment here for this test. i like that you're testing ElementAt across
more than 1 inner list, but this test ends up testing a lot of other things,.

[weiliangc, 2014-09-25 15:06:38]

On 2014/09/24 23:55:57, danakj wrote:
> LGTM w/ test quibbles
> 
>
https://codereview.chromium.org/448303002/diff/200001/cc/quads/list_container...
> File cc/quads/list_container_unittest.cc (right):
> 
>
https://codereview.chromium.org/448303002/diff/200001/cc/quads/list_container...
> cc/quads/list_container_unittest.cc:485: TEST(ListContainerTest,
> SimpleIterationWithIndexSimpleDrawQuad) {
> This test seems to be testing iterators, which are tested elsewhere and don't
> iteract with ElementAt(), so I'm not sure why it was added, or at the least, I
> think it should be more focused on the new thing it's trying to test.
> 
>
https://codereview.chromium.org/448303002/diff/200001/cc/quads/list_container...
> cc/quads/list_container_unittest.cc:548:
> std::vector<SimpleDrawQuad*>::const_iterator dq_iter = dq_list.begin();
> similar comment here for this test. i like that you're testing ElementAt
across
> more than 1 inner list, but this test ends up testing a lot of other things,.

Changed to just test setter/getter with ElementAt.

[weiliangc, 2014-09-25 15:06:46]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-09-25 15:07:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/448303002/220001

[danakj, 2014-09-25 15:14:31]

LGTM

[commit-bot: I haz the power, 2014-09-25 15:30:19]

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

[commit-bot: I haz the power, 2014-09-25 15:30:20]

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

[danakj, 2014-09-25 15:43:05]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-09-25 15:43:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/448303002/220001

[commit-bot: I haz the power, 2014-09-25 16:06:42]

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

[commit-bot: I haz the power, 2014-09-25 16:06:43]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[weiliangc, 2014-09-25 17:42:11]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-09-25 17:43:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/448303002/240001

[commit-bot: I haz the power, 2014-09-25 18:07:45]

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

[commit-bot: I haz the power, 2014-09-25 18:07:46]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[weiliangc, 2014-09-25 18:51:52]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-09-25 18:53:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/448303002/240001

[commit-bot: I haz the power, 2014-09-25 19:22:14]

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

[commit-bot: I haz the power, 2014-09-25 19:22:15]

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

[weiliangc, 2014-09-25 20:39:55]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-09-25 20:41:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/448303002/260001

[commit-bot: I haz the power, 2014-09-25 21:09:16]

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

[commit-bot: I haz the power, 2014-09-25 21:09:17]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[weiliangc, 2014-09-25 23:40:39]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-09-25 23:42:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/448303002/280001

[commit-bot: I haz the power, 2014-09-26 01:55:09]

Committed patchset #15 (id:280001) as 6a4640bacb7e3275a89525771d0aa8be80919eb6

[commit-bot: I haz the power, 2014-09-26 01:55:43]

Patchset 15 (id:??) landed as
https://crrev.com/032e929d161f8e466a55913deb748f0975c1f7de
Cr-Commit-Position: refs/heads/master@{#296855}