cc: Reset tile priority before deleting tiling.

cc: Reset tile priority before deleting tiling.

BUG=251938
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208880

[reveman, 2013-06-26 14:21:32]

Please take a look. There's probably more cases where we need to reset the tile
priority but maybe we should do something more reliable like change TileMap to:

typedef base::hash_map<TileMapKey, PrioritizedTile> TileMap;

where PrioritizedTile just contains a Tile ref and resets tile priority in its
dtor.

Wdyt?

[enne (OOO), 2013-06-26 15:39:27]

On 2013/06/26 14:21:32, David Reveman wrote:
> Please take a look. There's probably more cases where we need to reset the
tile
> priority but maybe we should do something more reliable like change TileMap
to:
> 
> typedef base::hash_map<TileMapKey, PrioritizedTile> TileMap;
> 
> where PrioritizedTile just contains a Tile ref and resets tile priority in its
> dtor.

I don't like trusting ctor/dtor logic inside of a container, since it may happen
just as a part of growing/shrinking/rearranging that container.

One other option that we've previously talked about is having the tile priority
also store a frame number that it's valid for, such that if a priority doesn't
get updated then it is automatically no longer valid.  On the upside, we
wouldn't have to do update priorities during tiling destruction or during
activation.  On the downside, this precludes any optimizations to limit the rate
at which we update priorities.  It's just a thought.

https://codereview.chromium.org/17891002/diff/1/cc/resources/picture_layer_ti...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/17891002/diff/1/cc/resources/picture_layer_ti...
cc/resources/picture_layer_tiling.cc:60: it->second->SetPriority(ACTIVE_TREE,
TilePriority());
This tiling is only on one tree, so you shouldn't clear both.

[vmpstr, 2013-06-26 17:16:28]

On 2013/06/26 15:39:27, enne wrote:
> On 2013/06/26 14:21:32, David Reveman wrote:
> > Please take a look. There's probably more cases where we need to reset the
> tile
> > priority but maybe we should do something more reliable like change TileMap
> to:
> > 
> > typedef base::hash_map<TileMapKey, PrioritizedTile> TileMap;
> > 
> > where PrioritizedTile just contains a Tile ref and resets tile priority in
its
> > dtor.
> 
> I don't like trusting ctor/dtor logic inside of a container, since it may
happen
> just as a part of growing/shrinking/rearranging that container.
> 
> One other option that we've previously talked about is having the tile
priority
> also store a frame number that it's valid for, such that if a priority doesn't
> get updated then it is automatically no longer valid.  On the upside, we
> wouldn't have to do update priorities during tiling destruction or during
> activation.  On the downside, this precludes any optimizations to limit the
rate
> at which we update priorities.  It's just a thought.
> 
>
https://codereview.chromium.org/17891002/diff/1/cc/resources/picture_layer_ti...
> File cc/resources/picture_layer_tiling.cc (right):
> 
>
https://codereview.chromium.org/17891002/diff/1/cc/resources/picture_layer_ti...
> cc/resources/picture_layer_tiling.cc:60: it->second->SetPriority(ACTIVE_TREE,
> TilePriority());
> This tiling is only on one tree, so you shouldn't clear both.

Do we always update all tiles priorities when we update at least one? If that's
the case we could store the last update count (instead of frame number), so that
we don't have to update every frame and tile manager can weed out tiles with
update count < max(update count)

[reveman, 2013-06-26 17:55:45]

Added a DestroyTile client function to latest patch. PTAL.

Long term I think we can remove the ref counting from the tile manager and just
have tiles deleted and unregistered from the tile manager when no longer
referenced by a tiling.

[enne (OOO), 2013-06-26 17:58:30]

Do you think it'd be worth DCHECKing in ~Tile to make sure the priority was set
correctly?  Alternatively, in TileManager::UnregisterTile, which might be a more
test-friendly solution?

[enne (OOO), 2013-06-26 18:04:53]

Actually, ignore that thought.  This issue is about the TileManager holding onto
refs, so neither of those suggestions make any sense.

This lgtm, and I like the longer term plan.

[commit-bot: I haz the power, 2013-06-26 19:53:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/17891002/6001

[commit-bot: I haz the power, 2013-06-26 20:18:16]

Retried try job too often on win_x64_rel for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-06-26 21:12:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/17891002/24002

[commit-bot: I haz the power, 2013-06-26 21:23:36]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-06-27 09:01:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/17891002/24002

[commit-bot: I haz the power, 2013-06-27 09:55:36]

Change committed as 208880