Added perftests for layer sorting

Added perftests for layer sorting

Added LayerSorter::Sort perftests and two JSON tree dumps to be loaded
by the new perftests

[Ian Vollick, 2014-05-30 17:32:36]

https://codereview.chromium.org/305063002/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/305063002/diff/1/cc/layers/layer_impl.cc#newc...
cc/layers/layer_impl.cc:1486: state->Set("transform",
MathUtil::AsValue(transform()).release());
Some of the values you're serializing here are just for debugging purposes?

https://codereview.chromium.org/305063002/diff/1/cc/layers/layer_impl.cc#newc...
cc/layers/layer_impl.cc:1488: if (!draw_transform().IsIdentity())
Hmm. This is an output of CalcDrawProps, no? Is it necessary to serialize this,
or will it be regenerated the first time we run CDP?

https://codereview.chromium.org/305063002/diff/1/cc/test/layer_tree_json_pars...
File cc/test/layer_tree_json_parser.cc (right):

https://codereview.chromium.org/305063002/diff/1/cc/test/layer_tree_json_pars...
cc/test/layer_tree_json_parser.cc:89: new_layer->SetAnchorPoint(gfx::Point());
Should this anchor point not come from the parsed value?

https://codereview.chromium.org/305063002/diff/1/cc/test/layer_tree_json_pars...
cc/test/layer_tree_json_parser.cc:151: success &= dict->GetList("DrawTransform",
&list);
Hmm, this seems fishy. You serialize DrawTransform, and it looks like you stuff
it into "transform" here. Should the line above be dict->GetList("Transform")?
(Please correct me if I'm missing something).

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
File cc/trees/layer_tree_host_common_perftest.cc (right):

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common_perftest.cc:177: host_impl);
If I understand this test correctly, you use CalcDrawProps to get the right
transforms baked into the layers, recursively collect them from the tree and
then you stress ::Sort.

nit: I'm ok with this, but if it's not too much trouble, perhaps you could just
serialize the layers w/ all the baked transforms and have all the pertinent
layers be direct children of the root. Then you could just sort children
repeatedly. This test doesn't seem to be representative of the real sorting that
would be done in a tree, anyhow: you're simultaneously sorting all the layers
that participate in _any_ 3D rendering context, but CalcDrawProps will sort the
contexts separately. Not that this is a big deal.

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common_perftest.cc:180: std::cout << "Number of layers:
" << base_list.size() << std::endl;
This is debugging code?

[Troy Hildebrandt, 2014-05-30 20:47:20]

https://codereview.chromium.org/305063002/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/305063002/diff/1/cc/layers/layer_impl.cc#newc...
cc/layers/layer_impl.cc:1486: state->Set("transform",
MathUtil::AsValue(transform()).release());
On 2014/05/30 17:32:36, Ian Vollick wrote:
> Some of the values you're serializing here are just for debugging purposes?

Not sure what version of the code this came from, but it's been removed.

https://codereview.chromium.org/305063002/diff/1/cc/layers/layer_impl.cc#newc...
cc/layers/layer_impl.cc:1488: if (!draw_transform().IsIdentity())
On 2014/05/30 17:32:36, Ian Vollick wrote:
> Hmm. This is an output of CalcDrawProps, no? Is it necessary to serialize
this,
> or will it be regenerated the first time we run CDP?

Same as above.

https://codereview.chromium.org/305063002/diff/1/cc/test/layer_tree_json_pars...
File cc/test/layer_tree_json_parser.cc (right):

https://codereview.chromium.org/305063002/diff/1/cc/test/layer_tree_json_pars...
cc/test/layer_tree_json_parser.cc:89: new_layer->SetAnchorPoint(gfx::Point());
On 2014/05/30 17:32:36, Ian Vollick wrote:
> Should this anchor point not come from the parsed value?

This isn't something I've written, but I can look into it.

https://codereview.chromium.org/305063002/diff/1/cc/test/layer_tree_json_pars...
cc/test/layer_tree_json_parser.cc:151: success &= dict->GetList("DrawTransform",
&list);
On 2014/05/30 17:32:36, Ian Vollick wrote:
> Hmm, this seems fishy. You serialize DrawTransform, and it looks like you
stuff
> it into "transform" here. Should the line above be dict->GetList("Transform")?
> (Please correct me if I'm missing something).

Same as above.

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
File cc/trees/layer_tree_host_common_perftest.cc (right):

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common_perftest.cc:177: host_impl);
On 2014/05/30 17:32:36, Ian Vollick wrote:
> If I understand this test correctly, you use CalcDrawProps to get the right
> transforms baked into the layers, recursively collect them from the tree and
> then you stress ::Sort.
> 
> nit: I'm ok with this, but if it's not too much trouble, perhaps you could
just
> serialize the layers w/ all the baked transforms and have all the pertinent
> layers be direct children of the root. Then you could just sort children
> repeatedly. This test doesn't seem to be representative of the real sorting
that
> would be done in a tree, anyhow: you're simultaneously sorting all the layers
> that participate in _any_ 3D rendering context, but CalcDrawProps will sort
the
> contexts separately. Not that this is a big deal.

You're correct. As far as walking the tree, I thought it would be more flexible
and would allow us to test sorting of separate contexts if necessary. Since
we're only testing the performance of the layer sorter, I imagine a single
context will still provide useful performance feedback.

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common_perftest.cc:180: std::cout << "Number of layers:
" << base_list.size() << std::endl;
On 2014/05/30 17:32:36, Ian Vollick wrote:
> This is debugging code?

Removed.

[Ian Vollick, 2014-05-31 01:52:00]

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
File cc/trees/layer_tree_host_common_perftest.cc (right):

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common_perftest.cc:177: host_impl);
On 2014/05/30 20:47:21, Troy Hildebrandt wrote:
> On 2014/05/30 17:32:36, Ian Vollick wrote:
> > If I understand this test correctly, you use CalcDrawProps to get the right
> > transforms baked into the layers, recursively collect them from the tree and
> > then you stress ::Sort.
> > 
> > nit: I'm ok with this, but if it's not too much trouble, perhaps you could
> just
> > serialize the layers w/ all the baked transforms and have all the pertinent
> > layers be direct children of the root. Then you could just sort children
> > repeatedly. This test doesn't seem to be representative of the real sorting
> that
> > would be done in a tree, anyhow: you're simultaneously sorting all the
layers
> > that participate in _any_ 3D rendering context, but CalcDrawProps will sort
> the
> > contexts separately. Not that this is a big deal.
> 
> You're correct. As far as walking the tree, I thought it would be more
flexible
> and would allow us to test sorting of separate contexts if necessary. Since
> we're only testing the performance of the layer sorter, I imagine a single
> context will still provide useful performance feedback.

While it's true that you would get useful perf numbers if you combined them into
a single context, I don't think it'd be too awful to store a list per context.
Unless you think this would prevent you from getting useful perf numbers, I'd
prefer it if you did that (details in my comment for the list building code). I
could imagine, for example, that spinning up the layer sorter on many,
differently-sized lists could give a different performance profile than
repeatedly sorting the same sized list.

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

https://codereview.chromium.org/305063002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:186: test_list.clear();
For each list you sort (I think there should be one per 3d rendering context --
see below), I think you can just do this:

LayerImplList test_list = base_list;

to do a copy since it's really just a std::vector.

https://codereview.chromium.org/305063002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:203: if (layer == NULL)
Will this ever be the case? Correct me if I'm wrong, but I don't think we have
NULL entries in children.

https://codereview.chromium.org/305063002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:206: size_t sorted_3d =
sorted_3d_level + (layer->is_3d_sorted() ? 1 : 0);
Using a size_t here is a little wonky and, I think, a little broken. The value
appears to be non-decreasing, so once sorted_3d > 0, every descendant will get
added to the list.

If you want to retain the current behavior, I'd ditch that parameter altogether
and just do
if (layer->is_3d_sorted())
  list->push_back(layer)

But I think there's a better option that I'd prefer you go with.
 - Store a std::vector<LayerImplList> rendering_contexts_;
 - Expose the functions LayerIsInExisting3DRenderingContext and
IsRootLayerOfNewRenderingContext from layer_tree_host_common.cc
 - During your recursion, if you hit a layer that's a root of a new rendering
context append a list for the context.
 - If the layer is 3d sorted, add it to the most recent context.
 - Each "lap" above will be a sort of each 3d rendering context.

https://codereview.chromium.org/305063002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:207: if (sorted_3d) {
Nit: no braces around one liners. Here and elsewhere.

[enne (OOO), 2014-06-02 17:54:18]

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
File cc/trees/layer_tree_host_common_perftest.cc (right):

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common_perftest.cc:177: host_impl);
On 2014/05/31 01:52:01, Ian Vollick wrote:
> On 2014/05/30 20:47:21, Troy Hildebrandt wrote:
> > On 2014/05/30 17:32:36, Ian Vollick wrote:
> > > If I understand this test correctly, you use CalcDrawProps to get the
right
> > > transforms baked into the layers, recursively collect them from the tree
and
> > > then you stress ::Sort.
> > > 
> > > nit: I'm ok with this, but if it's not too much trouble, perhaps you could
> > just
> > > serialize the layers w/ all the baked transforms and have all the
pertinent
> > > layers be direct children of the root. Then you could just sort children
> > > repeatedly. This test doesn't seem to be representative of the real
sorting
> > that
> > > would be done in a tree, anyhow: you're simultaneously sorting all the
> layers
> > > that participate in _any_ 3D rendering context, but CalcDrawProps will
sort
> > the
> > > contexts separately. Not that this is a big deal.
> > 
> > You're correct. As far as walking the tree, I thought it would be more
> flexible
> > and would allow us to test sorting of separate contexts if necessary. Since
> > we're only testing the performance of the layer sorter, I imagine a single
> > context will still provide useful performance feedback.
> 
> While it's true that you would get useful perf numbers if you combined them
into
> a single context, I don't think it'd be too awful to store a list per context.
> Unless you think this would prevent you from getting useful perf numbers, I'd
> prefer it if you did that (details in my comment for the list building code).
I
> could imagine, for example, that spinning up the layer sorter on many,
> differently-sized lists could give a different performance profile than
> repeatedly sorting the same sized list.

I was the one that suggested only sorting a single list.  Sure, multiple lists
will give different profiles, but I think sorting one  list makes this more of a
unit test and easier to compare across tests, i.e. X layer sorting test vs Y
layer sorting test.

Ian, if this is not palatable to you, I will counter-suggest the idea of reading
in the JSON munging it into this format of one parent with lots of children in a
3d context, and then writing that out as the new data.  In other words,
transforming the input to be what this test is testing.

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

https://codereview.chromium.org/305063002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:207: if (sorted_3d) {
On 2014/05/31 01:52:01, Ian Vollick wrote:
> Nit: no braces around one liners. Here and elsewhere.

Nit nit: Braces on oneliners are not forbidden by Chrome style.  ;)

[Ian Vollick, 2014-06-02 20:19:30]

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
File cc/trees/layer_tree_host_common_perftest.cc (right):

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common_perftest.cc:177: host_impl);
On 2014/06/02 17:54:19, enne wrote:
> On 2014/05/31 01:52:01, Ian Vollick wrote:
> > On 2014/05/30 20:47:21, Troy Hildebrandt wrote:
> > > On 2014/05/30 17:32:36, Ian Vollick wrote:
> > > > If I understand this test correctly, you use CalcDrawProps to get the
> right
> > > > transforms baked into the layers, recursively collect them from the tree
> and
> > > > then you stress ::Sort.
> > > > 
> > > > nit: I'm ok with this, but if it's not too much trouble, perhaps you
could
> > > just
> > > > serialize the layers w/ all the baked transforms and have all the
> pertinent
> > > > layers be direct children of the root. Then you could just sort children
> > > > repeatedly. This test doesn't seem to be representative of the real
> sorting
> > > that
> > > > would be done in a tree, anyhow: you're simultaneously sorting all the
> > layers
> > > > that participate in _any_ 3D rendering context, but CalcDrawProps will
> sort
> > > the
> > > > contexts separately. Not that this is a big deal.
> > > 
> > > You're correct. As far as walking the tree, I thought it would be more
> > flexible
> > > and would allow us to test sorting of separate contexts if necessary.
Since
> > > we're only testing the performance of the layer sorter, I imagine a single
> > > context will still provide useful performance feedback.
> > 
> > While it's true that you would get useful perf numbers if you combined them
> into
> > a single context, I don't think it'd be too awful to store a list per
context.
> > Unless you think this would prevent you from getting useful perf numbers,
I'd
> > prefer it if you did that (details in my comment for the list building
code).
> I
> > could imagine, for example, that spinning up the layer sorter on many,
> > differently-sized lists could give a different performance profile than
> > repeatedly sorting the same sized list.
> 
> I was the one that suggested only sorting a single list.  Sure, multiple lists
> will give different profiles, but I think sorting one  list makes this more of
a
> unit test and easier to compare across tests, i.e. X layer sorting test vs Y
> layer sorting test.
> 
> Ian, if this is not palatable to you, I will counter-suggest the idea of
reading
> in the JSON munging it into this format of one parent with lots of children in
a
> 3d context, and then writing that out as the new data.  In other words,
> transforming the input to be what this test is testing.

I've got a weak preference for multiple lists, but I don't feel too strongly
about it. I'm ok with this. But maybe add a comment saying that we're fusing the
lists so that it's obvious it's not intending to match compositor behavior
exactly?

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

https://codereview.chromium.org/305063002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:207: if (sorted_3d) {
On 2014/06/02 17:54:19, enne wrote:
> On 2014/05/31 01:52:01, Ian Vollick wrote:
> > Nit: no braces around one liners. Here and elsewhere.
> 
> Nit nit: Braces on oneliners are not forbidden by Chrome style.  ;)

Wait, what?! That's awesome. I love braces around one-liners :)

[Troy Hildebrandt, 2014-06-02 22:08:30]

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
File cc/trees/layer_tree_host_common_perftest.cc (right):

https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common_perftest.cc:177: host_impl);
On 2014/06/02 20:19:30, Ian Vollick wrote:
> On 2014/06/02 17:54:19, enne wrote:
> > On 2014/05/31 01:52:01, Ian Vollick wrote:
> > > On 2014/05/30 20:47:21, Troy Hildebrandt wrote:
> > > > On 2014/05/30 17:32:36, Ian Vollick wrote:
> > > > > If I understand this test correctly, you use CalcDrawProps to get the
> > right
> > > > > transforms baked into the layers, recursively collect them from the
tree
> > and
> > > > > then you stress ::Sort.
> > > > > 
> > > > > nit: I'm ok with this, but if it's not too much trouble, perhaps you
> could
> > > > just
> > > > > serialize the layers w/ all the baked transforms and have all the
> > pertinent
> > > > > layers be direct children of the root. Then you could just sort
children
> > > > > repeatedly. This test doesn't seem to be representative of the real
> > sorting
> > > > that
> > > > > would be done in a tree, anyhow: you're simultaneously sorting all the
> > > layers
> > > > > that participate in _any_ 3D rendering context, but CalcDrawProps will
> > sort
> > > > the
> > > > > contexts separately. Not that this is a big deal.
> > > > 
> > > > You're correct. As far as walking the tree, I thought it would be more
> > > flexible
> > > > and would allow us to test sorting of separate contexts if necessary.
> Since
> > > > we're only testing the performance of the layer sorter, I imagine a
single
> > > > context will still provide useful performance feedback.
> > > 
> > > While it's true that you would get useful perf numbers if you combined
them
> > into
> > > a single context, I don't think it'd be too awful to store a list per
> context.
> > > Unless you think this would prevent you from getting useful perf numbers,
> I'd
> > > prefer it if you did that (details in my comment for the list building
> code).
> > I
> > > could imagine, for example, that spinning up the layer sorter on many,
> > > differently-sized lists could give a different performance profile than
> > > repeatedly sorting the same sized list.
> > 
> > I was the one that suggested only sorting a single list.  Sure, multiple
lists
> > will give different profiles, but I think sorting one  list makes this more
of
> a
> > unit test and easier to compare across tests, i.e. X layer sorting test vs Y
> > layer sorting test.
> > 
> > Ian, if this is not palatable to you, I will counter-suggest the idea of
> reading
> > in the JSON munging it into this format of one parent with lots of children
in
> a
> > 3d context, and then writing that out as the new data.  In other words,
> > transforming the input to be what this test is testing.
> 
> I've got a weak preference for multiple lists, but I don't feel too strongly
> about it. I'm ok with this. But maybe add a comment saying that we're fusing
the
> lists so that it's obvious it's not intending to match compositor behavior
> exactly?

Comment added to explain the different behaviour, layers are currently all put
into a single big context if is_3d_sorted() is true.

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

https://codereview.chromium.org/305063002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:186: test_list.clear();
On 2014/05/31 01:52:01, Ian Vollick wrote:
> For each list you sort (I think there should be one per 3d rendering context
--
> see below), I think you can just do this:
> 
> LayerImplList test_list = base_list;
> 
> to do a copy since it's really just a std::vector.

Done.

https://codereview.chromium.org/305063002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:203: if (layer == NULL)
On 2014/05/31 01:52:01, Ian Vollick wrote:
> Will this ever be the case? Correct me if I'm wrong, but I don't think we have
> NULL entries in children.

As far as I can tell, it shouldn't be. I removed this.

https://codereview.chromium.org/305063002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_perftest.cc:206: size_t sorted_3d =
sorted_3d_level + (layer->is_3d_sorted() ? 1 : 0);
On 2014/05/31 01:52:01, Ian Vollick wrote:
> Using a size_t here is a little wonky and, I think, a little broken. The value
> appears to be non-decreasing, so once sorted_3d > 0, every descendant will get
> added to the list.
> 
> If you want to retain the current behavior, I'd ditch that parameter
altogether
> and just do
> if (layer->is_3d_sorted())
>   list->push_back(layer)
> 
> But I think there's a better option that I'd prefer you go with.
>  - Store a std::vector<LayerImplList> rendering_contexts_;
>  - Expose the functions LayerIsInExisting3DRenderingContext and
> IsRootLayerOfNewRenderingContext from layer_tree_host_common.cc
>  - During your recursion, if you hit a layer that's a root of a new rendering
> context append a list for the context.
>  - If the layer is 3d sorted, add it to the most recent context.
>  - Each "lap" above will be a sort of each 3d rendering context.

Removed, using layer->is_3d_sorted() now.

[Ian Vollick, 2014-06-02 23:27:34]

On 2014/06/02 22:08:30, Troy Hildebrandt wrote:
>
https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
> File cc/trees/layer_tree_host_common_perftest.cc (right):
> 
>
https://codereview.chromium.org/305063002/diff/1/cc/trees/layer_tree_host_com...
> cc/trees/layer_tree_host_common_perftest.cc:177: host_impl);
> On 2014/06/02 20:19:30, Ian Vollick wrote:
> > On 2014/06/02 17:54:19, enne wrote:
> > > On 2014/05/31 01:52:01, Ian Vollick wrote:
> > > > On 2014/05/30 20:47:21, Troy Hildebrandt wrote:
> > > > > On 2014/05/30 17:32:36, Ian Vollick wrote:
> > > > > > If I understand this test correctly, you use CalcDrawProps to get
the
> > > right
> > > > > > transforms baked into the layers, recursively collect them from the
> tree
> > > and
> > > > > > then you stress ::Sort.
> > > > > > 
> > > > > > nit: I'm ok with this, but if it's not too much trouble, perhaps you
> > could
> > > > > just
> > > > > > serialize the layers w/ all the baked transforms and have all the
> > > pertinent
> > > > > > layers be direct children of the root. Then you could just sort
> children
> > > > > > repeatedly. This test doesn't seem to be representative of the real
> > > sorting
> > > > > that
> > > > > > would be done in a tree, anyhow: you're simultaneously sorting all
the
> > > > layers
> > > > > > that participate in _any_ 3D rendering context, but CalcDrawProps
will
> > > sort
> > > > > the
> > > > > > contexts separately. Not that this is a big deal.
> > > > > 
> > > > > You're correct. As far as walking the tree, I thought it would be more
> > > > flexible
> > > > > and would allow us to test sorting of separate contexts if necessary.
> > Since
> > > > > we're only testing the performance of the layer sorter, I imagine a
> single
> > > > > context will still provide useful performance feedback.
> > > > 
> > > > While it's true that you would get useful perf numbers if you combined
> them
> > > into
> > > > a single context, I don't think it'd be too awful to store a list per
> > context.
> > > > Unless you think this would prevent you from getting useful perf
numbers,
> > I'd
> > > > prefer it if you did that (details in my comment for the list building
> > code).
> > > I
> > > > could imagine, for example, that spinning up the layer sorter on many,
> > > > differently-sized lists could give a different performance profile than
> > > > repeatedly sorting the same sized list.
> > > 
> > > I was the one that suggested only sorting a single list.  Sure, multiple
> lists
> > > will give different profiles, but I think sorting one  list makes this
more
> of
> > a
> > > unit test and easier to compare across tests, i.e. X layer sorting test vs
Y
> > > layer sorting test.
> > > 
> > > Ian, if this is not palatable to you, I will counter-suggest the idea of
> > reading
> > > in the JSON munging it into this format of one parent with lots of
children
> in
> > a
> > > 3d context, and then writing that out as the new data.  In other words,
> > > transforming the input to be what this test is testing.
> > 
> > I've got a weak preference for multiple lists, but I don't feel too strongly
> > about it. I'm ok with this. But maybe add a comment saying that we're fusing
> the
> > lists so that it's obvious it's not intending to match compositor behavior
> > exactly?
> 
> Comment added to explain the different behaviour, layers are currently all put
> into a single big context if is_3d_sorted() is true.
> 
>
https://codereview.chromium.org/305063002/diff/20001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_common_perftest.cc (right):
> 
>
https://codereview.chromium.org/305063002/diff/20001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_common_perftest.cc:186: test_list.clear();
> On 2014/05/31 01:52:01, Ian Vollick wrote:
> > For each list you sort (I think there should be one per 3d rendering context
> --
> > see below), I think you can just do this:
> > 
> > LayerImplList test_list = base_list;
> > 
> > to do a copy since it's really just a std::vector.
> 
> Done.
> 
>
https://codereview.chromium.org/305063002/diff/20001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_common_perftest.cc:203: if (layer == NULL)
> On 2014/05/31 01:52:01, Ian Vollick wrote:
> > Will this ever be the case? Correct me if I'm wrong, but I don't think we
have
> > NULL entries in children.
> 
> As far as I can tell, it shouldn't be. I removed this.
> 
>
https://codereview.chromium.org/305063002/diff/20001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_common_perftest.cc:206: size_t sorted_3d =
> sorted_3d_level + (layer->is_3d_sorted() ? 1 : 0);
> On 2014/05/31 01:52:01, Ian Vollick wrote:
> > Using a size_t here is a little wonky and, I think, a little broken. The
value
> > appears to be non-decreasing, so once sorted_3d > 0, every descendant will
get
> > added to the list.
> > 
> > If you want to retain the current behavior, I'd ditch that parameter
> altogether
> > and just do
> > if (layer->is_3d_sorted())
> >   list->push_back(layer)
> > 
> > But I think there's a better option that I'd prefer you go with.
> >  - Store a std::vector<LayerImplList> rendering_contexts_;
> >  - Expose the functions LayerIsInExisting3DRenderingContext and
> > IsRootLayerOfNewRenderingContext from layer_tree_host_common.cc
> >  - During your recursion, if you hit a layer that's a root of a new
rendering
> > context append a list for the context.
> >  - If the layer is 3d sorted, add it to the most recent context.
> >  - Each "lap" above will be a sort of each 3d rendering context.
> 
> Removed, using layer->is_3d_sorted() now.

lgtm

[Ian Vollick, 2014-06-02 23:27:38]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-06-02 23:29:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thildebrCG@gmail.com/305063002/40001

[commit-bot: I haz the power, 2014-06-03 03:20:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-03 03:24:31]

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

[commit-bot: I haz the power, 2014-06-03 03:24:31]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)