cc: Remove tile ref counting in tile manager.

cc: Remove tile ref counting in tile manager.

Instead of increasing a ref count on a tile when passing a task
into raster worker pool, use a unique id and a hash map. This allows
tiles to be deleted even when raster task is working on rasterizing
a resource.

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

[vmpstr, 2013-07-02 22:34:13]

Please take a look.

[reveman, 2013-07-03 00:32:47]

This eliminates the need for PictureLayerTilingClient::DestroyTile. Please
remove it as part of this change. Basically just revert
https://chromiumcodereview.appspot.com/17891002

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

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile.h#newcod...
cc/resources/tile.h:157: bool can_use_lcd_text_;
nit: maybe a blank line here.

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

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:776: ManagedTileId managed_id,
Lets use the same type name for the Id everywhere. Preferably Tile::TileId.

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:114: typedef uint64 ManagedTileId;
can you move this to the Tile class instead? Tile::TileId.

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:151: TileVector tiles_;
I think we can remove |tiles_|. It's just local to ManageTiles().

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:152: TileVector tiles_that_need_to_be_rasterized_;
|tiles_that_need_to_be_rasterized_| definitely doesn't have to be a member
variable. Do you mind fixing that as part of this patch?

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:154: TileSet
tiles_that_need_to_be_initialized_for_activation_;
fyi, I think we can remove this once my "ready to activate" patch lands.

In general, we shouldn't store any "Tile*" after we add this TileMap. Preferably
existing "Tile*" containers are removed. The ones that can't be removed should
store Tile::TileId instead.

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:169: typedef base::hash_map<ManagedTileId, Tile*>
ManagedTileMap;
I like naming these based on the value type. How about just TileMap?

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:170: ManagedTileMap managed_tile_map_;
we usually don't use the type in the name for these containers. How about
"TileMap managed_tiles_"? Or maybe just |tiles_| after removing the old vector
with this name.

[vmpstr, 2013-07-03 19:08:03]

PTAL.

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

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile.h#newcod...
cc/resources/tile.h:157: bool can_use_lcd_text_;
On 2013/07/03 00:32:47, David Reveman wrote:
> nit: maybe a blank line here.

Done.

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:114: typedef uint64 ManagedTileId;
On 2013/07/03 00:32:47, David Reveman wrote:
> can you move this to the Tile class instead? Tile::TileId.

I added a todo here. Right now doing this would cause a circular dependency
between tile_manager.h and tile.h. This should be fixed shortly, and then I will
do what you're suggesting.

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:151: TileVector tiles_;
On 2013/07/03 00:32:47, David Reveman wrote:
> I think we can remove |tiles_|. It's just local to ManageTiles().

It's also used in DispatchMoreTasks (this avoids an extra sort by priority). I
agree that it should be possible to remove this, but I think this requires a bit
of thought and code rework not to regress on efficiency. Are you ok with this
being a follow-up?

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:152: TileVector tiles_that_need_to_be_rasterized_;
On 2013/07/03 00:32:47, David Reveman wrote:
> |tiles_that_need_to_be_rasterized_| definitely doesn't have to be a member
> variable. Do you mind fixing that as part of this patch?

Done.

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:154: TileSet
tiles_that_need_to_be_initialized_for_activation_;
On 2013/07/03 00:32:47, David Reveman wrote:
> fyi, I think we can remove this once my "ready to activate" patch lands.
> 
> In general, we shouldn't store any "Tile*" after we add this TileMap.
Preferably
> existing "Tile*" containers are removed. The ones that can't be removed should
> store Tile::TileId instead. 

Sounds good. I think we would have to rethink the flow of how we throttle things
based on available memory, but I think this is work worth doing

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:169: typedef base::hash_map<ManagedTileId, Tile*>
ManagedTileMap;
On 2013/07/03 00:32:47, David Reveman wrote:
> I like naming these based on the value type. How about just TileMap?

Done.

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:170: ManagedTileMap managed_tile_map_;
On 2013/07/03 00:32:47, David Reveman wrote:
> we usually don't use the type in the name for these containers. How about
> "TileMap managed_tiles_"? Or maybe just |tiles_| after removing the old vector
> with this name.

Named it managed_tiles_ for now.

[reveman, 2013-07-03 20:20:06]

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:114: typedef uint64 ManagedTileId;
On 2013/07/03 19:08:04, vmpstr wrote:
> On 2013/07/03 00:32:47, David Reveman wrote:
> > can you move this to the Tile class instead? Tile::TileId.
> 
> I added a todo here. Right now doing this would cause a circular dependency
> between tile_manager.h and tile.h. This should be fixed shortly, and then I
will
> do what you're suggesting.

Ah, lets just make sure this is fixed before landing this patch.

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:151: TileVector tiles_;
On 2013/07/03 19:08:04, vmpstr wrote:
> On 2013/07/03 00:32:47, David Reveman wrote:
> > I think we can remove |tiles_|. It's just local to ManageTiles().
> 
> It's also used in DispatchMoreTasks (this avoids an extra sort by priority). I
> agree that it should be possible to remove this, but I think this requires a
bit
> of thought and code rework not to regress on efficiency. Are you ok with this
> being a follow-up?

I think we need include some of it in this patch. How about we keep this vector
but instead of erasing elements from it we just mark it as invalid in
Register/UnregisterTile?

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:154: TileSet
tiles_that_need_to_be_initialized_for_activation_;
On 2013/07/03 19:08:04, vmpstr wrote:
> On 2013/07/03 00:32:47, David Reveman wrote:
> > fyi, I think we can remove this once my "ready to activate" patch lands.
> > 
> > In general, we shouldn't store any "Tile*" after we add this TileMap.
> Preferably
> > existing "Tile*" containers are removed. The ones that can't be removed
should
> > store Tile::TileId instead. 
> 
> Sounds good. I think we would have to rethink the flow of how we throttle
things
> based on available memory, but I think this is work worth doing

I think it's trivial to replace
|tiles_that_need_to_be_initialized_for_activation_| with a simple "bool
tiles_need_to_be_initialized_for_activation_" now that my patch has landed. We
can do that as part of this patch or land it in a separate patch before this.

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:160: tiles_.push_back(tile);
I prefer if this becomes |sorted_tiles_| as that better represents what it is
and will help prevent us from using it when not valid.

in here we could just "sorted_tiles_need_update_ = true" or something like that

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:170: tiles_.erase(std::remove(tiles_.begin(),
tiles_.end(), tile));
"sorted_tiles_need_update_ = true" or whatever we decide

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:174: managed_tiles_.erase(managed_iter);
I prefer:
DCHECK(tiles_.find(tile->id()) != tiles_.end());
tiles_.erase(tile->id());

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.h:161: typedef std::set<Tile*> TileSet;
move this below TileVector tiles_;

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.h:162: TileVector tiles_;
how about sorted_tiles_?

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.h:164: TileSet
oom_tiles_that_need_to_be_initialized_for_activation_;
|oom_tiles_that_need_to_be_initialized_for_activation_| doesn't have to be a
member. It can be local to functions calling AssignGpuMem and passed as
parameter to that AssignGpuMem

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.h:180: TileMap managed_tiles_;
This is now the most important container of this class. I really think we should
move it to the top and call it |tiles_|.

[vmpstr, 2013-07-03 23:47:51]

I'm still testing this, but you can take a look to see if that's the direction
you had in mind.

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:114: typedef uint64 ManagedTileId;
On 2013/07/03 20:20:06, David Reveman wrote:
> On 2013/07/03 19:08:04, vmpstr wrote:
> > On 2013/07/03 00:32:47, David Reveman wrote:
> > > can you move this to the Tile class instead? Tile::TileId.
> > 
> > I added a todo here. Right now doing this would cause a circular dependency
> > between tile_manager.h and tile.h. This should be fixed shortly, and then I
> will
> > do what you're suggesting.
> 
> Ah, lets just make sure this is fixed before landing this patch.

Agreed.

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:151: TileVector tiles_;
On 2013/07/03 20:20:06, David Reveman wrote:
> On 2013/07/03 19:08:04, vmpstr wrote:
> > On 2013/07/03 00:32:47, David Reveman wrote:
> > > I think we can remove |tiles_|. It's just local to ManageTiles().
> > 
> > It's also used in DispatchMoreTasks (this avoids an extra sort by priority).
I
> > agree that it should be possible to remove this, but I think this requires a
> bit
> > of thought and code rework not to regress on efficiency. Are you ok with
this
> > being a follow-up?
> 
> I think we need include some of it in this patch. How about we keep this
vector
> but instead of erasing elements from it we just mark it as invalid in
> Register/UnregisterTile?

Done.

https://codereview.chromium.org/18581004/diff/2001/cc/resources/tile_manager....
cc/resources/tile_manager.h:154: TileSet
tiles_that_need_to_be_initialized_for_activation_;
On 2013/07/03 20:20:06, David Reveman wrote:
> On 2013/07/03 19:08:04, vmpstr wrote:
> > On 2013/07/03 00:32:47, David Reveman wrote:
> > > fyi, I think we can remove this once my "ready to activate" patch lands.
> > > 
> > > In general, we shouldn't store any "Tile*" after we add this TileMap.
> > Preferably
> > > existing "Tile*" containers are removed. The ones that can't be removed
> should
> > > store Tile::TileId instead. 
> > 
> > Sounds good. I think we would have to rethink the flow of how we throttle
> things
> > based on available memory, but I think this is work worth doing
> 
> I think it's trivial to replace
> |tiles_that_need_to_be_initialized_for_activation_| with a simple "bool
> tiles_need_to_be_initialized_for_activation_" now that my patch has landed. We
> can do that as part of this patch or land it in a separate patch before this.

I'd rather do this separately. Do you mind putting up a patch, if you have some
time?

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:160: tiles_.push_back(tile);
On 2013/07/03 20:20:06, David Reveman wrote:
> I prefer if this becomes |sorted_tiles_| as that better represents what it is
> and will help prevent us from using it when not valid.
> 
> in here we could just "sorted_tiles_need_update_ = true" or something like
that

Done.

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:170: tiles_.erase(std::remove(tiles_.begin(),
tiles_.end(), tile));
On 2013/07/03 20:20:06, David Reveman wrote:
> "sorted_tiles_need_update_ = true" or whatever we decide

Done.

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:174: managed_tiles_.erase(managed_iter);
On 2013/07/03 20:20:06, David Reveman wrote:
> I prefer:
> DCHECK(tiles_.find(tile->id()) != tiles_.end());
> tiles_.erase(tile->id());

Done.

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.h:161: typedef std::set<Tile*> TileSet;
On 2013/07/03 20:20:06, David Reveman wrote:
> move this below TileVector tiles_;

Done.

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.h:162: TileVector tiles_;
On 2013/07/03 20:20:06, David Reveman wrote:
> how about sorted_tiles_?

Done.

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.h:164: TileSet
oom_tiles_that_need_to_be_initialized_for_activation_;
On 2013/07/03 20:20:06, David Reveman wrote:
> |oom_tiles_that_need_to_be_initialized_for_activation_| doesn't have to be a
> member. It can be local to functions calling AssignGpuMem and passed as
> parameter to that AssignGpuMem

Done.

https://codereview.chromium.org/18581004/diff/8001/cc/resources/tile_manager....
cc/resources/tile_manager.h:180: TileMap managed_tiles_;
On 2013/07/03 20:20:06, David Reveman wrote:
> This is now the most important container of this class. I really think we
should
> move it to the top and call it |tiles_|.

Done.

[reveman, 2013-07-04 00:46:11]

This looks great!

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:139: DCHECK(tiles_.empty());
DCHECK_EQ(0u, tiles_.size()) is preferred

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:147:
ScheduleTasks(tiles_that_need_to_be_rasterized);
I don't think we need to exercise all this code here. how about just doing:
sorted_tiles_.clear();
TileVector empty;
ScheduleTasks(empty);

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:195: SortTiles(&sorted_tiles_);
I would prefer building the vector here and passing it to AssignBinsToTiles
instead of having that function build it.

It might also be a bit cleaner to use a new TileVector and
sorted_tiles_.swap(new_tiles) after calling SortTiles().

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:502: void TileManager::AssignGpuMemoryToTiles(
Maybe pass sorted tiles as a parameter to this function instead of having this
function assume it's valid.

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:628: void
TileManager::ReassignGpuMemoryToOOMTilesRequiredForActivation(
Same thing here. Maybe pass sorted tiles as a parameter.

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:710: for (TileMap::iterator it = tiles_.begin(); it
!= tiles_.end(); ++it)
maybe use sorted tiles here as that will likely be more efficient to iterate
over.

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:891: TileMap::iterator tile_iter =
tiles_.find(managed_id);
TileMap::iterator it = tiles_.fi.. ?

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:929:
tiles_that_need_to_be_initialized_for_activation_.erase(tile);
we don't need this anymore. you can just set
all_tiles_required_for_activation_have_been_initialized_ to true before calling
NotifyReadyToActivate.

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.h:177: TileSet
tiles_that_need_to_be_initialized_for_activation_;
I don't think we need this set.
bool all_tiles_required_for_activation_have_been_initialized_;
should be enough.

[vmpstr, 2013-07-08 17:28:30]

ptal.

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:139: DCHECK(tiles_.empty());
On 2013/07/04 00:46:11, David Reveman wrote:
> DCHECK_EQ(0u, tiles_.size()) is preferred

Done.

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:147:
ScheduleTasks(tiles_that_need_to_be_rasterized);
On 2013/07/04 00:46:11, David Reveman wrote:
> I don't think we need to exercise all this code here. how about just doing:
> sorted_tiles_.clear();
> TileVector empty;
> ScheduleTasks(empty);

Done.

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:195: SortTiles(&sorted_tiles_);
On 2013/07/04 00:46:11, David Reveman wrote:
> I would prefer building the vector here and passing it to AssignBinsToTiles
> instead of having that function build it.
> 
> It might also be a bit cleaner to use a new TileVector and
> sorted_tiles_.swap(new_tiles) after calling SortTiles().

Done.

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:502: void TileManager::AssignGpuMemoryToTiles(
On 2013/07/04 00:46:11, David Reveman wrote:
> Maybe pass sorted tiles as a parameter to this function instead of having this
> function assume it's valid.

Done.

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:628: void
TileManager::ReassignGpuMemoryToOOMTilesRequiredForActivation(
On 2013/07/04 00:46:11, David Reveman wrote:
> Same thing here. Maybe pass sorted tiles as a parameter.

Done.

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:710: for (TileMap::iterator it = tiles_.begin(); it
!= tiles_.end(); ++it)
On 2013/07/04 00:46:11, David Reveman wrote:
> maybe use sorted tiles here as that will likely be more efficient to iterate
> over.

I prefer to keep it as is, since tiles_ is the authoritative source of what
tiles exist. If we use sorted tiles here, then the call site has to ensure that
it's up to date (after possible removal/addition of new tiles)

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:891: TileMap::iterator tile_iter =
tiles_.find(managed_id);
On 2013/07/04 00:46:11, David Reveman wrote:
> TileMap::iterator it = tiles_.fi.. ?

Done.

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:929:
tiles_that_need_to_be_initialized_for_activation_.erase(tile);
On 2013/07/04 00:46:11, David Reveman wrote:
> we don't need this anymore. you can just set
> all_tiles_required_for_activation_have_been_initialized_ to true before
calling
> NotifyReadyToActivate.

Done.

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/18581004/diff/14001/cc/resources/tile_manager...
cc/resources/tile_manager.h:177: TileSet
tiles_that_need_to_be_initialized_for_activation_;
On 2013/07/04 00:46:11, David Reveman wrote:
> I don't think we need this set.
> bool all_tiles_required_for_activation_have_been_initialized_;
> should be enough.

Done.

[vmpstr, 2013-07-09 00:36:17]

I think this is ready for another review. PTAL.

[reveman, 2013-07-09 01:15:36]

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

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile.h#newco...
cc/resources/tile.h:23: typedef uint64 Id;
should this be an int and we deal with the possibility of it wrapping around
like layer ids?

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:337: UpdateSortedTiles();
I would prefer if you just moved the updatesortedtiles code here. That should
never be done from anywhere else and having the code part of ManageTiles makes
it more clear and deter people from using it elsewhere.

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:427: void
TileManager::AddRequiredTileForActivation(Tile* tile) {
I don't think this helper function adds much. The code is probably easier to
read without it.

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:871: void
TileManager::DidFinishTileInitialization(Tile* tile) {
I don't think this helper function is worth much anymore. Consider moving these
two lines to OnRasterTaskCompleted.

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:878: WhichTree tree) {
looks like this helper is also just making the code harder to read at this
point. we can fix that in a follow up but I don't mind if you removed it as part
of this patch.

[tomhudson, 2013-07-09 09:06:07]

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:337: UpdateSortedTiles();
On 2013/07/09 01:15:37, David Reveman wrote:
> I would prefer if you just moved the updatesortedtiles code here. That should
> never be done from anywhere else and having the code part of ManageTiles makes
> it more clear and deter people from using it elsewhere.

I like most of your other suggestions to get rid of helper functions, but
UpdateSortedTiles() is a large chunk of work, with a meaningful name, which we
want to independently TRACE... Leaving it broken out into a function (with
perhaps a slightly clearer name?) seems like a win to me.

[reveman, 2013-07-09 15:54:03]

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:337: UpdateSortedTiles();
On 2013/07/09 09:06:07, tomhudson wrote:
> On 2013/07/09 01:15:37, David Reveman wrote:
> > I would prefer if you just moved the updatesortedtiles code here. That
should
> > never be done from anywhere else and having the code part of ManageTiles
makes
> > it more clear and deter people from using it elsewhere.
> 
> I like most of your other suggestions to get rid of helper functions, but
> UpdateSortedTiles() is a large chunk of work, with a meaningful name, which we
> want to independently TRACE... Leaving it broken out into a function (with
> perhaps a slightly clearer name?) seems like a win to me.

We already have tracing for SortTiles and AssignBinsToTiles so the only part
we're missing is the process of creating the vector from the map. Maybe we
should just move that part to a different function instead of nesting it?

I think we should eventually move the tile map out of the tile manager and
instead have it passed as a parameter to ManageTiles(). That would prevent the
tile manager from incorrectly using unmanaged tiles. Having this code well
aligned with that idea might help prevent incorrect behavior to some degree
without actually moving the tile map.

[vmpstr, 2013-07-09 16:48:18]

PTAL.

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

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile.h#newco...
cc/resources/tile.h:23: typedef uint64 Id;
On 2013/07/09 01:15:37, David Reveman wrote:
> should this be an int and we deal with the possibility of it wrapping around
> like layer ids?

signed integer overflow invokes undefined behavior... I'd rather keep this as
is, unless there is a strong reason not to. Also, the reason it's 64 not 32 is
that we typically generate a lot more tiles than layers, so there's a bigger
chance of running into same id, which would cause weirdness (although it's still
unlikely)

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:337: UpdateSortedTiles();
On 2013/07/09 15:54:03, David Reveman wrote:
> On 2013/07/09 09:06:07, tomhudson wrote:
> > On 2013/07/09 01:15:37, David Reveman wrote:
> > > I would prefer if you just moved the updatesortedtiles code here. That
> should
> > > never be done from anywhere else and having the code part of ManageTiles
> makes
> > > it more clear and deter people from using it elsewhere.
> > 
> > I like most of your other suggestions to get rid of helper functions, but
> > UpdateSortedTiles() is a large chunk of work, with a meaningful name, which
we
> > want to independently TRACE... Leaving it broken out into a function (with
> > perhaps a slightly clearer name?) seems like a win to me.
> 
> We already have tracing for SortTiles and AssignBinsToTiles so the only part
> we're missing is the process of creating the vector from the map. Maybe we
> should just move that part to a different function instead of nesting it?
> 
> I think we should eventually move the tile map out of the tile manager and
> instead have it passed as a parameter to ManageTiles(). That would prevent the
> tile manager from incorrectly using unmanaged tiles. Having this code well
> aligned with that idea might help prevent incorrect behavior to some degree
> without actually moving the tile map.

I tend to agree with Tom for two reasons: 1. It's a nice separate function that
does a limited amount of work and it's easy to reason about it. 2. It allows us
to test AssignGpuMemory (first UpdateSortedTiles, then AssignGpuMemory). If we
move the code directly in here, then we wouldn't be able to AssignGpuMemory
without doing the sorting ourselves). My plan was to actually write up a few
unittests for just UpdateSortedTiles as well... In the future, we can pass an
unordered set of tiles to UpdateSortedTiles, so that it would construct the
vector from the argument instead of from the class member. Wdyt?

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:427: void
TileManager::AddRequiredTileForActivation(Tile* tile) {
On 2013/07/09 01:15:37, David Reveman wrote:
> I don't think this helper function adds much. The code is probably easier to
> read without it.

Done.

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:871: void
TileManager::DidFinishTileInitialization(Tile* tile) {
On 2013/07/09 01:15:37, David Reveman wrote:
> I don't think this helper function is worth much anymore. Consider moving
these
> two lines to OnRasterTaskCompleted.

Done.

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:878: WhichTree tree) {
On 2013/07/09 01:15:37, David Reveman wrote:
> looks like this helper is also just making the code harder to read at this
> point. we can fix that in a follow up but I don't mind if you removed it as
part
> of this patch.

Done.

[reveman, 2013-07-09 18:52:29]

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/18581004/diff/24001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:337: UpdateSortedTiles();
On 2013/07/09 16:48:18, vmpstr wrote:
> On 2013/07/09 15:54:03, David Reveman wrote:
> > On 2013/07/09 09:06:07, tomhudson wrote:
> > > On 2013/07/09 01:15:37, David Reveman wrote:
> > > > I would prefer if you just moved the updatesortedtiles code here. That
> > should
> > > > never be done from anywhere else and having the code part of ManageTiles
> > makes
> > > > it more clear and deter people from using it elsewhere.
> > > 
> > > I like most of your other suggestions to get rid of helper functions, but
> > > UpdateSortedTiles() is a large chunk of work, with a meaningful name,
which
> we
> > > want to independently TRACE... Leaving it broken out into a function (with
> > > perhaps a slightly clearer name?) seems like a win to me.
> > 
> > We already have tracing for SortTiles and AssignBinsToTiles so the only part
> > we're missing is the process of creating the vector from the map. Maybe we
> > should just move that part to a different function instead of nesting it?
> > 
> > I think we should eventually move the tile map out of the tile manager and
> > instead have it passed as a parameter to ManageTiles(). That would prevent
the
> > tile manager from incorrectly using unmanaged tiles. Having this code well
> > aligned with that idea might help prevent incorrect behavior to some degree
> > without actually moving the tile map.
> 
> I tend to agree with Tom for two reasons: 1. It's a nice separate function
that
> does a limited amount of work and it's easy to reason about it. 2. It allows
us
> to test AssignGpuMemory (first UpdateSortedTiles, then AssignGpuMemory). If we
> move the code directly in here, then we wouldn't be able to AssignGpuMemory
> without doing the sorting ourselves). My plan was to actually write up a few
> unittests for just UpdateSortedTiles as well... In the future, we can pass an
> unordered set of tiles to UpdateSortedTiles, so that it would construct the
> vector from the argument instead of from the class member. Wdyt?

Not sure. This function is doing more than the name entails. ie. it's dropping
references which cause tile resources to be freed.

https://codereview.chromium.org/18581004/diff/33001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/18581004/diff/33001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:141: void TileManager::UpdateSortedTiles() {
If you keep this function move it just above ManageTiles() to be consistent with
the current "caller below callee" pattern.

https://codereview.chromium.org/18581004/diff/33001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:148: TileRefVector tiles;
should this be a parameter instead? it's a bit weird that we clear sorted_tiles_
here and then create a temporary new vector that we swap with.

https://codereview.chromium.org/18581004/diff/33001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:154: SortTiles(&tiles);
Is the nesting here necessary? Would it make more sense to make these calls from
ManageTiles and this function just becomes a helper for creating a new
TileRefVector?

[vmpstr, 2013-07-09 19:29:31]

PTAL.

https://codereview.chromium.org/18581004/diff/33001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/18581004/diff/33001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:141: void TileManager::UpdateSortedTiles() {
On 2013/07/09 18:52:30, David Reveman wrote:
> If you keep this function move it just above ManageTiles() to be consistent
with
> the current "caller below callee" pattern.

Done.

https://codereview.chromium.org/18581004/diff/33001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:148: TileRefVector tiles;
On 2013/07/09 18:52:30, David Reveman wrote:
> should this be a parameter instead? it's a bit weird that we clear
sorted_tiles_
> here and then create a temporary new vector that we swap with.

I changed this to be "GetSortedTiles" instead, which I think makes a bit more
sense. It now takes an argument, gathers all of the tiles from the hash map,
assigns bins (priority), and sorts it. Let me know if this looks better to you.

I would prefer to keep this a GetSortedTiles, instead of just GetTiles, which
means I think we should sort inside the function as well.

[reveman, 2013-07-09 20:06:31]

lgtm

https://codereview.chromium.org/18581004/diff/36001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/18581004/diff/36001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:329: sorted_tiles_.clear();
nit: please add blank line between this and GetSortedTiles() and keep the
comment you had next to this before so it's clear that this will free resources
and delete old tiles.

[commit-bot: I haz the power, 2013-07-09 20:53:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/18581004/40001

[vmpstr, 2013-07-09 20:55:38]

https://codereview.chromium.org/18581004/diff/36001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/18581004/diff/36001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:329: sorted_tiles_.clear();
On 2013/07/09 20:06:31, David Reveman wrote:
> nit: please add blank line between this and GetSortedTiles() and keep the
> comment you had next to this before so it's clear that this will free
resources
> and delete old tiles.

Done.

[commit-bot: I haz the power, 2013-07-09 21:14:26]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-09 21:33:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/18581004/61001

[commit-bot: I haz the power, 2013-07-10 02:08:13]

Change committed as 210713