cc: Improve tile deletion and general tile manager performance.

cc: Improve tile deletion and general tile manager performance.

Separate managed tile state from Tile class so that the tile manager
can more efficiently control what tiles to manage cleanly handle
tile deletion.

BUG=165852

[reveman, 2013-01-30 23:39:26]

This is work in progress and doesn't do anything to improve performance at this
point. Just wanted to get this up in case you wanted to leave some early
feedback.

[reveman, 2013-02-05 01:36:09]

Latest patch fixes tile deletion performance and respects the Tile::is_live
property. Only tile with is_live set to true are sorted and considered for
raster.

[nduca, 2013-02-05 06:42:58]

Do you think this is landable?

[nduca, 2013-02-05 06:50:00]

Isn't there a slightly simpler form where we still register Tile pointers, store
them in a map, and address the is_live by only sorting/managing on the live
vector? Yes we still have to delete things from lists on delete, but I'm not
sure I can fully grok this type of change so close to stable release.

https://codereview.chromium.org/12082086/diff/2001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12082086/diff/2001/cc/tile_manager.cc#newcode106
cc/tile_manager.cc:106: bytes_consumed_if_allocated(0),
I'm worried about the duplication of state here. This code is hard to follow as
it is. What necessitates this?

[reveman, 2013-02-05 18:04:51]

I think this is what we want long term not sure it's something we can land this
close to a stable release.

There might be a simpler version of this that just avoids sorting tiles with
is_live not set but I'm not sure how much that will buy us. afaik, the only
tiles we'll avoid sorting that way are the ones the tile manager currently hold
references to (tiles with pending raster or upload).

https://codereview.chromium.org/12082086/diff/2001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12082086/diff/2001/cc/tile_manager.cc#newcode106
cc/tile_manager.cc:106: bytes_consumed_if_allocated(0),
On 2013/02/05 06:50:01, nduca wrote:
> I'm worried about the duplication of state here. This code is hard to follow
as
> it is. What necessitates this?

With this patch ManagedTileState can now outlive a Tile so all state that the
tile manager uses needs to be duplicated. This is how the deletion problem is
solved. Destroying a tile just drops its managed state ref while the tile
manager can keep running. The managed state will be deleted when ManageTiles is
called.

Before this patch some state is computed/duplicated in ManageTiles and some tile
state can change without ManageTiles being called. I think that having one place
where the tile manager updates its knowledge of tiles helps.  E.g. without this
patch a tile can be visible and high res when ManagedTiles is called and
prioritized as such but suddenly when the upload completes it's considered
visible and high res. I've found that very hard to follow when debugging this
code.

[whunt, 2013-02-05 18:58:19]

On 2013/02/05 18:04:51, David Reveman wrote:
> I think this is what we want long term not sure it's something we can land
this
> close to a stable release.
> 
> There might be a simpler version of this that just avoids sorting tiles with
> is_live not set but I'm not sure how much that will buy us. afaik, the only
> tiles we'll avoid sorting that way are the ones the tile manager currently
hold
> references to (tiles with pending raster or upload).
> 
> https://codereview.chromium.org/12082086/diff/2001/cc/tile_manager.cc
> File cc/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/12082086/diff/2001/cc/tile_manager.cc#newcode106
> cc/tile_manager.cc:106: bytes_consumed_if_allocated(0),
> On 2013/02/05 06:50:01, nduca wrote:
> > I'm worried about the duplication of state here. This code is hard to follow
> as
> > it is. What necessitates this?
> 
> With this patch ManagedTileState can now outlive a Tile so all state that the
> tile manager uses needs to be duplicated. This is how the deletion problem is
> solved. Destroying a tile just drops its managed state ref while the tile
> manager can keep running. The managed state will be deleted when ManageTiles
is
> called.
> 
> Before this patch some state is computed/duplicated in ManageTiles and some
tile
> state can change without ManageTiles being called. I think that having one
place
> where the tile manager updates its knowledge of tiles helps.  E.g. without
this
> patch a tile can be visible and high res when ManagedTiles is called and
> prioritized as such but suddenly when the upload completes it's considered
> visible and high res. I've found that very hard to follow when debugging this
> code.

I agree this looks like a better long term approach.  I've just uploaded a
stop-gap change list that should be easier to review and at least uses is_live. 
https://codereview.chromium.org/12213019

[nduca, 2013-02-13 09:24:23]

Looks like whunt/ccameron are systematically pecking away at this btw. I think
we may be able to close this.

[ccameron, 2013-02-16 08:21:17]

Having thought about this some more, this patch is really really really the way
to go.

[epenner, 2013-02-22 21:46:27]

Just an extra two-cents now that I understand this patch a bit better. 

It feels like we want to allow creation/deletion/managing of tiles to happen in
an arbitrary non-deterministic order, so this code 'commits' to a snapshot of
that chaos. It looks like the non-deterministic ordering comes from the
scheduler and async tasks calling things in arbitrary orders. Should we
considering limiting the number of operations that can happen out of order?  It
seems like in practice almost everything just needs to happen in a deterministic
order once per frame.