cc: Move LayerEvictionTileIterator to a separate file and make it a queue

cc: Move LayerEvictionTileIterator to a separate file and make it a queue

In line with LayerRasterTileIterator patch, this patch removes
PictureLayerImpl::LayerEvictionTileIterator, moves it to a separate
file, renames it to TilingSetEvictionQueue, makes its functionality be
more queue-like instead of iterator-like and merges
PictureLayerTiling::TilingEvictionTileIterator into it (as those two
iterators were so tightly coupled).

Also, this patch adds a PLI::CreateEvictionQueue to get a eviction queue
for the layer.

This hides a bit more details of PLTS from PLI and avoid duplicating
eviction category handling in multiple different iterator classes.

Committed: https://crrev.com/ada8f5dae0fd99ef5a74fecdff1b91dacd7183b3
Cr-Commit-Position: refs/heads/master@{#305972}

[USE eero AT chromium.org, 2014-11-21 15:22:46]

e.hakkinen@samsung.com changed reviewers:
+ enne@chromium.org, reveman@chromium.org, vmpstr@chromium.org

[USE eero AT chromium.org, 2014-11-21 15:22:46]

Here is a change for LayerEvictionTileIterator which is very similar to the
change for LayerRasterTileIterator at https://codereview.chromium.org/742343002/
.

This also merges TilingEvictionTileIterator functionality into the new
TilingSetEvictionQueue as those two depend heavily on eviction category and it
would make much more sense to put all eviction category handling into the new
TilingSetEvictionQueue alone. This should simplify reverse spiral difference
iterator based eviction as suggested at
https://codereview.chromium.org/644313002/ and
https://codereview.chromium.org/736753002/ .

So, PTAL after https://codereview.chromium.org/742343002/ .

[USE eero AT chromium.org, 2014-11-21 15:24:52]

I did not notice any performance degradation related to this patch.

[vmpstr, 2014-11-21 18:08:23]

Thanks for doing this! This looks good from my perspective, but I kinda feel
awkward reviewing this, since this is a parallel change to what I'm proposing in
https://codereview.chromium.org/742343002/

Basically, if reveman and enne are OK with my change, then they should be ok
with this change as well. FWIW, I support this.

https://codereview.chromium.org/741683003/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/741683003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:104: return nullptr;
I think this should return some sort of a "default empty queue" instead of null,
that way the calling code will always get an object.

[vmpstr, 2014-11-21 18:10:02]

On 2014/11/21 18:08:23, vmpstr wrote:
> Thanks for doing this! This looks good from my perspective, but I kinda feel
> awkward reviewing this, since this is a parallel change to what I'm proposing
in
> https://codereview.chromium.org/742343002/
> 
> Basically, if reveman and enne are OK with my change, then they should be ok
> with this change as well. FWIW, I support this.
> 
>
https://codereview.chromium.org/741683003/diff/1/cc/layers/picture_layer_impl.cc
> File cc/layers/picture_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/741683003/diff/1/cc/layers/picture_layer_impl...
> cc/layers/picture_layer_impl.cc:104: return nullptr;
> I think this should return some sort of a "default empty queue" instead of
null,
> that way the calling code will always get an object.

Can the tiling eviction tile iterator go away with this change (since it merges
it into the tiling set)

[enne (OOO), 2014-11-21 19:15:31]

In a general sense, this lgtm % vmpstr.

vmpstr, I think it's fine for you to review this.  I think independently being
on exactly the same page as somebody else says that it's a good change and not
that you should avoid reviewing it.  :)

[USE eero AT chromium.org, 2014-11-25 11:42:33]

On 2014/11/21 18:10:02, vmpstr wrote:
> Can the tiling eviction tile iterator go away with this change (since it
merges
> it into the tiling set)

Yes, they can. Should that be included into the same commit or be a separate
issue? What should be done for tiling eviction tile iterator related perf and
unit tests? Just removed, or what? As there are perf and unit tests for layer
eviction tile iterators (which are changed by this patch set to be perf and unit
tests for tiling set eviction queue), that could be the route.

On 2014/11/21 19:15:31, enne wrote:
> In a general sense, this lgtm % vmpstr.

Sorry, does that mean that I should wait for vmpstr's review?

[USE eero AT chromium.org, 2014-11-25 11:42:42]

https://codereview.chromium.org/741683003/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/741683003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:104: return nullptr;
On 2014/11/21 18:08:23, vmpstr wrote:
> I think this should return some sort of a "default empty queue" instead of
null,
> that way the calling code will always get an object.

The only calling code (apart from perf/unit tests) is
cc::EvictionTilePriorityQueue::PairedTilingSetQueue which have to deal with
nulls anyway as it does not create "default empty queues" for non-existing
active or pending twin layers anyway. So in this particular case, IMO, "default
empty queues" would not simplify anything.

If you disagree, I can change this.

[vmpstr, 2014-11-25 17:37:05]

Can you also remove PictureLayerTiling::TilingEvictionTileIterator while here? I
believe it's not being used anymore.

https://codereview.chromium.org/741683003/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/741683003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:104: return nullptr;
On 2014/11/25 11:42:42, e_hakkinen wrote:
> On 2014/11/21 18:08:23, vmpstr wrote:
> > I think this should return some sort of a "default empty queue" instead of
> null,
> > that way the calling code will always get an object.
> 
> The only calling code (apart from perf/unit tests) is
> cc::EvictionTilePriorityQueue::PairedTilingSetQueue which have to deal with
> nulls anyway as it does not create "default empty queues" for non-existing
> active or pending twin layers anyway. So in this particular case, IMO,
"default
> empty queues" would not simplify anything.
> 
> If you disagree, I can change this.

It simplifies the interface. When we call layer->CreateEvictionQueue(...); it
should return a queue. Typically, if we request something be created and it
returns a nullptr, it means that it wasn't possible to create it for whatever
reason (out of memory). 

In this case, that means if the queue that would be created is going to be
empty, we should still create it as an empty (default) queue.

https://codereview.chromium.org/741683003/diff/1/cc/resources/tiling_set_evic...
File cc/resources/tiling_set_eviction_queue.cc (right):

https://codereview.chromium.org/741683003/diff/1/cc/resources/tiling_set_evic...
cc/resources/tiling_set_eviction_queue.cc:153: Tile*
TilingSetEvictionQueue::PopEvictionTile() {
I think it would be a bit cleaner if we separate this into two functions,
something like "GetCurrentEvictionTile" from "AdvanceToNextEvictionTile"

Initially reading this code it's a bit confusing that current_tile_ !=
(*current_eviction_tiles_)[current_evcition_tiles_index_] and the latter
actually refers to the next tile.

https://codereview.chromium.org/741683003/diff/1/cc/resources/tiling_set_evic...
cc/resources/tiling_set_eviction_queue.cc:154: while
(current_eviction_tiles_index_ != current_eviction_tiles_->size()) {
nit: <

https://codereview.chromium.org/741683003/diff/1/cc/resources/tiling_set_evic...
File cc/resources/tiling_set_eviction_queue.h (right):

https://codereview.chromium.org/741683003/diff/1/cc/resources/tiling_set_evic...
cc/resources/tiling_set_eviction_queue.h:41: const std::vector<Tile*>*
current_eviction_tiles_;
Is it worth to make a struct here something like

struct EvictionData {
  const std::vector<Tile*>* tiles_;
  size_t index_;
  Tile* tile_;
} current_data_;

tile_ might even be:

Tile* tile() {
  if (tiles_ && index_ < tiles_->size())
    return (*tiles_)[index_];
  return nullptr;
}

(assuming that we point to the same tile as current)

Or maybe we should just rename the variables not to have current_* in front of
everything and leave a few comments to describe at a general level how this
queue will advance.

[USE eero AT chromium.org, 2014-11-26 09:17:19]

On 2014/11/25 17:37:05, vmpstr wrote:
> Can you also remove PictureLayerTiling::TilingEvictionTileIterator while here?
I
> believe it's not being used anymore.

Sure.

https://codereview.chromium.org/741683003/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/741683003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:104: return nullptr;
On 2014/11/25 17:37:04, vmpstr wrote:
> On 2014/11/25 11:42:42, e_hakkinen wrote:
> > On 2014/11/21 18:08:23, vmpstr wrote:
> > > I think this should return some sort of a "default empty queue" instead of
> > null,
> > > that way the calling code will always get an object.
> > 
> > The only calling code (apart from perf/unit tests) is
> > cc::EvictionTilePriorityQueue::PairedTilingSetQueue which have to deal with
> > nulls anyway as it does not create "default empty queues" for non-existing
> > active or pending twin layers anyway. So in this particular case, IMO,
> "default
> > empty queues" would not simplify anything.
> > 
> > If you disagree, I can change this.
> 
> It simplifies the interface. When we call layer->CreateEvictionQueue(...); it
> should return a queue. Typically, if we request something be created and it
> returns a nullptr, it means that it wasn't possible to create it for whatever
> reason (out of memory). 
> 
> In this case, that means if the queue that would be created is going to be
> empty, we should still create it as an empty (default) queue. 

Acknowledged.

[USE eero AT chromium.org, 2014-11-26 14:20:46]

https://codereview.chromium.org/741683003/diff/1/cc/resources/tiling_set_evic...
File cc/resources/tiling_set_eviction_queue.cc (right):

https://codereview.chromium.org/741683003/diff/1/cc/resources/tiling_set_evic...
cc/resources/tiling_set_eviction_queue.cc:153: Tile*
TilingSetEvictionQueue::PopEvictionTile() {
On 2014/11/25 17:37:05, vmpstr wrote:
> I think it would be a bit cleaner if we separate this into two functions,
> something like "GetCurrentEvictionTile" from "AdvanceToNextEvictionTile"

I replaced PopEvictionTile with AdvanceToNextEvictionTile.

> Initially reading this code it's a bit confusing that current_tile_ !=
> (*current_eviction_tiles_)[current_evcition_tiles_index_] and the latter
> actually refers to the next tile.

I replaced current_evcition_tiles_index_ with next_evcition_tile_index_. Should
be more cleaner now.

https://codereview.chromium.org/741683003/diff/1/cc/resources/tiling_set_evic...
cc/resources/tiling_set_eviction_queue.cc:154: while
(current_eviction_tiles_index_ != current_eviction_tiles_->size()) {
On 2014/11/25 17:37:04, vmpstr wrote:
> nit: <

Done.

https://codereview.chromium.org/741683003/diff/1/cc/resources/tiling_set_evic...
File cc/resources/tiling_set_eviction_queue.h (right):

https://codereview.chromium.org/741683003/diff/1/cc/resources/tiling_set_evic...
cc/resources/tiling_set_eviction_queue.h:41: const std::vector<Tile*>*
current_eviction_tiles_;
On 2014/11/25 17:37:05, vmpstr wrote:
> Or maybe we should just rename the variables not to have current_* in front of
> everything and leave a few comments to describe at a general level how this
> queue will advance.

I took this route.

[USE eero AT chromium.org, 2014-11-26 14:29:57]

PTAL.

[vmpstr, 2014-11-26 17:20:31]

lgtm

[USE eero AT chromium.org, 2014-11-27 08:49:34]

The CQ bit was checked by e.hakkinen@samsung.com

[commit-bot: I haz the power, 2014-11-27 08:49:58]

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

[commit-bot: I haz the power, 2014-11-27 09:41:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-27 09:42:34]

Patchset 3 (id:??) landed as
https://crrev.com/ada8f5dae0fd99ef5a74fecdff1b91dacd7183b3
Cr-Commit-Position: refs/heads/master@{#305972}