Issue 367833003: cc: Start using raster/eviction iterators.

vmpstr

Please take a look. I've tried my best to annotate the changes I've made, but ...

6 years, 5 months ago (2014-07-02 23:51:29 UTC) #1

enne (OOO)

https://codereview.chromium.org/367833003/diff/80001/cc/layers/heads_up_display_layer_impl.cc File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/367833003/diff/80001/cc/layers/heads_up_display_layer_impl.cc#newcode1 cc/layers/heads_up_display_layer_impl.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. ...

6 years, 5 months ago (2014-07-07 19:41:59 UTC) #2

reveman

https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile.h File cc/resources/tile.h (right): https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile.h#newcode64 cc/resources/tile.h:64: void UnsetRequiredForActivation(); I think these functions should be replaced ...

6 years, 5 months ago (2014-07-07 20:48:13 UTC) #3

vmpstr

PTAL https://codereview.chromium.org/367833003/diff/80001/cc/layers/heads_up_display_layer_impl.cc File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/367833003/diff/80001/cc/layers/heads_up_display_layer_impl.cc#newcode1 cc/layers/heads_up_display_layer_impl.cc:1: // Copyright 2012 The Chromium Authors. All rights ...

6 years, 5 months ago (2014-07-07 23:24:48 UTC) #4

PTAL

https://codereview.chromium.org/367833003/diff/80001/cc/layers/heads_up_displ...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/367833003/diff/80001/cc/layers/heads_up_displ...
cc/layers/heads_up_display_layer_impl.cc:1: // Copyright 2012 The Chromium
Authors. All rights reserved.
On 2014/07/07 19:41:59, enne wrote:
> On 2014/07/02 23:51:29, vmpstr wrote:
> > Changes in this file are a part of the original patch.
> 
> I'm not sure what you mean by that?

I just mean that this is a part of the original pre-optimizations patch that was
reverted. I'm just trying to save reviewer time, but feel free to review of
course :P

https://codereview.chromium.org/367833003/diff/80001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/367833003/diff/80001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:484: // Compute occluded indecies.
On 2014/07/07 19:41:59, enne wrote:
> I'm not buying the occlusion_set_.  Why can't UpdateTileOcclusion do this
work?

From my understanding of occlusion, we can only really get it from
occlusion_tracker during a front to back walk of the tree. UpdateTilePriorities
happens to be in that walk. However, when we UpdateTileOcclusion, we're not in a
tree walk (and don't have an occlusion tracker for that matter).

https://codereview.chromium.org/367833003/diff/80001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:539: tile->MarkRequiredForActivation();
On 2014/07/07 19:41:59, enne wrote:
> Does it matter that this can mark active tree only tiles too? That seems a
> little sketchy to me.

IsTileRequiredForActivation has a tree check that returns false if this is not a
pending tree. To pair it with line 521, I could make this more explicit? ie

if (tree == PENDING_TREE && IsTileRequiredForActivation(tile))

wdyt?

https://codereview.chromium.org/367833003/diff/80001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:783: UpdateTileOcclusion(it->second);
On 2014/07/07 19:41:59, enne wrote:
> I feel like there are a lot of paired UpdateTileOcclusion/SetTilePriority.  I
> feel a bit like occlusion is part of priority.  Is it possible to combine
these?
> I know there's one case where you update tile occlusion and then decide you
> don't need to raster so you don't update the tile priority, but I kind of feel
> like then you have a partially stale priority, which is odd.

It's possible... I just feel like priority calculation thing is a pretty
expensive operation. Also, current approach amounts to:

UpdateTileOcclusion(tile)
if (TileNeedsRaster(tile))
  SetTilePriority(tile);
  return tile for raster;

I feel like if it was a part of priority then it would be something like

SetTilePriority(tile)
if (TileNeedsRaster(tile))
  return tile for raster;

However, this means that we would update priorities for tiles that are already
done as well, which definitely seems wasteful.

The alternative is

if (TileDoesntNeedsRasterBecauseItHasResources(tile))
  skip
else
  SetTilePriority(tile)
  if (TileDoesntNeedRasterBecauseOfOcclusion(tile))
    skip
return tile for raster;

which only updates priorities if the tile wasn't done yet, but it also seems
more awkward (modulo function names)?

<off topic>
Maybe in general, a tile should be something like the following:

class Tile {
 // common data
 picture pile
 use picture analysis
 managed state (tile versions)
 content rect, content scale

 // per tree data
 TreeSpecificTileData tree_data_[NUM_TREES]
};

and TreeSpecificTileData be something like

class TreeSpecificTileData {
 tile priority
 occlusion
 required for activation
};

I'm just not sure it makes sense to clump some of these values into
TilePriority, since to me priority really is just means distance and bin, the
rest is kind of non-priority per-tree settings. Wdyt? Maybe occlusion is also a
priority thing... I'm just throwing ideas out there for now.

https://codereview.chromium.org/367833003/diff/80001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:784: SetTilePriority(it->second, true);
On 2014/07/07 19:41:59, enne wrote:
> The boolean parameter is hard to read.  I think callsites would be more
readable
> if you had SetTilePriority and SetTileAndTwinPriority, even if it did the same
> thing underneath.

Done.

https://codereview.chromium.org/367833003/diff/80001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:823:
tiling_->UpdateTileOcclusion(current_tile_);
On 2014/07/07 19:41:59, enne wrote:
> Does this need to get wrapped by !current_tile_ too?

Whoops.

https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:1153: if (!initialized_)
On 2014/07/07 19:41:59, enne wrote:
> Is this to avoid doing too much work in the constructor?

Yeah, this also ensures that we don't do any hard work if we remain within
memory budget.

https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:1170: Initialize();
On 2014/07/07 20:48:13, reveman wrote:
> This feels like the wrong approach.

I buy that there are cases where this is unnecessary in that we will breach the
memory limit and initialize the iterator. At the same time though, there are
tons of examples where we always stay within the memory limit (note that we
still "evict" tiles by clipping all live tiles to the interest rect in the
tiling, so if all tiles fit within the interest rect budget, then we never
initialize the eviction iterator). This provides a substantial savings in terms
of time on "regular" pages, whatever that may mean. Without this approach, there
are only one option that I can think of and that is making eviction at the
tiling level super cheap. Without one of those two things, the iterators will
simply be too slow.

Maybe a prioritized tile set for tiling eviction? :P

> 
> Is the general problem that we're trying to implement iterators using a heap?
> Are these iterators supposed to be priority queues instead? Implementing a
> priority queue using a heap makes sense and that would allow you to only
> re-build the heap when absolutely necessary. ie. when the top priority tile of
a
> layer has changed.

The way that I use a heap here is really a priority queue. After making the
initial heap, I pop the top element, modify it (by consuming a tile), and
reinsert it back if it still has tiles. I think that's equivalent to priority
queue's top, pop, and push, no? Note that I think I can just change it to be the
actual std::priority_queue adapter if that would make it more readable?

vmpstr

The numbers on the current patch are kind of on par with tip of tree ...

6 years, 5 months ago (2014-07-07 23:29:29 UTC) #5

reveman

https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manager.cc#newcode1170 cc/resources/tile_manager.cc:1170: Initialize(); On 2014/07/07 23:24:47, vmpstr wrote: > On 2014/07/07 ...

6 years, 5 months ago (2014-07-08 00:13:12 UTC) #6

https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:1170: Initialize();
On 2014/07/07 23:24:47, vmpstr wrote:
> On 2014/07/07 20:48:13, reveman wrote:
> > This feels like the wrong approach.
> 
> I buy that there are cases where this is unnecessary in that we will breach
the
> memory limit and initialize the iterator. At the same time though, there are
> tons of examples where we always stay within the memory limit (note that we
> still "evict" tiles by clipping all live tiles to the interest rect in the
> tiling, so if all tiles fit within the interest rect budget, then we never
> initialize the eviction iterator). This provides a substantial savings in
terms
> of time on "regular" pages, whatever that may mean. Without this approach,
there
> are only one option that I can think of and that is making eviction at the
> tiling level super cheap. Without one of those two things, the iterators will
> simply be too slow.
> 
> Maybe a prioritized tile set for tiling eviction? :P

I don't understand this comment. Is this still relevant if we go with the
priority queue approach discussed below?
 
> 
> > 
> > Is the general problem that we're trying to implement iterators using a
heap?
> > Are these iterators supposed to be priority queues instead? Implementing a
> > priority queue using a heap makes sense and that would allow you to only
> > re-build the heap when absolutely necessary. ie. when the top priority tile
of
> a
> > layer has changed.
> 
> The way that I use a heap here is really a priority queue. After making the
> initial heap, I pop the top element, modify it (by consuming a tile), and
> reinsert it back if it still has tiles. I think that's equivalent to priority
> queue's top, pop, and push, no? Note that I think I can just change it to be
the
> actual std::priority_queue adapter if that would make it more readable?

Yes, it looks like the interface used for raster/eviction should be a priority
queue. And a heap is used to implement this interface. Maybe better to use your
own priority queue interface as "Push" doesn't have to be exposed to the
raster/eviction code.

vmpstr

On 2014/07/08 00:13:12, reveman wrote: > https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manager.cc > File cc/resources/tile_manager.cc (right): > > https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manager.cc#newcode1170 > ...

6 years, 5 months ago (2014-07-08 00:31:58 UTC) #7

On 2014/07/08 00:13:12, reveman wrote:
>
https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manage...
> File cc/resources/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manage...
> cc/resources/tile_manager.cc:1170: Initialize();
> On 2014/07/07 23:24:47, vmpstr wrote:
> > On 2014/07/07 20:48:13, reveman wrote:
> > > This feels like the wrong approach.
> > 
> > I buy that there are cases where this is unnecessary in that we will breach
> the
> > memory limit and initialize the iterator. At the same time though, there are
> > tons of examples where we always stay within the memory limit (note that we
> > still "evict" tiles by clipping all live tiles to the interest rect in the
> > tiling, so if all tiles fit within the interest rect budget, then we never
> > initialize the eviction iterator). This provides a substantial savings in
> terms
> > of time on "regular" pages, whatever that may mean. Without this approach,
> there
> > are only one option that I can think of and that is making eviction at the
> > tiling level super cheap. Without one of those two things, the iterators
will
> > simply be too slow.
> > 
> > Maybe a prioritized tile set for tiling eviction? :P
> 
> I don't understand this comment. Is this still relevant if we go with the
> priority queue approach discussed below?
>  

Yeah, I think it is. Here's how the code is layed out (for eviction):

TileManager::EvictionTileIterator
 - has a heap of PictureLayerImpl::LayerEvictionTileIterator pairs (essentially
a priority queue)
   - has a list of PictureLayerTiling::TilingEvictionTileIterators (simple
vector in some predetermined order of tiling scales)
     - has a sorted list of tiles (vector, ordered by priority lowest ->
highest)

Getting one tile from eviction iterator means getting the next layer pair from
the heap (lowest priority in the eviction case), which means getting the next
list from the layer eviction tile iterator and then getting the next tile out of
the sorted list of tiles.

The slow part of all of this is getting a sorted list of tiles in the first
place. Constructing this iterator means constructing the layer list and getting
a sorted list of layer tiles with all priorities updated. That's the slow part.
With the lazy approach, we only ever construct this when we need at least one
tile. Without the lazy approach, we would always construct this.

We can't really push this laziness into layers/tilings, because the heap itself
will need to know the next priority of each layer, which means it will do an
access for the next tile.

> > 
> > > 
> > > Is the general problem that we're trying to implement iterators using a
> heap?
> > > Are these iterators supposed to be priority queues instead? Implementing a
> > > priority queue using a heap makes sense and that would allow you to only
> > > re-build the heap when absolutely necessary. ie. when the top priority
tile
> of
> > a
> > > layer has changed.
> > 
> > The way that I use a heap here is really a priority queue. After making the
> > initial heap, I pop the top element, modify it (by consuming a tile), and
> > reinsert it back if it still has tiles. I think that's equivalent to
priority
> > queue's top, pop, and push, no? Note that I think I can just change it to be
> the
> > actual std::priority_queue adapter if that would make it more readable?
> 
> Yes, it looks like the interface used for raster/eviction should be a priority
> queue. And a heap is used to implement this interface. Maybe better to use
your
> own priority queue interface as "Push" doesn't have to be exposed to the
> raster/eviction code.

Ok.. If I understand correctly, then you're saying something like this:

  if (paired_iterator->PeekTile(tree_priority_) != NULL) {
    iterator_heap_.push_back(paired_iterator);
    std::push_heap(iterator_heap_.begin(), iterator_heap_.end(), comparator_);
  }

should become something like this:

  if (paired_iterator->PeekTile(tree_priority_) != NULL)
    iterator_queue_.push(paired_iterator);

with iterator_queue_ being a std::priority_queue? That approach is slightly more
complicated as comparator_ is a type that takes a constructor argument. Or do
you mean something else? Let's talk offline tomorrow, since I think I'm missing
your point.

reveman

On 2014/07/08 00:31:58, vmpstr wrote: > On 2014/07/08 00:13:12, reveman wrote: > > > https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manager.cc ...

6 years, 5 months ago (2014-07-08 02:20:19 UTC) #8

On 2014/07/08 00:31:58, vmpstr wrote:
> On 2014/07/08 00:13:12, reveman wrote:
> >
>
https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manage...
> > File cc/resources/tile_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/367833003/diff/80001/cc/resources/tile_manage...
> > cc/resources/tile_manager.cc:1170: Initialize();
> > On 2014/07/07 23:24:47, vmpstr wrote:
> > > On 2014/07/07 20:48:13, reveman wrote:
> > > > This feels like the wrong approach.
> > > 
> > > I buy that there are cases where this is unnecessary in that we will
breach
> > the
> > > memory limit and initialize the iterator. At the same time though, there
are
> > > tons of examples where we always stay within the memory limit (note that
we
> > > still "evict" tiles by clipping all live tiles to the interest rect in the
> > > tiling, so if all tiles fit within the interest rect budget, then we never
> > > initialize the eviction iterator). This provides a substantial savings in
> > terms
> > > of time on "regular" pages, whatever that may mean. Without this approach,
> > there
> > > are only one option that I can think of and that is making eviction at the
> > > tiling level super cheap. Without one of those two things, the iterators
> will
> > > simply be too slow.
> > > 
> > > Maybe a prioritized tile set for tiling eviction? :P
> > 
> > I don't understand this comment. Is this still relevant if we go with the
> > priority queue approach discussed below?
> >  
> 
> Yeah, I think it is. Here's how the code is layed out (for eviction):
> 
> TileManager::EvictionTileIterator
>  - has a heap of PictureLayerImpl::LayerEvictionTileIterator pairs
(essentially
> a priority queue)
>    - has a list of PictureLayerTiling::TilingEvictionTileIterators (simple
> vector in some predetermined order of tiling scales)
>      - has a sorted list of tiles (vector, ordered by priority lowest ->
> highest)
> 

I seems to me like we should instead have:

EvictionTilePriorityQueue
 - has a heap of PictureLayerImpl::LayerEvictionTilePriorityQueue (or
::LayerEvictionTileHeap if necessary)
  - has a list of PictureLayerTiling::TilingEvictionTileVector (could be a Heap
in the future if that make sense?)
   - has a sorted list of tiles

and the same thing for raster.

I think of iterators as something that is basically free to create/use and lazy
init/work doesn't make sense unless it's done on the underlying data type. While
a priority queue can be the interface to a complicated implementation where some
operations are expensive and a lot of work is done lazily to save time. I think
a lazy approach is right but not when using iterators unless you make the lazy
work happen on the underlying data type and affect all iterators that might be
wrapping that same object.

> 
> Getting one tile from eviction iterator means getting the next layer pair from
> the heap (lowest priority in the eviction case), which means getting the next
> list from the layer eviction tile iterator and then getting the next tile out
of
> the sorted list of tiles.
> 
> The slow part of all of this is getting a sorted list of tiles in the first
> place. Constructing this iterator means constructing the layer list and
getting
> a sorted list of layer tiles with all priorities updated. That's the slow
part.
> With the lazy approach, we only ever construct this when we need at least one
> tile. Without the lazy approach, we would always construct this.
> 
> We can't really push this laziness into layers/tilings, because the heap
itself
> will need to know the next priority of each layer, which means it will do an
> access for the next tile.

Yes, this is why I think the interface should be a priority queue and the
implementation can take care of this laziness.

> 
> > > 
> > > > 
> > > > Is the general problem that we're trying to implement iterators using a
> > heap?
> > > > Are these iterators supposed to be priority queues instead? Implementing
a
> > > > priority queue using a heap makes sense and that would allow you to only
> > > > re-build the heap when absolutely necessary. ie. when the top priority
> tile
> > of
> > > a
> > > > layer has changed.
> > > 
> > > The way that I use a heap here is really a priority queue. After making
the
> > > initial heap, I pop the top element, modify it (by consuming a tile), and
> > > reinsert it back if it still has tiles. I think that's equivalent to
> priority
> > > queue's top, pop, and push, no? Note that I think I can just change it to
be
> > the
> > > actual std::priority_queue adapter if that would make it more readable?
> > 
> > Yes, it looks like the interface used for raster/eviction should be a
priority
> > queue. And a heap is used to implement this interface. Maybe better to use
> your
> > own priority queue interface as "Push" doesn't have to be exposed to the
> > raster/eviction code.
> 
> Ok.. If I understand correctly, then you're saying something like this:
> 
>   if (paired_iterator->PeekTile(tree_priority_) != NULL) {
>     iterator_heap_.push_back(paired_iterator);
>     std::push_heap(iterator_heap_.begin(), iterator_heap_.end(), comparator_);
>   }
> 
> should become something like this:
> 
>   if (paired_iterator->PeekTile(tree_priority_) != NULL)
>     iterator_queue_.push(paired_iterator);
> 
> with iterator_queue_ being a std::priority_queue? That approach is slightly
more
> complicated as comparator_ is a type that takes a constructor argument. Or do
> you mean something else? Let's talk offline tomorrow, since I think I'm
missing
> your point.

Yes, let's chat tomorrow.

vmpstr

PTAL. Here's a summary of changes from the last reviewed patch set: - Moved tile ...

6 years, 5 months ago (2014-07-09 00:05:36 UTC) #9

vmpstr

PS tests are broken, or rather don't compile yet. David, I just want to see ...

6 years, 5 months ago (2014-07-09 00:10:29 UTC) #10

reveman

This is close to what I had in mind. I think that if we make ...

6 years, 5 months ago (2014-07-09 02:21:10 UTC) #11

This is close to what I had in mind. I think that if we make a few of the
adjustments I suggest in this review we'll have a clean interface that can be
optimized as much as needed in follow up patches.

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:20: #include "cc/layers/picture_layer_impl.h"
if possible, could you forward declare PictureLayerImpl as part of this patch
instead of the include?

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:35: virtual const std::vector<PictureLayerImpl*>&
GetPictureLayers() const = 0;
Note: this was added with the assumption that we'd use it for this patch. Now
that this is no longer the case, I think we should somehow merge it with
NotifyReadyToActivate function and not expose layers to the tile manager but
that can be done in a separate follow-up patch. Maybe just add a TODO here for
now.

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:48: virtual EvictionTileQueue*
GetEvictionQueue(TreePriority tree_priority) = 0;
Can we pass these queues to the ctor instead? If the tree priority changes,
maybe the priority queues given to the tile manager could just automatically
reflect that. Or we have one queue for each tree priority type but that seems a
bit overkill.

Btw, I don't think tree_priority is something that the tile manager should cache
and only update when ManageTiles is called anymore. That made sense when the
tile manager was in control over the raster order and could prevent it from
changing between calls to ManageTiles. So GlobalStateThatImpactsTilePriority
should probably be retrieved using a client function instead of being passed to
ManageTiles. We can change this in a follow-up patch if you prefer though.

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_prior...
File cc/resources/tile_priority_queue.h (right):

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_prior...
cc/resources/tile_priority_queue.h:27: class CC_EXPORT RasterTileQueue {
How about only declaring an abstract TilePriortyQueue class here that is as
simple as:

class TilePriortyQueue {
 public:
  virtual bool IsEmpty() = 0;
  virtual Tile* Top() = 0;
  virtual void Pop() = 0;

 protected:
  virtual ~TilePriortyQueue() {}
};

as that's really the only thing the tile manager needs. Raster and eviction are
just different implementations of this interface but that's not something the
tile manager needs to be aware of.

Tile{Raster|Eviction}Queue implementations of this interface can live in
different files or just in the LTHI .cc file.

https://codereview.chromium.org/367833003/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/367833003/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1258: raster_queue_.Prepare(paired_layers,
tree_priority);
I don't think we should use a client function to trigger work that needs to be
done before the queue can be used. I'd rather see us handle that transparently
to the tile manager by preparing the queue or marking it as needing to be
prepared before use as needed.

I don't care how much you optimize this in this patch. You can call Prepare()
before each ManageTiles and whenever a layer is added/removed as far as I care.
I'm just interested in making the interface to the tile manager such that we can
optimize this as much as we like in the future without having to change how we
interact with the tile manager.

vmpstr

PTAL. tldr: I'd like to keep prepare outside of tile manager. Preparing a queue is ...

6 years, 5 months ago (2014-07-09 18:35:38 UTC) #12

PTAL.

tldr: I'd like to keep prepare outside of tile manager. Preparing a queue is a
bit of an implementation detail, and exposing it via some interface to tile
manager seems incorrect. (more in comments below)

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:20: #include "cc/layers/picture_layer_impl.h"
On 2014/07/09 02:21:09, reveman wrote:
> if possible, could you forward declare PictureLayerImpl as part of this patch
> instead of the include?

Done.

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:35: virtual const std::vector<PictureLayerImpl*>&
GetPictureLayers() const = 0;
On 2014/07/09 02:21:09, reveman wrote:
> Note: this was added with the assumption that we'd use it for this patch. Now
> that this is no longer the case, I think we should somehow merge it with
> NotifyReadyToActivate function and not expose layers to the tile manager but
> that can be done in a separate follow-up patch. Maybe just add a TODO here for
> now.

Done.

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:48: virtual EvictionTileQueue*
GetEvictionQueue(TreePriority tree_priority) = 0;
On 2014/07/09 02:21:09, reveman wrote:
> Can we pass these queues to the ctor instead? If the tree priority changes,
> maybe the priority queues given to the tile manager could just automatically
> reflect that. Or we have one queue for each tree priority type but that seems
a
> bit overkill.

In the constructor of tile manager? That change would require us to prepare the
queues whenever layers/priorities change (see last comment), which is something
I want to avoid at least for this patch, since I think it might be a bit error
prone to determine exactly when to do the preparation (hence the TODO in the
implementations of those functions).

> 
> Btw, I don't think tree_priority is something that the tile manager should
cache
> and only update when ManageTiles is called anymore. That made sense when the
> tile manager was in control over the raster order and could prevent it from
> changing between calls to ManageTiles. So GlobalStateThatImpactsTilePriority
> should probably be retrieved using a client function instead of being passed
to
> ManageTiles. We can change this in a follow-up patch if you prefer though.

That seems a bit out of scope for this patch, let's do that as a follow-up.

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_prior...
File cc/resources/tile_priority_queue.h (right):

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_prior...
cc/resources/tile_priority_queue.h:27: class CC_EXPORT RasterTileQueue {
On 2014/07/09 02:21:09, reveman wrote:
> How about only declaring an abstract TilePriortyQueue class here that is as
> simple as:
> 
> class TilePriortyQueue {
>  public:
>   virtual bool IsEmpty() = 0;
>   virtual Tile* Top() = 0;
>   virtual void Pop() = 0;
> 
>  protected:
>   virtual ~TilePriortyQueue() {}
> };
> 
> as that's really the only thing the tile manager needs. Raster and eviction
are
> just different implementations of this interface but that's not something the
> tile manager needs to be aware of.
> 
> Tile{Raster|Eviction}Queue implementations of this interface can live in
> different files or just in the LTHI .cc file.

Done.

https://codereview.chromium.org/367833003/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/367833003/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1258: raster_queue_.Prepare(paired_layers,
tree_priority);
On 2014/07/09 02:21:09, reveman wrote:
> I don't think we should use a client function to trigger work that needs to be
> done before the queue can be used. I'd rather see us handle that transparently
> to the tile manager by preparing the queue or marking it as needing to be
> prepared before use as needed.
> 
> I don't care how much you optimize this in this patch. You can call Prepare()
> before each ManageTiles and whenever a layer is added/removed as far as I
care.
> I'm just interested in making the interface to the tile manager such that we
can
> optimize this as much as we like in the future without having to change how we
> interact with the tile manager.

Calling prepare in tile manager seems like a wrong thing to do. I'd like to see
tile manager simply use these queues, without any knowledge of what needs to be
done to them before they are ready to be consumed. Also, preparing requires a
set of layer pairs, and I'd like to avoid both having tile manager get those
pairs and having a client back pointer in the queues.

All in all, when we call prepare can be changed, but I still think tile manager
should not know how to initialize the queues. Would you agree to this solution
as a way that makes tile manager interface simple? I'm willing to work on
figuring out the ideal spot for prepare as a follow-up.

reveman

I think we're on the same page in terms of how the tile manager should ...

6 years, 5 months ago (2014-07-09 20:47:51 UTC) #13

I think we're on the same page in terms of how the tile manager should interact
with these queues. Calling Prepare in the tile manager would be wrong but so is
calling GetRasterQueue as it's essentially the same because it needs to be
called before the tile manager can use the queue. I tried to explain in the
comment how we could fix this. Let me know what you think or if it's not clear.

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:48: virtual EvictionTileQueue*
GetEvictionQueue(TreePriority tree_priority) = 0;
On 2014/07/09 18:35:38, vmpstr wrote:
> On 2014/07/09 02:21:09, reveman wrote:
> > Can we pass these queues to the ctor instead? If the tree priority changes,
> > maybe the priority queues given to the tile manager could just automatically
> > reflect that. Or we have one queue for each tree priority type but that
seems
> a
> > bit overkill.
> 
> In the constructor of tile manager? That change would require us to prepare
the
> queues whenever layers/priorities change (see last comment), which is
something
> I want to avoid at least for this patch, since I think it might be a bit error
> prone to determine exactly when to do the preparation (hence the TODO in the
> implementations of those functions).

Yes, having the tile manager call Prepare() would be wrong. That's not what I
was suggesting.

From the tile managers pov, there are 2 queues (or maybe 6 if we decide to use
one for each tree priority) and they are always up to date. Tile manager
shouldn't have to call Prepare or GetRasterQueue to get an up to date version of
a queue. Hence, we should be able to pass each queue to the tile manager ctor
and the tile manager should just be able to use them as it likes.

For efficiency it makes sense to lazy update the queue as needed when
IsEmpty/Top/Pop is called. We can make this lazy update mechanism optimal in the
future (delay as much work as possible and never do more work than necessary)
but for now it might be good enough to have LTHI mark each queue as in need of
lazy prepare work before calling ManageTiles or when a layer is added/removed.
LTHI would only mark the queue as in need of work and not actually do it. The
work would only be done if one of the queue API functions are called by the tile
manager.

Wdyt?

> 
> > 
> > Btw, I don't think tree_priority is something that the tile manager should
> cache
> > and only update when ManageTiles is called anymore. That made sense when the
> > tile manager was in control over the raster order and could prevent it from
> > changing between calls to ManageTiles. So GlobalStateThatImpactsTilePriority
> > should probably be retrieved using a client function instead of being passed
> to
> > ManageTiles. We can change this in a follow-up patch if you prefer though.
> 
> That seems a bit out of scope for this patch, let's do that as a follow-up.
>

vmpstr

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manager.h File cc/resources/tile_manager.h (right): https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manager.h#newcode48 cc/resources/tile_manager.h:48: virtual EvictionTileQueue* GetEvictionQueue(TreePriority tree_priority) = 0; On 2014/07/09 20:47:51, ...

6 years, 5 months ago (2014-07-09 22:46:50 UTC) #14

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:48: virtual EvictionTileQueue*
GetEvictionQueue(TreePriority tree_priority) = 0;
On 2014/07/09 20:47:51, reveman wrote:
> On 2014/07/09 18:35:38, vmpstr wrote:
> > On 2014/07/09 02:21:09, reveman wrote:
> > > Can we pass these queues to the ctor instead? If the tree priority
changes,
> > > maybe the priority queues given to the tile manager could just
automatically
> > > reflect that. Or we have one queue for each tree priority type but that
> seems
> > a
> > > bit overkill.
> > 
> > In the constructor of tile manager? That change would require us to prepare
> the
> > queues whenever layers/priorities change (see last comment), which is
> something
> > I want to avoid at least for this patch, since I think it might be a bit
error
> > prone to determine exactly when to do the preparation (hence the TODO in the
> > implementations of those functions).
> 
> Yes, having the tile manager call Prepare() would be wrong. That's not what I
> was suggesting.
> 
> From the tile managers pov, there are 2 queues (or maybe 6 if we decide to use
> one for each tree priority) and they are always up to date. Tile manager
> shouldn't have to call Prepare or GetRasterQueue to get an up to date version
of
> a queue. Hence, we should be able to pass each queue to the tile manager ctor
> and the tile manager should just be able to use them as it likes.
> 
> For efficiency it makes sense to lazy update the queue as needed when
> IsEmpty/Top/Pop is called. We can make this lazy update mechanism optimal in
the
> future (delay as much work as possible and never do more work than necessary)
> but for now it might be good enough to have LTHI mark each queue as in need of
> lazy prepare work before calling ManageTiles or when a layer is added/removed.
> LTHI would only mark the queue as in need of work and not actually do it. The
> work would only be done if one of the queue API functions are called by the
tile
> manager.
> 
> Wdyt?
> 

Let's assume that we're going to work with 2 queues, as having 6 of them is a
bit bulky, I think. In theory, I like the approach you are suggesting. There's a
few details that make this approach awkward/fragile or at least that need to be
addressed. In particular, when we call manage tiles, we pop off some raster
tiles, turn them into tasks, and schedule them to be rasterized. This means that
these tiles are not in the raster queue, but also are not yet rasterized. At a
later time, we can ScheduleMoreWork (or whatever it's called nowadays) and if
those tiles were not yet rasterized, we expect to see them again. I know that
schedule more work is technically should only be called when we complete all
scheduled work, but I think that makes the interaction somewhat fragile and
non-obvious.

If we accept that schedule more tiles can be called regardless of the status of
previously scheduled work, then we somehow need to inform the queue that it
needs to do something before AssignGpuMem call. Since this call can happen
independently of LTHI or anything outside the tile manager, I don't see how we
can get away with having a member queue pointer in tile manager (passed in
ctor), and only call one of Top/Pop/IsEmpty. 

Note that I'm OK with assuming that schedule more work is only called when all
tiles complete, but that's something that we have to consciously decide.

Wdyt?

> > 
> > > 
> > > Btw, I don't think tree_priority is something that the tile manager should
> > cache
> > > and only update when ManageTiles is called anymore. That made sense when
the
> > > tile manager was in control over the raster order and could prevent it
from
> > > changing between calls to ManageTiles. So
GlobalStateThatImpactsTilePriority
> > > should probably be retrieved using a client function instead of being
passed
> > to
> > > ManageTiles. We can change this in a follow-up patch if you prefer though.
> > 
> > That seems a bit out of scope for this patch, let's do that as a follow-up.
> > 
>

reveman

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manager.h File cc/resources/tile_manager.h (right): https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manager.h#newcode48 cc/resources/tile_manager.h:48: virtual EvictionTileQueue* GetEvictionQueue(TreePriority tree_priority) = 0; On 2014/07/09 22:46:50, ...

6 years, 5 months ago (2014-07-10 11:28:24 UTC) #15

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/367833003/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:48: virtual EvictionTileQueue*
GetEvictionQueue(TreePriority tree_priority) = 0;
On 2014/07/09 22:46:50, vmpstr wrote:
> On 2014/07/09 20:47:51, reveman wrote:
> > On 2014/07/09 18:35:38, vmpstr wrote:
> > > On 2014/07/09 02:21:09, reveman wrote:
> > > > Can we pass these queues to the ctor instead? If the tree priority
> changes,
> > > > maybe the priority queues given to the tile manager could just
> automatically
> > > > reflect that. Or we have one queue for each tree priority type but that
> > seems
> > > a
> > > > bit overkill.
> > > 
> > > In the constructor of tile manager? That change would require us to
prepare
> > the
> > > queues whenever layers/priorities change (see last comment), which is
> > something
> > > I want to avoid at least for this patch, since I think it might be a bit
> error
> > > prone to determine exactly when to do the preparation (hence the TODO in
the
> > > implementations of those functions).
> > 
> > Yes, having the tile manager call Prepare() would be wrong. That's not what
I
> > was suggesting.
> > 
> > From the tile managers pov, there are 2 queues (or maybe 6 if we decide to
use
> > one for each tree priority) and they are always up to date. Tile manager
> > shouldn't have to call Prepare or GetRasterQueue to get an up to date
version
> of
> > a queue. Hence, we should be able to pass each queue to the tile manager
ctor
> > and the tile manager should just be able to use them as it likes.
> > 
> > For efficiency it makes sense to lazy update the queue as needed when
> > IsEmpty/Top/Pop is called. We can make this lazy update mechanism optimal in
> the
> > future (delay as much work as possible and never do more work than
necessary)
> > but for now it might be good enough to have LTHI mark each queue as in need
of
> > lazy prepare work before calling ManageTiles or when a layer is
added/removed.
> > LTHI would only mark the queue as in need of work and not actually do it.
The
> > work would only be done if one of the queue API functions are called by the
> tile
> > manager.
> > 
> > Wdyt?
> > 
> 
> Let's assume that we're going to work with 2 queues, as having 6 of them is a
> bit bulky, I think. In theory, I like the approach you are suggesting. There's
a
> few details that make this approach awkward/fragile or at least that need to
be
> addressed. In particular, when we call manage tiles, we pop off some raster
> tiles, turn them into tasks, and schedule them to be rasterized. This means
that
> these tiles are not in the raster queue, but also are not yet rasterized. At a
> later time, we can ScheduleMoreWork (or whatever it's called nowadays) and if
> those tiles were not yet rasterized, we expect to see them again. I know that
> schedule more work is technically should only be called when we complete all
> scheduled work, but I think that makes the interaction somewhat fragile and
> non-obvious.
> 
> If we accept that schedule more tiles can be called regardless of the status
of
> previously scheduled work, then we somehow need to inform the queue that it
> needs to do something before AssignGpuMem call. Since this call can happen
> independently of LTHI or anything outside the tile manager, I don't see how we
> can get away with having a member queue pointer in tile manager (passed in
> ctor), and only call one of Top/Pop/IsEmpty. 
> 
> Note that I'm OK with assuming that schedule more work is only called when all
> tiles complete, but that's something that we have to consciously decide.
> 
> Wdyt?

Ah! I forgot the reason we were using iterators and not a priority queue in the
tile manager. To be able to cancel tasks, of course. I think that's very useful
and something we should keep. Sorry for all this priority queue stuff.

Not all to waste though, I hope. Doing lazy work as part of the underlying data
type instead of the iterator still makes sense. I guess the data type is some
sort of lazy initializing collection of tiles that are ordered by priority.
Makes sense for the tile manager to use an iterator to access it but the logic
for lazy initialization should probably be in the collection rather than the
iterator.

Let's chat offline today when you have time.

> 
> > > 
> > > > 
> > > > Btw, I don't think tree_priority is something that the tile manager
should
> > > cache
> > > > and only update when ManageTiles is called anymore. That made sense when
> the
> > > > tile manager was in control over the raster order and could prevent it
> from
> > > > changing between calls to ManageTiles. So
> GlobalStateThatImpactsTilePriority
> > > > should probably be retrieved using a client function instead of being
> passed
> > > to
> > > > ManageTiles. We can change this in a follow-up patch if you prefer
though.
> > > 
> > > That seems a bit out of scope for this patch, let's do that as a
follow-up.
> > > 
> > 
>

vmpstr

PTAL. I renamed Get*Queue to Rebuild*Queue, which aligns with what we've discussed offline. I do ...

6 years, 5 months ago (2014-07-11 18:39:37 UTC) #16

reveman

https://codereview.chromium.org/367833003/diff/220001/cc/resources/eviction_tile_priority_queue.cc File cc/resources/eviction_tile_priority_queue.cc (right): https://codereview.chromium.org/367833003/diff/220001/cc/resources/eviction_tile_priority_queue.cc#newcode63 cc/resources/eviction_tile_priority_queue.cc:63: Initialize(); I think this lazy init stuff should be ...

6 years, 5 months ago (2014-07-14 15:52:38 UTC) #17

vmpstr

PTAL. Mostly just replies to comments... https://codereview.chromium.org/367833003/diff/220001/cc/resources/eviction_tile_priority_queue.cc File cc/resources/eviction_tile_priority_queue.cc (right): https://codereview.chromium.org/367833003/diff/220001/cc/resources/eviction_tile_priority_queue.cc#newcode63 cc/resources/eviction_tile_priority_queue.cc:63: Initialize(); On 2014/07/14 ...

6 years, 5 months ago (2014-07-14 18:10:45 UTC) #18

PTAL. Mostly just replies to comments...

https://codereview.chromium.org/367833003/diff/220001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.cc (right):

https://codereview.chromium.org/367833003/diff/220001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:63: Initialize();
On 2014/07/14 15:52:38, reveman wrote:
> I think this lazy init stuff should be consistent between raster/eviction.
> However, now that it's explicit that building a queue has a cost we could
> instead handle this optimization in AssignGpuMemory and only create/rebuild
the
> queue if necessary.

I would strongly prefer to keep this knowledge outside of tile manager. I think
the fact that we lazy initialize is an implementation detail of this particular
queue. If we tie in the construction to be in tile manager, then I think we
restrict how much we can change the underlying implementation later. For
instance, if we maintain a persistent queue in LTHI, and update it as it
evolves, we can have TilePriorityQueue* RebuildPriorityQueue simply return a
pointer (and maybe rebuild it in special situations). If we enforce tile manager
calls some sort of a "construct/rebuild" operation on it, then it's either going
to be no-op or do unnecessary work.

https://codereview.chromium.org/367833003/diff/220001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.h (right):

https://codereview.chromium.org/367833003/diff/220001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.h:59: void Initialize();
On 2014/07/14 15:52:38, reveman wrote:
> Why is there both a Prepare function and an Initialize function? Could they be
> combined somehow and/or renamed to better describe what they do?

Prepare is a part of a public interface, Initialize is a private function...
I've renamed Prepare to be RebuildQueue... Other than that, if you have other
naming suggestions, I'd be happy to change them.

https://codereview.chromium.org/367833003/diff/220001/cc/resources/raster_til...
File cc/resources/raster_tile_priority_queue.cc (right):

https://codereview.chromium.org/367833003/diff/220001/cc/resources/raster_til...
cc/resources/raster_tile_priority_queue.cc:216: }  // namespace cc
On 2014/07/14 15:52:38, reveman wrote:
> A lot of code duplication in eviction/raster_tile_priority_queue.cc. How do we
> fix that?

Initial thought is to move common code up to TilePriorityQueue, but I don't
exactly like that. I don't want us to fall into the same trap as we did with
rasterizer worker things. There, we had a lot of common code, but that greatly
reduced the readability and simplicity of the code. As you know, we eventually
went with no duplication since the implementations grew in separate directions.

At this point in time, these queues are very similar. However, eviction for
instance might change via https://codereview.chromium.org/377793003/. At this
time, I'd like to keep these separate. Once we're happy with the performance and
implementation of each of the queues, we can refactor. Wdyt?

https://codereview.chromium.org/367833003/diff/220001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/367833003/diff/220001/cc/resources/tile_manag...
cc/resources/tile_manager.h:51: TreePriority tree_priority) = 0;
On 2014/07/14 15:52:38, reveman wrote:
> This API is not perfectly clear. What happens if I call RebuildRasterQueue
> twice? Does that invalidate the first queue returned? Will the pointer
returned
> always be the same?
> 
> I can think of two options to make this cleaner. We can me make the lifetime
of
> the queue explicit:
> 
> scoped_ptr<TilePriorityQueue> CreateRasterQueue(TreePriority priority);
> 
> or if you want to avoid allocation of a new queue each time I think the API
> should be something like this:
> 
> void BuildRasterQueue(RasterTilePriorityQueue* queue,
>                       TreePriority priority);
> 
> where the tile manager owns the RasterTilePriorityQueue instance.

Yes, I'd like to avoid allocations of a new queue. The second approach limits
what we can maintain in the raster queue. That is, if we do come up with a way
to maintain a persistent queue, then having tile manager own it limits that. Is
it possible to address this with either a function rename or maybe an explicit
comment here?

https://codereview.chromium.org/367833003/diff/220001/cc/resources/tile_prior...
File cc/resources/tile_priority_queue.h (right):

https://codereview.chromium.org/367833003/diff/220001/cc/resources/tile_prior...
cc/resources/tile_priority_queue.h:14: class CC_EXPORT TilePriorityQueue {
On 2014/07/14 15:52:38, reveman wrote:
> I don't think you need CC_EXPORT when the dtor is protected.

Done.

reveman

https://codereview.chromium.org/367833003/diff/220001/cc/resources/raster_tile_priority_queue.cc File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/367833003/diff/220001/cc/resources/raster_tile_priority_queue.cc#newcode216 cc/resources/raster_tile_priority_queue.cc:216: } // namespace cc On 2014/07/14 18:10:44, vmpstr wrote: ...

6 years, 5 months ago (2014-07-14 19:09:46 UTC) #19

https://codereview.chromium.org/367833003/diff/220001/cc/resources/raster_til...
File cc/resources/raster_tile_priority_queue.cc (right):

https://codereview.chromium.org/367833003/diff/220001/cc/resources/raster_til...
cc/resources/raster_tile_priority_queue.cc:216: }  // namespace cc
On 2014/07/14 18:10:44, vmpstr wrote:
> On 2014/07/14 15:52:38, reveman wrote:
> > A lot of code duplication in eviction/raster_tile_priority_queue.cc. How do
we
> > fix that?
> 
> Initial thought is to move common code up to TilePriorityQueue, but I don't
> exactly like that. I don't want us to fall into the same trap as we did with
> rasterizer worker things. There, we had a lot of common code, but that greatly
> reduced the readability and simplicity of the code. As you know, we eventually
> went with no duplication since the implementations grew in separate
directions.
> 
> At this point in time, these queues are very similar. However, eviction for
> instance might change via https://codereview.chromium.org/377793003/. At this
> time, I'd like to keep these separate. Once we're happy with the performance
and
> implementation of each of the queues, we can refactor. Wdyt?

I'm not sure. I'll re-evaluate this when we're happy with other parts of the
patch.

https://codereview.chromium.org/367833003/diff/220001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/367833003/diff/220001/cc/resources/tile_manag...
cc/resources/tile_manager.h:51: TreePriority tree_priority) = 0;
On 2014/07/14 18:10:44, vmpstr wrote:
> On 2014/07/14 15:52:38, reveman wrote:
> > This API is not perfectly clear. What happens if I call RebuildRasterQueue
> > twice? Does that invalidate the first queue returned? Will the pointer
> returned
> > always be the same?
> > 
> > I can think of two options to make this cleaner. We can me make the lifetime
> of
> > the queue explicit:
> > 
> > scoped_ptr<TilePriorityQueue> CreateRasterQueue(TreePriority priority);
> > 
> > or if you want to avoid allocation of a new queue each time I think the API
> > should be something like this:
> > 
> > void BuildRasterQueue(RasterTilePriorityQueue* queue,
> >                       TreePriority priority);
> > 
> > where the tile manager owns the RasterTilePriorityQueue instance.
> 
> Yes, I'd like to avoid allocations of a new queue. The second approach limits
> what we can maintain in the raster queue. That is, if we do come up with a way
> to maintain a persistent queue, then having tile manager own it limits that.
Is
> it possible to address this with either a function rename or maybe an explicit
> comment here?

We can't maintain a persistent queue with the current design, as it's a priority
queue and the only way to use it, is to destroy it. Making it persistent in a
way that's not obvious to the tile manager would be a big mistake imo. Except
maybe [1].

We need to expose a generic collection to the tile manager that can be iterated
if we want to leave the door open to a persistent data type. I think the current
priority queue approach is fine even though it limits the optimizations that can
be done. But I'm also OK with a collection/iterator approach that allows us to
make it persistent in the future but the collection should in that case be
passed to the tile manager ctor and the tile manager should not have to be aware
of the fact that something needs to be rebuilt from scratch when priorities
change.

Just to be clear. If we stick with the current priority queue approach but move
the queue ownership to the tile manager, usage would look something like this:

raster_priority_queue_.Reset();
client_->BuildRasterQueue(&raster_priority_queue_);

I like this as it makes it explicit that we're reusing existing queue storage to
avoid unnecessary allocations and it's consistent with existing code that does
the same. ie. usage of RasterTaskQueue and TaskGraph..

[1] We could allow a persistent queue if the tile manager was responsible for
pushing all elements back on the queue in the same order they were pop-ed.

vmpstr

> We can't maintain a persistent queue with the current design, as it's a priority ...

6 years, 5 months ago (2014-07-14 19:45:24 UTC) #20

> We can't maintain a persistent queue with the current design, as it's a
priority
> queue and the only way to use it, is to destroy it. Making it persistent in a
> way that's not obvious to the tile manager would be a big mistake imo. Except
> maybe [1].

Perhaps persistent isn't the word I was looking for. What I'm saying is that we
could have a priority queue that can be rebuilt using an existing partial
priority queue. For instance, tile manager pops off 3 tiles and schedules them;
the next rebuild (assuming nothing else changed) could check those three tiles
(since it knows what it popped off) and if they still need work, then just
prepend them to be accessed first before the internal heap. Sure this is a bit
contrived, and possibly not worth the complexity, but the point is that it's an
implementation detail of this priority queue and from the tile manager's
perspective, calling "rebuild" is the same whether we do it from scratch or we
do it incrementally. Calling an explicit reset in the tile manager (or even
having tile manager own these queues) eliminates this as a possibility
altogether; it also doesn't introduce any new optimization paths IMO (but I
haven't thought about this too much yet).

> 
> We need to expose a generic collection to the tile manager that can be
iterated
> if we want to leave the door open to a persistent data type. I think the
current
> priority queue approach is fine even though it limits the optimizations that
can
> be done. But I'm also OK with a collection/iterator approach that allows us to
> make it persistent in the future but the collection should in that case be
> passed to the tile manager ctor and the tile manager should not have to be
aware
> of the fact that something needs to be rebuilt from scratch when priorities
> change.
> 
> Just to be clear. If we stick with the current priority queue approach but
move
> the queue ownership to the tile manager, usage would look something like this:
> 
> raster_priority_queue_.Reset();
> client_->BuildRasterQueue(&raster_priority_queue_);

Sure I'll do this.

> 
> I like this as it makes it explicit that we're reusing existing queue storage
to
> avoid unnecessary allocations and it's consistent with existing code that does
> the same. ie. usage of RasterTaskQueue and TaskGraph..
> 
> 
> [1] We could allow a persistent queue if the tile manager was responsible for
> pushing all elements back on the queue in the same order they were pop-ed.

reveman

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_tile_priority_queue.cc File cc/resources/eviction_tile_priority_queue.cc (right): https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_tile_priority_queue.cc#newcode27 cc/resources/eviction_tile_priority_queue.cc:27: paired_picture_layers_ = paired_picture_layers; Hm, you're making a copy of ...

6 years, 5 months ago (2014-07-15 01:51:10 UTC) #22

vmpstr

Just replies to comments, I will update the patch in the morning. https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_tile_priority_queue.cc File cc/resources/eviction_tile_priority_queue.cc ...

6 years, 5 months ago (2014-07-15 05:36:27 UTC) #23

Just replies to comments, I will update the patch in the morning.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.cc (right):

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:27: paired_picture_layers_ =
paired_picture_layers;
On 2014/07/15 01:51:10, reveman wrote:
> Hm, you're making a copy of a large vector here. I would have guessed that
> iterating the vector and building the paired_iterators_ vector would have a
> similar cost. Now we're doing both..

We can pass the raw layers vector and build it, if you prefer.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:35:
iterator_heap_.reserve(paired_picture_layers_.size());
On 2014/07/15 01:51:09, reveman wrote:
> Let's not bother with reserve() when this only grows once.

That knowledge is not obvious in this class, but sure I can do that.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:57: }
On 2014/07/15 01:51:10, reveman wrote:
> How much of the cost of Initialize is building these vectors compared to
> make_heap? Lazy make_heap makes sense but it's not clear why building a vector
> that is already large enough would be expensive. Would it help to not call
> PeekTile here?

The vector building here is the expensive part. The ctor of
LayerEvictionTileIterator creates TilingEvictionTileIterators, which cause us to
do work. After iterator construction, PeekTile is almost free. make_heap has its
own cost, but I doubt it's the dominant one.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.h (right):

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.h:46: std::vector<Tile*>
returned_shared_tiles;
On 2014/07/15 01:51:10, reveman wrote:
> can you explain what this is for? looked at the .cc file but not perfectly
> clear.

active and pending iterators can return the same tile (if it's shared on both
trees). When we return a tile, we put it in this vector. Later, when a tile is
returned we check this vector and if the tile was already returned (ie is
present in the vector), we skip it.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.h:63:
std::vector<PairedPictureLayerIterator*> iterator_heap_;
On 2014/07/15 01:51:10, reveman wrote:
> why both paired_iterators_ and iterator_heap_ instead of just making
> paired_iterators_ a heap? are iterator instances large enough that they become
> expensive to move? in that case, can we do something about that?

They are certainly more expensive than moving a pointer. Not sure what we can do
here: layer iterators contain a vector or two, with some more data. During
iteration we remove and re-add those, so the cost accumulates.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/367833003/diff/280001/cc/resources/tile_manag...
cc/resources/tile_manager.h:53: TreePriority tree_priority) = 0;
On 2014/07/15 01:51:10, reveman wrote:
> Looks good. Can you add a short comment above each of these functions that
> explains what the queue can be used for and that it needs to be empty when
> passed to this function.

Will do.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/tile_prior...
File cc/resources/tile_priority_queue.h (right):

https://codereview.chromium.org/367833003/diff/280001/cc/resources/tile_prior...
cc/resources/tile_priority_queue.h:12: class TilePriorityQueue {
On 2014/07/15 01:51:10, reveman wrote:
> Do we still need this interface?

Now that both LTHI and tile manager know about specific types, this can be
removed. I will do that.

reveman

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_tile_priority_queue.cc File cc/resources/eviction_tile_priority_queue.cc (right): https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_tile_priority_queue.cc#newcode27 cc/resources/eviction_tile_priority_queue.cc:27: paired_picture_layers_ = paired_picture_layers; On 2014/07/15 05:36:26, vmpstr wrote: > ...

6 years, 5 months ago (2014-07-15 16:49:57 UTC) #24

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.cc (right):

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:27: paired_picture_layers_ =
paired_picture_layers;
On 2014/07/15 05:36:26, vmpstr wrote:
> On 2014/07/15 01:51:10, reveman wrote:
> > Hm, you're making a copy of a large vector here. I would have guessed that
> > iterating the vector and building the paired_iterators_ vector would have a
> > similar cost. Now we're doing both..
> 
> We can pass the raw layers vector and build it, if you prefer.

Keeping a raw pointer around and assuming that it's valid when we like to use it
is really nasty. I'd much rather we made the building of the iterator vector
fast enough that we don't need to delay that part. That would not only improve
this specific use case you're trying to optimize for here but the general usage.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:57: }
On 2014/07/15 05:36:26, vmpstr wrote:
> On 2014/07/15 01:51:10, reveman wrote:
> > How much of the cost of Initialize is building these vectors compared to
> > make_heap? Lazy make_heap makes sense but it's not clear why building a
vector
> > that is already large enough would be expensive. Would it help to not call
> > PeekTile here?
> 
> The vector building here is the expensive part. The ctor of
> LayerEvictionTileIterator creates TilingEvictionTileIterators, which cause us
to
> do work. After iterator construction, PeekTile is almost free. make_heap has
its
> own cost, but I doubt it's the dominant one.

Could most of the ctor cost be moved to PeekTile in a clean way?

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.h (right):

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.h:33: struct
PairedPictureLayerIterator {
This doesn't look like an iterator. PeekTile/PopTile, looks like this is also a
priority queue. PairedPictureLayerQueue?

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.h:46: std::vector<Tile*>
returned_shared_tiles;
On 2014/07/15 05:36:26, vmpstr wrote:
> On 2014/07/15 01:51:10, reveman wrote:
> > can you explain what this is for? looked at the .cc file but not perfectly
> > clear.
> 
> active and pending iterators can return the same tile (if it's shared on both
> trees). When we return a tile, we put it in this vector. Later, when a tile is
> returned we check this vector and if the tile was already returned (ie is
> present in the vector), we skip it.

That's unfortunate. Sort of defeats the purpose of reusing the queue to avoid
heap allocations if you're going to have to heap allocate these vectors every
time.. But I guess it's not too bad as they are empty by default and not
populated until PopTile is called.

We could handle this much more efficiently at the tile manager level as we
already have a vector (never larger than 32) with the tiles already returned
that we can check against..

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.h:63:
std::vector<PairedPictureLayerIterator*> iterator_heap_;
On 2014/07/15 05:36:27, vmpstr wrote:
> On 2014/07/15 01:51:10, reveman wrote:
> > why both paired_iterators_ and iterator_heap_ instead of just making
> > paired_iterators_ a heap? are iterator instances large enough that they
become
> > expensive to move? in that case, can we do something about that?
> 
> They are certainly more expensive than moving a pointer. Not sure what we can
do
> here: layer iterators contain a vector or two, with some more data. During
> iteration we remove and re-add those, so the cost accumulates.

How hard would it be to remove these vectors or replace them by arrays? Having
all the priority queue data stored in one chunk of memory could make a big
difference performance wise.

A heap with pointers might still make sense though. Have you done any profiling
of this?

vmpstr

PTAL https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_tile_priority_queue.cc File cc/resources/eviction_tile_priority_queue.cc (right): https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_tile_priority_queue.cc#newcode27 cc/resources/eviction_tile_priority_queue.cc:27: paired_picture_layers_ = paired_picture_layers; On 2014/07/15 16:49:56, reveman wrote: ...

6 years, 5 months ago (2014-07-15 17:47:21 UTC) #25

PTAL

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.cc (right):

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:27: paired_picture_layers_ =
paired_picture_layers;
On 2014/07/15 16:49:56, reveman wrote:
> On 2014/07/15 05:36:26, vmpstr wrote:
> > On 2014/07/15 01:51:10, reveman wrote:
> > > Hm, you're making a copy of a large vector here. I would have guessed that
> > > iterating the vector and building the paired_iterators_ vector would have
a
> > > similar cost. Now we're doing both..
> > 
> > We can pass the raw layers vector and build it, if you prefer.
> 
> Keeping a raw pointer around and assuming that it's valid when we like to use
it
> is really nasty. I'd much rather we made the building of the iterator vector
> fast enough that we don't need to delay that part. That would not only improve
> this specific use case you're trying to optimize for here but the general
usage.

I meant we can pass in const std::vector<PictureLayerImpl*>& layers, and build
paired_picture_layers_ as a part of BuildQueue (this is definitely can be common
code to TilePriorityIterator), which would save a copy (take a look at the
latest patch set).

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:57: }
On 2014/07/15 16:49:56, reveman wrote:
> On 2014/07/15 05:36:26, vmpstr wrote:
> > On 2014/07/15 01:51:10, reveman wrote:
> > > How much of the cost of Initialize is building these vectors compared to
> > > make_heap? Lazy make_heap makes sense but it's not clear why building a
> vector
> > > that is already large enough would be expensive. Would it help to not call
> > > PeekTile here?
> > 
> > The vector building here is the expensive part. The ctor of
> > LayerEvictionTileIterator creates TilingEvictionTileIterators, which cause
us
> to
> > do work. After iterator construction, PeekTile is almost free. make_heap has
> its
> > own cost, but I doubt it's the dominant one.
> 
> Could most of the ctor cost be moved to PeekTile in a clean way?

PeekTile will find a tiling iterator and access its tile. We _could_ make more
things lazy, but I think that's overly complicating things since then
TilingRasterTileIterator becomes a queue as well, and it becomes lazy in much
the same way as this is.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.h (right):

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.h:33: struct
PairedPictureLayerIterator {
On 2014/07/15 16:49:56, reveman wrote:
> This doesn't look like an iterator. PeekTile/PopTile, looks like this is also
a
> priority queue. PairedPictureLayerQueue?

Yeah you're right, I will change that.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.h:46: std::vector<Tile*>
returned_shared_tiles;
On 2014/07/15 16:49:56, reveman wrote:
> On 2014/07/15 05:36:26, vmpstr wrote:
> > On 2014/07/15 01:51:10, reveman wrote:
> > > can you explain what this is for? looked at the .cc file but not perfectly
> > > clear.
> > 
> > active and pending iterators can return the same tile (if it's shared on
both
> > trees). When we return a tile, we put it in this vector. Later, when a tile
is
> > returned we check this vector and if the tile was already returned (ie is
> > present in the vector), we skip it.
> 
> That's unfortunate. Sort of defeats the purpose of reusing the queue to avoid
> heap allocations if you're going to have to heap allocate these vectors every
> time.. But I guess it's not too bad as they are empty by default and not
> populated until PopTile is called.
> 
> We could handle this much more efficiently at the tile manager level as we
> already have a vector (never larger than 32) with the tiles already returned
> that we can check against..

That was one of the considerations I had initially. It feels really awkward to
have a priority queue return a tile, then check if that tile was already
returned though. It's kind of non-obvious that this particular queue can return
duplicate items. You're right that the thing that makes it OK, IMO, is the fact
that it starts empty and only grows on a pop.

Note that this is also per layer pair, which means that they probably remain
smaller than 32 with more than one layer. At tile manager level, this would be
all layers together (probably doesn't make much of a difference though).

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.h:63:
std::vector<PairedPictureLayerIterator*> iterator_heap_;
On 2014/07/15 16:49:57, reveman wrote:
> On 2014/07/15 05:36:27, vmpstr wrote:
> > On 2014/07/15 01:51:10, reveman wrote:
> > > why both paired_iterators_ and iterator_heap_ instead of just making
> > > paired_iterators_ a heap? are iterator instances large enough that they
> become
> > > expensive to move? in that case, can we do something about that?
> > 
> > They are certainly more expensive than moving a pointer. Not sure what we
can
> do
> > here: layer iterators contain a vector or two, with some more data. During
> > iteration we remove and re-add those, so the cost accumulates.
> 
> How hard would it be to remove these vectors or replace them by arrays? Having
> all the priority queue data stored in one chunk of memory could make a big
> difference performance wise.
> 
> A heap with pointers might still make sense though. Have you done any
profiling
> of this?

Sorry, this is already two arrays, one is an array of 4 elements (struct with
two enums), and one is of size 3 (tiling iterators). Tiling iterators has two
tiling data iterators, a couple of enums, a pointer and a bool...

I mean, I would say the size is not small. I haven't done any profiling on this.
My goal at this stage is not really optimizing at this level, but rather
encapsulate all of these operations behind an interface and make sure it works.
If that's done properly, we should be able to play around with different ways of
actually implementing the interface without affecting correctness.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/tile_prior...
File cc/resources/tile_priority_queue.h (right):

https://codereview.chromium.org/367833003/diff/280001/cc/resources/tile_prior...
cc/resources/tile_priority_queue.h:12: class TilePriorityQueue {
On 2014/07/15 05:36:27, vmpstr wrote:
> On 2014/07/15 01:51:10, reveman wrote:
> > Do we still need this interface?
> 
> Now that both LTHI and tile manager know about specific types, this can be
> removed. I will do that.

I'm going to keep this for now, and put GetPairedPictureLayers in here.

reveman

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_tile_priority_queue.cc File cc/resources/eviction_tile_priority_queue.cc (right): https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_tile_priority_queue.cc#newcode57 cc/resources/eviction_tile_priority_queue.cc:57: } On 2014/07/15 17:47:20, vmpstr wrote: > On 2014/07/15 ...

6 years, 5 months ago (2014-07-15 19:44:38 UTC) #26

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.cc (right):

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:57: }
On 2014/07/15 17:47:20, vmpstr wrote:
> On 2014/07/15 16:49:56, reveman wrote:
> > On 2014/07/15 05:36:26, vmpstr wrote:
> > > On 2014/07/15 01:51:10, reveman wrote:
> > > > How much of the cost of Initialize is building these vectors compared to
> > > > make_heap? Lazy make_heap makes sense but it's not clear why building a
> > vector
> > > > that is already large enough would be expensive. Would it help to not
call
> > > > PeekTile here?
> > > 
> > > The vector building here is the expensive part. The ctor of
> > > LayerEvictionTileIterator creates TilingEvictionTileIterators, which cause
> us
> > to
> > > do work. After iterator construction, PeekTile is almost free. make_heap
has
> > its
> > > own cost, but I doubt it's the dominant one.
> > 
> > Could most of the ctor cost be moved to PeekTile in a clean way?
> 
> PeekTile will find a tiling iterator and access its tile. We _could_ make more
> things lazy, but I think that's overly complicating things since then
> TilingRasterTileIterator becomes a queue as well, and it becomes lazy in much
> the same way as this is.

Hm, looks like we already have some lazy init stuff there. I see some "if
(!IsValid()) Initialize();" code which looks OK to me as it's updating the
underlying data type (the tiling) and not destroying it in any way. Is this not
already providing the laziness we need?

Fyi, I don't think it's the laziness that makes it a queue. I think it can be
fine to have a lazy iterator. Ideally it would just make some lazy update to the
underlying data type as what it looks like the TilingRasterTileIterator is
doing. If the iterator uses a heap internally to iterate then it might be better
to use a priority queue API instead trying of an iterator. Doesn't look like
that's the case here.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.h (right):

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.h:46: std::vector<Tile*>
returned_shared_tiles;
On 2014/07/15 17:47:21, vmpstr wrote:
> On 2014/07/15 16:49:56, reveman wrote:
> > On 2014/07/15 05:36:26, vmpstr wrote:
> > > On 2014/07/15 01:51:10, reveman wrote:
> > > > can you explain what this is for? looked at the .cc file but not
perfectly
> > > > clear.
> > > 
> > > active and pending iterators can return the same tile (if it's shared on
> both
> > > trees). When we return a tile, we put it in this vector. Later, when a
tile
> is
> > > returned we check this vector and if the tile was already returned (ie is
> > > present in the vector), we skip it.
> > 
> > That's unfortunate. Sort of defeats the purpose of reusing the queue to
avoid
> > heap allocations if you're going to have to heap allocate these vectors
every
> > time.. But I guess it's not too bad as they are empty by default and not
> > populated until PopTile is called.
> > 
> > We could handle this much more efficiently at the tile manager level as we
> > already have a vector (never larger than 32) with the tiles already returned
> > that we can check against..
> 
> That was one of the considerations I had initially. It feels really awkward to
> have a priority queue return a tile, then check if that tile was already
> returned though. It's kind of non-obvious that this particular queue can
return
> duplicate items. You're right that the thing that makes it OK, IMO, is the
fact
> that it starts empty and only grows on a pop.
> 
> Note that this is also per layer pair, which means that they probably remain
> smaller than 32 with more than one layer. At tile manager level, this would be
> all layers together (probably doesn't make much of a difference though).

Instead of checking if we've returned the tile already using a vector, can you
check if the other tree priority is higher? In which case, we should have
returned the tile already, right?

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.h:63:
std::vector<PairedPictureLayerIterator*> iterator_heap_;
On 2014/07/15 17:47:21, vmpstr wrote:
> On 2014/07/15 16:49:57, reveman wrote:
> > On 2014/07/15 05:36:27, vmpstr wrote:
> > > On 2014/07/15 01:51:10, reveman wrote:
> > > > why both paired_iterators_ and iterator_heap_ instead of just making
> > > > paired_iterators_ a heap? are iterator instances large enough that they
> > become
> > > > expensive to move? in that case, can we do something about that?
> > > 
> > > They are certainly more expensive than moving a pointer. Not sure what we
> can
> > do
> > > here: layer iterators contain a vector or two, with some more data. During
> > > iteration we remove and re-add those, so the cost accumulates.
> > 
> > How hard would it be to remove these vectors or replace them by arrays?
Having
> > all the priority queue data stored in one chunk of memory could make a big
> > difference performance wise.
> > 
> > A heap with pointers might still make sense though. Have you done any
> profiling
> > of this?
> 
> Sorry, this is already two arrays, one is an array of 4 elements (struct with
> two enums), and one is of size 3 (tiling iterators). Tiling iterators has two
> tiling data iterators, a couple of enums, a pointer and a bool...

Cool, that's great! So if we just eliminate the |returned_shared_tiles| vector,
TilePriorityQueue is just one big chunk of memory. I think that's going to be
very valuable when trying to make this fast.

> 
> I mean, I would say the size is not small. I haven't done any profiling on
this.
> My goal at this stage is not really optimizing at this level, but rather
> encapsulate all of these operations behind an interface and make sure it
works.
> If that's done properly, we should be able to play around with different ways
of
> actually implementing the interface without affecting correctness. 

I agree. Getting the interface right is top priority. I think second is making
sure the implementation is simple and clean so that it's easy to follow up with
profiling and performance tweaks.

https://codereview.chromium.org/367833003/diff/300001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.cc (right):

https://codereview.chromium.org/367833003/diff/300001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:30: GetPairedPictureLayers(layers,
&paired_picture_layers_);
Sorry, I think this is the wrong approach. Now we're forced to build a vector
for each iterator while in previous patch LTHI could be maintaining an up to
date version of this vector and only update it when necessary or even make
incremental updates to it.

https://codereview.chromium.org/367833003/diff/300001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:39:
paired_queues_.reserve(paired_picture_layers_.size());
I don't understand this and the comment doesn't help. Why does this vector not
grow just once to the size needed as the other vectors?

vmpstr

David, Your comments here are starting to look more and more like they are addressing ...

6 years, 5 months ago (2014-07-15 20:38:17 UTC) #27

David,

Your comments here are starting to look more and more like they are addressing
some fundamental pieces of this particular implementation, including pieces that
have already landed in previous patches (this CL initially just moved the code
from one spot to another). 

I don't think all those changes are manageable in one CL. I would like to get a
full scope of all the changes that you think have to be made in order for some
version of this CL to land. What would be the best way to get that? We can have
a meeting and I'll file bugs or you can file some bugs? Up to you.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.h (right):

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.h:46: std::vector<Tile*>
returned_shared_tiles;
On 2014/07/15 19:44:35, reveman wrote:
> On 2014/07/15 17:47:21, vmpstr wrote:
> > On 2014/07/15 16:49:56, reveman wrote:
> > > On 2014/07/15 05:36:26, vmpstr wrote:
> > > > On 2014/07/15 01:51:10, reveman wrote:
> > > > > can you explain what this is for? looked at the .cc file but not
> perfectly
> > > > > clear.
> > > > 
> > > > active and pending iterators can return the same tile (if it's shared on
> > both
> > > > trees). When we return a tile, we put it in this vector. Later, when a
> tile
> > is
> > > > returned we check this vector and if the tile was already returned (ie
is
> > > > present in the vector), we skip it.
> > > 
> > > That's unfortunate. Sort of defeats the purpose of reusing the queue to
> avoid
> > > heap allocations if you're going to have to heap allocate these vectors
> every
> > > time.. But I guess it's not too bad as they are empty by default and not
> > > populated until PopTile is called.
> > > 
> > > We could handle this much more efficiently at the tile manager level as we
> > > already have a vector (never larger than 32) with the tiles already
returned
> > > that we can check against..
> > 
> > That was one of the considerations I had initially. It feels really awkward
to
> > have a priority queue return a tile, then check if that tile was already
> > returned though. It's kind of non-obvious that this particular queue can
> return
> > duplicate items. You're right that the thing that makes it OK, IMO, is the
> fact
> > that it starts empty and only grows on a pop.
> > 
> > Note that this is also per layer pair, which means that they probably remain
> > smaller than 32 with more than one layer. At tile manager level, this would
be
> > all layers together (probably doesn't make much of a difference though).
> 
> Instead of checking if we've returned the tile already using a vector, can you
> check if the other tree priority is higher? In which case, we should have
> returned the tile already, right?

I think this would not work for eviction, since tiles are turned in combined
priority order (lowest to highest).

https://codereview.chromium.org/367833003/diff/300001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.cc (right):

https://codereview.chromium.org/367833003/diff/300001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:30: GetPairedPictureLayers(layers,
&paired_picture_layers_);
On 2014/07/15 19:44:37, reveman wrote:
> Sorry, I think this is the wrong approach. Now we're forced to build a vector
> for each iterator while in previous patch LTHI could be maintaining an up to
> date version of this vector and only update it when necessary or even make
> incremental updates to it.

I don't think this is too bad. Incremental updates to this would require things
like activation handling, which will iterate everything, creating new pending
trees (which would be a bit of a pain to maintain) and deleting a layer would
involve looking through this whole vector anyway. 

Since we're talking about the number of layers here, I would argue this is
acceptable. If you disagree, then I'm not sure what we can do better (previous
version did a copy of pairs). I don't think we can build the heap here, because
it's too expensive. Making it cheaper is probably possible, but it will also
probably involve some redesign. Please suggest an alternative.

If making iterator creating cheap is the direction you're suggesting, then most
likely all of the pieces that have already landed will need to be changed. In
that case, I think we have to schedule a meeting and discuss this in a larger
setting (ie, back to the drawing board).

https://codereview.chromium.org/367833003/diff/300001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:39:
paired_queues_.reserve(paired_picture_layers_.size());
On 2014/07/15 19:44:37, reveman wrote:
> I don't understand this and the comment doesn't help. Why does this vector not
> grow just once to the size needed as the other vectors?

These pointers point to the vector's memory. If paired_queues_ is reallocated
internally, it invalidates the pointers that have already been created.

reveman

I'm hoping to see a clean implementations of these new tile priority queue classes before ...

6 years, 5 months ago (2014-07-15 22:31:55 UTC) #28

I'm hoping to see a clean implementations of these new tile priority queue
classes before this lands. Let's chat offline if that's useful.

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.h (right):

https://codereview.chromium.org/367833003/diff/280001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.h:46: std::vector<Tile*>
returned_shared_tiles;
On 2014/07/15 20:38:17, vmpstr wrote:
> On 2014/07/15 19:44:35, reveman wrote:
> > On 2014/07/15 17:47:21, vmpstr wrote:
> > > On 2014/07/15 16:49:56, reveman wrote:
> > > > On 2014/07/15 05:36:26, vmpstr wrote:
> > > > > On 2014/07/15 01:51:10, reveman wrote:
> > > > > > can you explain what this is for? looked at the .cc file but not
> > perfectly
> > > > > > clear.
> > > > > 
> > > > > active and pending iterators can return the same tile (if it's shared
on
> > > both
> > > > > trees). When we return a tile, we put it in this vector. Later, when a
> > tile
> > > is
> > > > > returned we check this vector and if the tile was already returned (ie
> is
> > > > > present in the vector), we skip it.
> > > > 
> > > > That's unfortunate. Sort of defeats the purpose of reusing the queue to
> > avoid
> > > > heap allocations if you're going to have to heap allocate these vectors
> > every
> > > > time.. But I guess it's not too bad as they are empty by default and not
> > > > populated until PopTile is called.
> > > > 
> > > > We could handle this much more efficiently at the tile manager level as
we
> > > > already have a vector (never larger than 32) with the tiles already
> returned
> > > > that we can check against..
> > > 
> > > That was one of the considerations I had initially. It feels really
awkward
> to
> > > have a priority queue return a tile, then check if that tile was already
> > > returned though. It's kind of non-obvious that this particular queue can
> > return
> > > duplicate items. You're right that the thing that makes it OK, IMO, is the
> > fact
> > > that it starts empty and only grows on a pop.
> > > 
> > > Note that this is also per layer pair, which means that they probably
remain
> > > smaller than 32 with more than one layer. At tile manager level, this
would
> be
> > > all layers together (probably doesn't make much of a difference though).
> > 
> > Instead of checking if we've returned the tile already using a vector, can
you
> > check if the other tree priority is higher? In which case, we should have
> > returned the tile already, right?
> 
> I think this would not work for eviction, since tiles are turned in combined
> priority order (lowest to highest).

Ok, if this is the only case where we need a temporary heap allocation when
queuing tiles for raster/eviction then it might be worth removing just as a
cleanup. No need to do it as part of this patch but it I think it's worth a TODO
for at least investigating.

https://codereview.chromium.org/367833003/diff/300001/cc/resources/eviction_t...
File cc/resources/eviction_tile_priority_queue.cc (right):

https://codereview.chromium.org/367833003/diff/300001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:30: GetPairedPictureLayers(layers,
&paired_picture_layers_);
On 2014/07/15 20:38:17, vmpstr wrote:
> On 2014/07/15 19:44:37, reveman wrote:
> > Sorry, I think this is the wrong approach. Now we're forced to build a
vector
> > for each iterator while in previous patch LTHI could be maintaining an up to
> > date version of this vector and only update it when necessary or even make
> > incremental updates to it.
> 
> I don't think this is too bad. Incremental updates to this would require
things
> like activation handling, which will iterate everything, creating new pending
> trees (which would be a bit of a pain to maintain) and deleting a layer would
> involve looking through this whole vector anyway.

Incremental approach might not be worth it but you can at least avoid building
it each time you build a queue.

> 
> Since we're talking about the number of layers here, I would argue this is
> acceptable. If you disagree, then I'm not sure what we can do better (previous
> version did a copy of pairs). I don't think we can build the heap here,
because
> it's too expensive. Making it cheaper is probably possible, but it will also
> probably involve some redesign. Please suggest an alternative.

We don't have to create the heap (call make_heap) but creating a vector of
iterators would be nice as not doing that requires storing some temporary
vector. A temporary vector makes the code less clean and it makes performance in
the general use case worse. Are you sure we need to re-design things to avoid a
temporary vector?

> 
> If making iterator creating cheap is the direction you're suggesting, then
most
> likely all of the pieces that have already landed will need to be changed. In
> that case, I think we have to schedule a meeting and discuss this in a larger
> setting (ie, back to the drawing board).

I think we should do both. Making iterators cheap to instantiate and not require
any heap allocations would be great as it improves performance of the general
use case and not just this specific "avoid building eviction queue unless used"
optimization. However, if there's a clean way to do this lazy work and that's
enough to land this then I'm OK with that. Problem is that I don't think the
current approach is good enough. It looks too much like an urgent short term fix
to get this landed without causing a major performance regression and less like
something written for good performance and code quality in the long term.

I'm just trying to suggest possible alternatives and explore possibilities for
how this can be done in a way that we both think is acceptable. Not having the
tile manager build the eviction queue unless it needs to use it is an acceptable
solution to me. And probably the easiest if you want to minimize the back and
forth needed before this can land. I'm happy to keep trying to figure out how we
can do this lazy work as part of the queue as well though.

If you think that the best we can do with the current design is basically what
you have here and we should already be trying to get the most out of it by
sacrificing code quality and performance of the general use case to gain some
for a specific use case, then I think it's best to get back to the drawing
board. I've been assuming that this is not the case.

https://codereview.chromium.org/367833003/diff/300001/cc/resources/eviction_t...
cc/resources/eviction_tile_priority_queue.cc:39:
paired_queues_.reserve(paired_picture_layers_.size());
On 2014/07/15 20:38:17, vmpstr wrote:
> On 2014/07/15 19:44:37, reveman wrote:
> > I don't understand this and the comment doesn't help. Why does this vector
not
> > grow just once to the size needed as the other vectors?
> 
> These pointers point to the vector's memory. If paired_queues_ is reallocated
> internally, it invalidates the pointers that have already been created.

Ah, I see. That's very subtle. Not using two vectors would be my preferred
solution but if we need this vector of pointers then I think building it using a
separate loop is better as I would have easily assumed that this was just an
optimization if not familiar with the code.

vmpstr

Still some testing to be done, but PTAL. This is so far what's left to ...

6 years, 4 months ago (2014-08-14 23:50:14 UTC) #29

reveman

not familiar with the occlusion code. let me know if you want me to review ...

6 years, 4 months ago (2014-08-17 18:26:06 UTC) #30

vmpstr

On 2014/08/17 18:26:06, reveman wrote: > not familiar with the occlusion code. let me know ...

6 years, 4 months ago (2014-08-18 18:04:52 UTC) #31

danakj

https://codereview.chromium.org/367833003/diff/320001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/367833003/diff/320001/cc/layers/picture_layer_impl.cc#newcode597 cc/layers/picture_layer_impl.cc:597: const PictureLayerImpl* layer = this; nit: active_layer https://codereview.chromium.org/367833003/diff/320001/cc/layers/picture_layer_impl.cc#newcode1266 cc/layers/picture_layer_impl.cc:1266: ...

6 years, 4 months ago (2014-08-18 19:29:14 UTC) #34

vmpstr

https://codereview.chromium.org/367833003/diff/320001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/367833003/diff/320001/cc/layers/picture_layer_impl.cc#newcode1266 cc/layers/picture_layer_impl.cc:1266: // TODO(vmpstr): Verify this is true if we create ...

6 years, 4 months ago (2014-08-18 19:49:52 UTC) #35

https://codereview.chromium.org/367833003/diff/320001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/367833003/diff/320001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1266: // TODO(vmpstr): Verify this is true if we
create tiles in raster
On 2014/08/18 19:29:13, danakj wrote:
> Is this going to maybe cause us to activate checkerboards? Or are we not
> creating tiles in raster iterators yet?

This is not the case yet, but would definitely be a follow-up optimization.

https://codereview.chromium.org/367833003/diff/320001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/367833003/diff/320001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1398:
EXPECT_TRUE(tile->required_for_activation());
On 2014/08/18 19:29:13, danakj wrote:
> Why isn't this doing tiling->IsTileRequiredForActivation(tile) like the
> PictureLayerImpl would? (This method is used all over this tile.)
> 
> Why does this even exist anymore? IsTileRequiredForActivation doesnt use it.

The idea, which is kind of subtle unfortunately is that required_for_activation,
priority, is_occluded on tile are only valid if the tile is returned using a
raster or eviction iterator. Otherwise, it might (and probably is) out of date.

I want to move all of those accesses into the iterator itself, so that it is not
exposed to any other code. One place that has to change is rasterizer that
accesses required_for_activation. It's fine do that in that case, since the tile
was retrieved using an iterator. I think ernstm is working on making those
notifications generic though, which would allow us to hide all of the
priority/required_for_activation/is_occluded accesses into the iterator. 

All those changes will of course come with various unittest fixes everywhere..

https://codereview.chromium.org/367833003/diff/320001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1543:
pending_layer_->HighResTiling()->UpdateTilePriorities(
On 2014/08/18 19:29:13, danakj wrote:
> Can you explain why this is needed here but UpdateAllTPForTesting is enough
> elsewhere?

This is for the tiling to get new bounds. All other tests don't modify a
property that needs the layer to update stuff.

https://codereview.chromium.org/367833003/diff/320001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/367833003/diff/320001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:610:
twin_tiling->TileAt(tile->tiling_i_index(), tile->tiling_j_index());
On 2014/08/18 19:29:13, danakj wrote:
> :( We do this every time we want to see if we can activate, for every tile in
> the visible content rect. This involves hash map lookups, and I feel like this
> is going to be crap for performance. Did you experience otherwise?
> 
> Perhaps we could investigate having the Tile know if it's shared?

I'll do this in a separate patch.

USE eero AT chromium.org

https://codereview.chromium.org/367833003/diff/320001/cc/resources/picture_layer_tiling.cc File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/367833003/diff/320001/cc/resources/picture_layer_tiling.cc#newcode488 cc/resources/picture_layer_tiling.cc:488: content_to_screen_scale_ = 1.0f / (contents_scale_ * ideal_contents_scale); This should ...

6 years, 4 months ago (2014-08-19 08:40:21 UTC) #36

vmpstr

https://codereview.chromium.org/367833003/diff/320001/cc/resources/picture_layer_tiling.cc File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/367833003/diff/320001/cc/resources/picture_layer_tiling.cc#newcode488 cc/resources/picture_layer_tiling.cc:488: content_to_screen_scale_ = 1.0f / (contents_scale_ * ideal_contents_scale); On 2014/08/19 ...

6 years, 4 months ago (2014-08-19 18:04:33 UTC) #37

vmpstr

I've rebased this, which was somewhat painful, so please take another (careful) look. I'll test ...

6 years, 3 months ago (2014-09-03 19:20:08 UTC) #38

vmpstr

On 2014/09/03 19:20:08, vmpstr wrote: > I've rebased this, which was somewhat painful, so please ...

6 years, 3 months ago (2014-09-04 20:57:04 UTC) #39

reveman

LGTM https://codereview.chromium.org/367833003/diff/380001/cc/layers/picture_layer_impl_unittest.cc File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/367833003/diff/380001/cc/layers/picture_layer_impl_unittest.cc#newcode3789 cc/layers/picture_layer_impl_unittest.cc:3789: tiling->UpdateAllTilePrioritiesForTesting(); 2 calls to UpdateAllTilePrioritiesForTesting intentional?

6 years, 3 months ago (2014-09-17 15:44:45 UTC) #41

vmpstr

PTAL. David, there's another change in the tile manager that I indicated if you wanted ...

6 years, 3 months ago (2014-09-18 18:35:09 UTC) #42

reveman

still lgtm https://codereview.chromium.org/367833003/diff/400001/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/367833003/diff/400001/cc/resources/tile_manager.cc#newcode379 cc/resources/tile_manager.cc:379: // would otherwise not be picked up ...

6 years, 3 months ago (2014-09-18 19:47:10 UTC) #43

danakj

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer_impl.cc#newcode532 cc/layers/picture_layer_impl.cc:532: layer_tree_impl()->DidModifyTilePriorities(); Can we do this from LayerTreeHostImpl directly and ...

6 years, 3 months ago (2014-09-19 01:41:48 UTC) #44

danakj

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_layer_tiling.cc File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_layer_tiling.cc#newcode684 cc/resources/picture_layer_tiling.cc:684: WhichTree tree = client_->GetTree(); can we DCHECK(PENDING_TREE) instead actually? ...

6 years, 3 months ago (2014-09-19 01:44:53 UTC) #45

vmpstr

PTAL https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer_impl.cc#newcode532 cc/layers/picture_layer_impl.cc:532: layer_tree_impl()->DidModifyTilePriorities(); On 2014/09/19 01:41:46, danakj wrote: > Can ...

6 years, 3 months ago (2014-09-19 21:22:53 UTC) #46

PTAL

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:532:
layer_tree_impl()->DidModifyTilePriorities();
On 2014/09/19 01:41:46, danakj wrote:
> Can we do this from LayerTreeHostImpl directly and remove this virtual method
> from existance? You can leave a TODO if you like.

I've tried to just call DidModifyTilePriorities from LTHI, but that failed
mostly because of tests, I suspect (a crash in unittests). I left a todo for
now, but will go back and try to make it work.

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:626: if (GetTree() == PENDING_TREE)
On 2014/09/19 01:41:46, danakj wrote:
> GetTree() is a virtual function call, how about just ask the layer_tree_impl()
> directly?

Done.

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1330: if
(tiling->IsTileRequiredForActivation(tile) && !tile->IsReadyToDraw())
On 2014/09/19 01:41:46, danakj wrote:
> The pieces all look good to me, but can you explain how they fit together?
What
> is doing the iterator walk and setting the priority on all the tiles before we
> come here?

The priority of the tile here isn't accessed (IsTileRequiredForActivation
doesn't care and IsReadyToDraw is independent of priority). If we were to update
the priority, then we'd find that it's in the NOW bin. That's done, because we
only do the coverage over the visible rect.

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1406: time_ticks +=
base::TimeDelta::FromMilliseconds(16);
On 2014/09/19 01:41:46, danakj wrote:
> 1 is enough right? avoid magic numbers, 1 is less magic than 16

*mumble*expected frame time*mumble*. Changed to 1.

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1686:
pending_layer_->HighResTiling()->UpdateAllTilePrioritiesForTesting();
On 2014/09/19 01:41:46, danakj wrote:
> can you update all tilings here before doing any iterating?

Done.

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1710:
pending_layer_->HighResTiling()->UpdateAllTilePrioritiesForTesting();
On 2014/09/19 01:41:46, danakj wrote:
> same here

Done.

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1730:
pending_layer_->HighResTiling()->UpdateAllTilePrioritiesForTesting();
On 2014/09/19 01:41:47, danakj wrote:
> etc..

Done.

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1805:
pending_layer_->HighResTiling()->UpdateTilePriorities(
On 2014/09/19 01:41:46, danakj wrote:
> This confused me again. Can you maybe reorder this code so this is grouped
with
> the SetBounds call and/or comment saying this it is to prioritize tiles at the
> new bounds?
> 
> Maybe even better would be to set up the pending tree at 200x200, activate,
then
> set up a new pending tree with a pile/layer/everything sized to 400x400 so we
> don't have weird function calls to private methods that will definitely need
> updating in the future?

I've added a TODO and the function name change. Let me know if it's better. I
wanna try avoiding bigger unittest changes so that I don't accidentally "fix" a
test to "pass" bad behaviour.

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:3789:
tiling->UpdateAllTilePrioritiesForTesting();
On 2014/09/19 01:41:46, danakj wrote:
> Why twice?

For good measure? :P Removed.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:534: void
PictureLayerTiling::UpdateTilePriorities(
On 2014/09/19 01:41:47, danakj wrote:
> this no longer updates tile priorities, and is super confusing in tests. can
you
> rename this? SaveSomeThingsBlahBlahSalmonPink()

I set this as ComputeTilePriorityRects. Let me know if the name is ok. Also let
me know if you want LTI::UpdateTilePriorities to change as well. I'm kind of on
the fence about it, since the action is to update tile priorities, although the
implementation details don't update it, just set the rects.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:599: const gfx::Rect& tiling_rect =
gfx::Rect(tiling_size());
On 2014/09/19 01:41:47, danakj wrote:
> why const&? just gfx::Rect tiling_rect(tiling_size()); ?

Done.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:668: gfx::Rect tile_query_rect =
ScaleToEnclosingRect(
On 2014/09/19 01:41:47, danakj wrote:
> This does int->float->int work. Can you do this only if contents_scale_ != 1?

Done.

> 
> Maybe also consider a current_occlusion_.HasOcclusion() early out so the
> renderer doesn't pay for the browser compositor's requirements here? Or TODO?

Added a TODO

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:671: // Explicitly check if the tile is
outside the viewport. If so, we need to
On 2014/09/19 01:41:47, danakj wrote:
> This isn't really right if current_visible_rect_ is the layer's
> visible_content_rect() which it is.. sometimes.
> 
> i'm not sure why we ever use the visible_content_rect() in tile
prioritization.
> the viewport projected to the layer seems strictly better.
> 
> Can you put a TODO that this is wrong for tiles clipped out but inside the
> viewport if current_visible_rect_ is not the viewport?

Done.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:676:
current_occlusion_in_layer_space_.IsOccluded(tile_query_rect);
On 2014/09/19 01:41:47, danakj wrote:
> just return this instead of using a variable?

Done.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:681: if (resolution_ != HIGH_RESOLUTION)
On 2014/09/19 01:41:47, danakj wrote:
> we used to consider tiles that were higher res than raster scale or ideal
scale
> as enough or so. can you add a TODO about we might want to do that?

Non ideal tilings won't be rasterizing any new content (as they are not
rasterizing right now either), so I'm not sure if it's worth it. It would only
really prevent the tiles from being evicted _somtimes_, which I feel should be a
different mechanism anyway.

> 
> also we used to require low res if the active tree had unshared low res there.
> what about that?

The problem is that when we did this all from PLI, it knew about
low-res/high-res at the same time. In PLT, it's kind of awkward to ask about
other tilings. So I'm not exactly sure what the correct approach is here.

> 
> do we not have tests for these cases?

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:684: WhichTree tree = client_->GetTree();
On 2014/09/19 01:41:47, danakj wrote:
> the temp variable not needed

Done.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:706: if (!twin_tile || twin_tile == tile)
On 2014/09/19 01:41:47, danakj wrote:
> there should be some readytodraw checks in here no? if the tile isn't ready to
> draw we should allow low res to be required?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_...
> 
> I changed that code recently, are the tests not testing anything anymore with
> this change? https://codereview.chromium.org/540663002

See above.. We did that to mark low res. Suggestions are welcome.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:740: if
(current_visible_rect_.Intersects(tile_bounds)) {
On 2014/09/19 01:41:47, danakj wrote:
> here i think we want the actual visible rect of the layer, not the viewport
> projected into the layer without any clipping.. i think this is currently bad
at
> overflow scroll for example. can you add a TODO about this?

I think this is OK? Tile bounds here are guaranteed to be within the content
bounds of the layer, so if this check passes then at least part of the viewport
in layer space is overlapping the content bounds. Does that make sense?

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:741: tile->SetPriority(tree,
TilePriority(resolution_, TilePriority::NOW, 0));
On 2014/09/19 01:41:47, danakj wrote:
> This priority logic is duplicated from the iterators, they decide the order
and
> the priority based on these rects. Then they call this to "SetPriority" to
what
> they decided. They should be the one setting this stuff or passing it in here
> based on their stage.
> 
> I think you commented elsewhere similarly about the is_occluded() and
> required_for_activation() being problematic similarly. We should definitely
> iterate (hah get it?) on this and make this better.
> 
> Can we improve this now, or leave a TODO about this?

I've added a general todo here. I want to move the priority to the iterators
altogether, which is a big change (mostly because all of the unittests will
simultaneously explode). I can move the setting code to the iterators, if that's
better. Let me know.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:954: UpdateTileAndTwinPriority(tile);
On 2014/09/19 01:41:47, danakj wrote:
> the tile and it's twin are both in this map right? does this mean we update
each
> of their priorities twice? is that worth worrying about?

Well.. you're right. But I'm not worried about it too much. I don't really know
of a better alternative aside from having some sort of "this tile priority has
been updated during this frame/manage tiles" sort of thing, which I'd like to
avoid.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_set.cc (right):

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:225: return NON_IDEAL_RESOLUTION;
On 2014/09/19 01:41:47, danakj wrote:
> can you NOTREACHED()?

Done.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:728: // 16 / 0.25 = 64 pixels.
On 2014/09/19 01:41:47, danakj wrote:
> This comment changed but the code didn't. Don't change the comment?

Whoops. I'm not sure how that crept in.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/tile.cc
File cc/resources/tile.cc (right):

https://codereview.chromium.org/367833003/diff/420001/cc/resources/tile.cc#ne...
cc/resources/tile.cc:51: void Tile::SetPriority(WhichTree tree, const
TilePriority& priority) {
On 2014/09/19 01:41:47, danakj wrote:
> move this to the header and remove the early out

Done.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/367833003/diff/420001/cc/resources/tile.h#new...
cc/resources/tile.h:68: bool is_occluded(WhichTree tree) const { return
is_occluded_[tree]; }
On 2014/09/19 01:41:47, danakj wrote:
> can you TODO about removing this and always using IsTileOccluded()? or
something
> along those lines.

Done.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/tile.h#new...
cc/resources/tile.h:87: bool required_for_activation() const { return
required_for_activation_; }
On 2014/09/19 01:41:47, danakj wrote:
> can you TODO to remove this and always use IsTileRequiredForActivation()?

Done.

https://codereview.chromium.org/367833003/diff/420001/cc/test/fake_picture_la...
File cc/test/fake_picture_layer_tiling_client.cc (right):

https://codereview.chromium.org/367833003/diff/420001/cc/test/fake_picture_la...
cc/test/fake_picture_layer_tiling_client.cc:95: return false;
On 2014/09/19 01:41:48, danakj wrote:
> this looks like a testing opportunity or like we have tests sitting at the
wrong
> layer of abstraction now. #randommusings

There are tests that test RequiresHighRes/activation things, iirc. They are
using the actual PLI as the client though.

danakj

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer_impl.cc#newcode1330 cc/layers/picture_layer_impl.cc:1330: if (tiling->IsTileRequiredForActivation(tile) && !tile->IsReadyToDraw()) On 2014/09/19 21:22:52, vmpstr wrote: ...

6 years, 3 months ago (2014-09-19 22:30:06 UTC) #47

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1330: if
(tiling->IsTileRequiredForActivation(tile) && !tile->IsReadyToDraw())
On 2014/09/19 21:22:52, vmpstr wrote:
> On 2014/09/19 01:41:46, danakj wrote:
> > The pieces all look good to me, but can you explain how they fit together?
> What
> > is doing the iterator walk and setting the priority on all the tiles before
we
> > come here?
> 
> The priority of the tile here isn't accessed (IsTileRequiredForActivation
> doesn't care and IsReadyToDraw is independent of priority). If we were to
update
> the priority, then we'd find that it's in the NOW bin. That's done, because we
> only do the coverage over the visible rect.

Oh ok. It'd be nice if I could tell from the function name if it uses the tile's
priority or not, maybe. Or maybe this gets more clear when priority is always
through the iterators, so okay, let's work on that next.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:681: if (resolution_ != HIGH_RESOLUTION)
On 2014/09/19 21:22:53, vmpstr wrote:
> On 2014/09/19 01:41:47, danakj wrote:
> > we used to consider tiles that were higher res than raster scale or ideal
> scale
> > as enough or so. can you add a TODO about we might want to do that?
> 
> Non ideal tilings won't be rasterizing any new content (as they are not
> rasterizing right now either), so I'm not sure if it's worth it. It would only
> really prevent the tiles from being evicted _somtimes_, which I feel should be
a
> different mechanism anyway.

Is that implemented as a different mechanism now? Can you point me to it?

> > also we used to require low res if the active tree had unshared low res
there.
> > what about that?
> 
> The problem is that when we did this all from PLI, it knew about
> low-res/high-res at the same time. In PLT, it's kind of awkward to ask about
> other tilings. So I'm not exactly sure what the correct approach is here.

This function is called from PictureLayerImpl.. so we should be able to pass in
what info we need or make the decision at that callsite.

Right now PLI::AllTilesRequiredReady is using a CoverageIterator on each tiling.
If any high res tile is missing, it could iterate on the low res and check them.
If all the high res tiles are ready it could just not call this method at all
for LOW_RES. So maybe this resolution check doesn't belong in this method at
all, and PLI should be making choices based on the resolution. WDYT?

It could also do the missing_region thing for non-ideal resolutions like we do
in mark visible to maintain letting us activate early with those resources at
the right times?

> > 
> > do we not have tests for these cases?
>

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:740: if
(current_visible_rect_.Intersects(tile_bounds)) {
On 2014/09/19 21:22:52, vmpstr wrote:
> On 2014/09/19 01:41:47, danakj wrote:
> > here i think we want the actual visible rect of the layer, not the viewport
> > projected into the layer without any clipping.. i think this is currently
bad
> at
> > overflow scroll for example. can you add a TODO about this?
> 
> I think this is OK? Tile bounds here are guaranteed to be within the content
> bounds of the layer, so if this check passes then at least part of the
viewport
> in layer space is overlapping the content bounds. Does that make sense?

Yes but consider a small overflow scroll div clipping stuff in teh middle of the
viewport, and a larger layer being clipped by that. It's all inside the
viewport, but it's mostly not visible at all, so we say NOW for tiles that are
entirely clipped away by the overflow scroll div.

https://codereview.chromium.org/367833003/diff/420001/cc/test/fake_picture_la...
File cc/test/fake_picture_layer_tiling_client.cc (right):

https://codereview.chromium.org/367833003/diff/420001/cc/test/fake_picture_la...
cc/test/fake_picture_layer_tiling_client.cc:95: return false;
On 2014/09/19 21:22:53, vmpstr wrote:
> On 2014/09/19 01:41:48, danakj wrote:
> > this looks like a testing opportunity or like we have tests sitting at the
> wrong
> > layer of abstraction now. #randommusings
> 
> There are tests that test RequiresHighRes/activation things, iirc. They are
> using the actual PLI as the client though.

ya, that no longer seems right to me. it'll make those tests more
fragile/complex than they need to be.

vmpstr

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer_impl.cc#newcode1330 cc/layers/picture_layer_impl.cc:1330: if (tiling->IsTileRequiredForActivation(tile) && !tile->IsReadyToDraw()) On 2014/09/19 22:30:06, danakj wrote: ...

6 years, 3 months ago (2014-09-22 18:02:00 UTC) #48

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/367833003/diff/420001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1330: if
(tiling->IsTileRequiredForActivation(tile) && !tile->IsReadyToDraw())
On 2014/09/19 22:30:06, danakj wrote:
> On 2014/09/19 21:22:52, vmpstr wrote:
> > On 2014/09/19 01:41:46, danakj wrote:
> > > The pieces all look good to me, but can you explain how they fit together?
> > What
> > > is doing the iterator walk and setting the priority on all the tiles
before
> we
> > > come here?
> > 
> > The priority of the tile here isn't accessed (IsTileRequiredForActivation
> > doesn't care and IsReadyToDraw is independent of priority). If we were to
> update
> > the priority, then we'd find that it's in the NOW bin. That's done, because
we
> > only do the coverage over the visible rect.
> 
> Oh ok. It'd be nice if I could tell from the function name if it uses the
tile's
> priority or not, maybe. Or maybe this gets more clear when priority is always
> through the iterators, so okay, let's work on that next.

Yeah, I want to start kind of assuming that we never use tile priority and make
it explicit when we do. I would prefer something like
GetIsTileRequiredForActivationFromTilePriority if it accessed priority. You know
what I mean? I don't know if that's the case right now, since I just came up
with that, but I think that would be the direction I prefer.

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:681: if (resolution_ != HIGH_RESOLUTION)
On 2014/09/19 22:30:06, danakj wrote:
> On 2014/09/19 21:22:53, vmpstr wrote:
> > On 2014/09/19 01:41:47, danakj wrote:
> > > we used to consider tiles that were higher res than raster scale or ideal
> > scale
> > > as enough or so. can you add a TODO about we might want to do that?
> > 
> > Non ideal tilings won't be rasterizing any new content (as they are not
> > rasterizing right now either), so I'm not sure if it's worth it. It would
only
> > really prevent the tiles from being evicted _somtimes_, which I feel should
be
> a
> > different mechanism anyway.
> 
> Is that implemented as a different mechanism now? Can you point me to it?

This is where we don't rasterize new non-ideal tiles:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/tile_...

As for eviction, required for activation would bump the tiles up in the priority
order, which would make them be evicted less frequently. But other than that, if
we're out of memory then we're out of memory; we'll evict anything to stay
within budget.

> 
> > > also we used to require low res if the active tree had unshared low res
> there.
> > > what about that?
> > 
> > The problem is that when we did this all from PLI, it knew about
> > low-res/high-res at the same time. In PLT, it's kind of awkward to ask about
> > other tilings. So I'm not exactly sure what the correct approach is here.
> 
> This function is called from PictureLayerImpl.. so we should be able to pass
in
> what info we need or make the decision at that callsite.
> 
> Right now PLI::AllTilesRequiredReady is using a CoverageIterator on each
tiling.
> If any high res tile is missing, it could iterate on the low res and check
them.
> If all the high res tiles are ready it could just not call this method at all
> for LOW_RES. So maybe this resolution check doesn't belong in this method at
> all, and PLI should be making choices based on the resolution. WDYT?
> 
> It could also do the missing_region thing for non-ideal resolutions like we do
> in mark visible to maintain letting us activate early with those resources at
> the right times?

The call to check whether we are ready to activate comes from PLI. However, the
thing to actually set it and schedule it with required for activation flag on
(so we get the signal from the rasterizer) is done via the raster iterator.
Granted, that raster iterator is also created from a layer raster iterator, but
I think it's a bit more involved that just passing the uncovered area.

I'm a bit conflicted here. I don't want to have only half of the functionality
(ie required checks do low res, but required setters don't). Also, I feel like
this would complicate the patch a bit, mostly because the layer iterator right
now doesn't care about required for activation -- it just uses the tiling
iterator. 

> 
> > > 
> > > do we not have tests for these cases?
> > 
>

https://codereview.chromium.org/367833003/diff/420001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:740: if
(current_visible_rect_.Intersects(tile_bounds)) {
On 2014/09/19 22:30:06, danakj wrote:
> On 2014/09/19 21:22:52, vmpstr wrote:
> > On 2014/09/19 01:41:47, danakj wrote:
> > > here i think we want the actual visible rect of the layer, not the
viewport
> > > projected into the layer without any clipping.. i think this is currently
> bad
> > at
> > > overflow scroll for example. can you add a TODO about this?
> > 
> > I think this is OK? Tile bounds here are guaranteed to be within the content
> > bounds of the layer, so if this check passes then at least part of the
> viewport
> > in layer space is overlapping the content bounds. Does that make sense?
> 
> Yes but consider a small overflow scroll div clipping stuff in teh middle of
the
> viewport, and a larger layer being clipped by that. It's all inside the
> viewport, but it's mostly not visible at all, so we say NOW for tiles that are
> entirely clipped away by the overflow scroll div.

(\sciencedog) But wouldn't we have the visible layer rect be the same thing as
the projected viewport clipped to this tiling bounds? Like I'm trying to
understand if this is different from taking current_visible_rect_ and then
clipping it to tiling bounds. I don't think it makes a difference, since
tile_bounds is already naturally clipped to those bounds so the intersection
check will not change value... Let me know if I completely misunderstand what
you mean :P

vmpstr

Friendly ping. There's no urgency here, but I do want to get this in way ...

6 years, 2 months ago (2014-10-06 19:02:11 UTC) #50

danakj

LGTM here's my last comments :) https://codereview.chromium.org/367833003/diff/540001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/367833003/diff/540001/cc/layers/picture_layer_impl.cc#newcode525 cc/layers/picture_layer_impl.cc:525: tiling->set_can_require_tiles_for_activation( is there ...

6 years, 2 months ago (2014-10-08 18:46:27 UTC) #51

vmpstr

https://codereview.chromium.org/367833003/diff/540001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/367833003/diff/540001/cc/layers/picture_layer_impl.cc#newcode525 cc/layers/picture_layer_impl.cc:525: tiling->set_can_require_tiles_for_activation( On 2014/10/08 18:46:25, danakj wrote: > is there ...

6 years, 2 months ago (2014-10-08 22:38:35 UTC) #52

danakj

https://codereview.chromium.org/367833003/diff/540001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/367833003/diff/540001/cc/layers/picture_layer_impl.cc#newcode525 cc/layers/picture_layer_impl.cc:525: tiling->set_can_require_tiles_for_activation( On 2014/10/08 22:38:34, vmpstr wrote: > On 2014/10/08 ...

6 years, 2 months ago (2014-10-08 23:38:01 UTC) #53

danakj

https://codereview.chromium.org/367833003/diff/540001/cc/resources/picture_layer_tiling.cc File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/367833003/diff/540001/cc/resources/picture_layer_tiling.cc#newcode1067 cc/resources/picture_layer_tiling.cc:1067: tiling_->UpdateTileAndTwinPriority(current_tile_); On 2014/10/08 23:38:01, danakj wrote: > On 2014/10/08 ...

6 years, 2 months ago (2014-10-08 23:39:27 UTC) #54

vmpstr

https://codereview.chromium.org/367833003/diff/540001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/367833003/diff/540001/cc/layers/picture_layer_impl.cc#newcode525 cc/layers/picture_layer_impl.cc:525: tiling->set_can_require_tiles_for_activation( On 2014/10/08 23:38:01, danakj wrote: > On 2014/10/08 ...

6 years, 2 months ago (2014-10-09 00:01:42 UTC) #55

https://codereview.chromium.org/367833003/diff/540001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/367833003/diff/540001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:525:
tiling->set_can_require_tiles_for_activation(
On 2014/10/08 23:38:01, danakj wrote:
> On 2014/10/08 22:38:34, vmpstr wrote:
> > On 2014/10/08 18:46:25, danakj wrote:
> > > is there a test that ensures we activate without requiring when we drew
with
> > > only low res/checkerboard?
> > 
> > Not that one in particular.. I've added a test to ensure that we don't have
> > anything marked as required if we disallow it though.
> 
> OK, do we have a test that verifies we save the "only used low res" flag
> correctly? We've regressed similar flags in AppendQuads before, recently, so
I'd
> feel better with coverage of that.

Makes sense, I'll add a test.

https://codereview.chromium.org/367833003/diff/540001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/367833003/diff/540001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:1067:
tiling_->UpdateTileAndTwinPriority(current_tile_);
On 2014/10/08 23:39:27, danakj wrote:
> On 2014/10/08 23:38:01, danakj wrote:
> > On 2014/10/08 22:38:34, vmpstr wrote:
> > > On 2014/10/08 18:46:26, danakj wrote:
> > > > while iterating you always update the priority of the tile before
> returning,
> > > but
> > > > here you don't if the first tile doesn't need raster. should this be
> > > consistent?
> > > > could this cause bugs?
> > > 
> > > The priority is only updated for returned values. That's true of the rest
of
> > the
> > > code here as well. For instance, operator++ has something along the lines
of
> > > 
> > > while (!current_tile_ || !TileNeedsRaster(current_tile_)) {
> > >  ...
> > > }
> > > 
> > > if (current_tile_)
> > >   tiling_->UpdateTileAndTwinPriority(current_tile_);
> > 
> > Right, but if the first tile in the iterator didn't need raster, wouldn't we
> > have a current_tile_ (ie operator* would have a tile) but it's priorities
> > wouldn't be updated?
> 
> Or would operator bool be false? I'm not sure why it would?

operator* can only return a tile that needs raster for this iterator.. That is,
we skip all tiles that don't need raster silently. If we run out of tiles,
current_tile_ becomes NULL, which is what operator bool checks. 

So basically, we can't have a situation where we would return a tile that
doesn't have its priorities updated and at the same time we can't return a tile
that doesn't need raster. All other cases are consistent in that we don't update
the tile priorities... At least that's the design.

danakj

https://codereview.chromium.org/367833003/diff/540001/cc/resources/picture_layer_tiling.cc File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/367833003/diff/540001/cc/resources/picture_layer_tiling.cc#newcode1067 cc/resources/picture_layer_tiling.cc:1067: tiling_->UpdateTileAndTwinPriority(current_tile_); On 2014/10/09 00:01:42, vmpstr wrote: > On 2014/10/08 ...

6 years, 2 months ago (2014-10-09 00:03:26 UTC) #56

https://codereview.chromium.org/367833003/diff/540001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/367833003/diff/540001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:1067:
tiling_->UpdateTileAndTwinPriority(current_tile_);
On 2014/10/09 00:01:42, vmpstr wrote:
> On 2014/10/08 23:39:27, danakj wrote:
> > On 2014/10/08 23:38:01, danakj wrote:
> > > On 2014/10/08 22:38:34, vmpstr wrote:
> > > > On 2014/10/08 18:46:26, danakj wrote:
> > > > > while iterating you always update the priority of the tile before
> > returning,
> > > > but
> > > > > here you don't if the first tile doesn't need raster. should this be
> > > > consistent?
> > > > > could this cause bugs?
> > > > 
> > > > The priority is only updated for returned values. That's true of the
rest
> of
> > > the
> > > > code here as well. For instance, operator++ has something along the
lines
> of
> > > > 
> > > > while (!current_tile_ || !TileNeedsRaster(current_tile_)) {
> > > >  ...
> > > > }
> > > > 
> > > > if (current_tile_)
> > > >   tiling_->UpdateTileAndTwinPriority(current_tile_);
> > > 
> > > Right, but if the first tile in the iterator didn't need raster, wouldn't
we
> > > have a current_tile_ (ie operator* would have a tile) but it's priorities
> > > wouldn't be updated?
> > 
> > Or would operator bool be false? I'm not sure why it would?
> 
> operator* can only return a tile that needs raster for this iterator.. That
is,
> we skip all tiles that don't need raster silently. If we run out of tiles,
> current_tile_ becomes NULL, which is what operator bool checks. 
> 
> So basically, we can't have a situation where we would return a tile that
> doesn't have its priorities updated and at the same time we can't return a
tile
> that doesn't need raster. All other cases are consistent in that we don't
update
> the tile priorities... At least that's the design.

Oh, ok ++ above would end up with a NULL tile in that case then I guess. OK
thanks!

vmpstr

Added the only used low res test, if you wanted to take a look. https://codereview.chromium.org/367833003/diff/600001/cc/layers/picture_layer_impl_unittest.cc ...

6 years, 2 months ago (2014-10-09 00:27:27 UTC) #57

commit-bot: I haz the power

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

6 years, 2 months ago (2014-10-09 19:37:05 UTC) #60

commit-bot: I haz the power

6 years, 2 months ago (2014-10-09 20:17:36 UTC) #62

Message was sent while issue was closed.

Patchset 32 (id:??) landed as
https://crrev.com/56ace236c9fc5e0f312818c870a56d373f143db5
Cr-Commit-Position: refs/heads/master@{#298962}

Issue 367833003: cc: Start using raster/eviction iterators. (Closed)

Description

Patch Set 1 #

Patch Set 2 : broken tests #

Patch Set 3 : debug #

Patch Set 4 : fixes #

Patch Set 5 : #

Patch Set 6 : update #

Patch Set 7 : update #

Patch Set 8 : update #

Patch Set 9 : review #

Patch Set 10 : rebase #

Patch Set 11 : update #

Patch Set 12 : perf test fix #

Patch Set 13 : update #

Patch Set 14 : #

Patch Set 15 : update #

Patch Set 16 : update #

Patch Set 17 : #

Patch Set 18 : rebase #

Patch Set 19 : rebase #

Patch Set 20 : #

Patch Set 21 : rebase + fix #

Patch Set 22 : rebase #

Patch Set 23 : update #

Patch Set 24 : #

Patch Set 25 : update #

Patch Set 26 : update #

Patch Set 27 : git log #

Patch Set 28 : rebase #

Patch Set 29 : rebase #

Patch Set 30 : comments #

Patch Set 31 : +test #

Patch Set 32 : rebase #

Messages