cc: Do not cleanup tiles with raster tasks.

cc/resources/tile_manager.cc

Index: cc/resources/tile_manager.cc
diff --git a/cc/resources/tile_manager.cc b/cc/resources/tile_manager.cc
index d508f6a0a17a5639290949fb9cf083eb4ffb6e10..3d84f1e02eb9556cedabcbba791e673835c42d6e 100644
--- a/cc/resources/tile_manager.cc
+++ b/cc/resources/tile_manager.cc
@@ -389,18 +389,16 @@ TileManager::~TileManager() {
   // our memory usage to drop to zero.
   global_state_ = GlobalStateThatImpactsTilePriority();
 
-  CleanUpReleasedTiles();
-  DCHECK_EQ(0u, tiles_.size());
-
   RasterTaskQueue empty;
   rasterizer_->ScheduleTasks(&empty);
-  orphan_raster_tasks_.clear();
 
   // This should finish all pending tasks and release any uninitialized
   // resources.
   rasterizer_->Shutdown();
   rasterizer_->CheckForCompletedTasks();
 
+  CleanUpReleasedTiles();
+
   DCHECK_EQ(0u, bytes_releasable_);
   DCHECK_EQ(0u, resources_releasable_);
 }
@@ -419,15 +417,21 @@ bool TileManager::ShouldForceTasksRequiredForActivationToComplete() const {
 }
 
 void TileManager::CleanUpReleasedTiles() {
-  for (std::vector<Tile*>::iterator it = released_tiles_.begin();
-       it != released_tiles_.end();
-       ++it) {
+  std::vector<Tile*>::iterator it = released_tiles_.begin();
+  while (it != released_tiles_.end()) {
     Tile* tile = *it;
     ManagedTileState& mts = tile->managed_state();
+    bool tile_has_raster_task = false;
 
     for (int mode = 0; mode < NUM_RASTER_MODES; ++mode) {
       FreeResourceForTile(tile, static_cast<RasterMode>(mode));

[reveman, 2014/07/09 14:15:30]

What's the difference between this and only releasing resources when the tile is
ready to be deleted? It's a subtle difference to the code so if it has a
critical impact on the behavior we should document it. Can you please describe a
use case where this makes a difference and what the difference is.

[sohanjg, 2014/07/09 14:32:11]

Hmm, 1 thing could be we optimize resource usage (if at all, till we hit memory
bump). Isnt it better to release whatever is releasable, rather than holding it
back ?

You and vmpstr@ are the best judge here.

[vmpstr, 2014/07/09 18:41:21]

I think at this point the tile is set to be deleted at one point or another, so
the memory will definitely be released. If we keep the resources until we
actually delete, then we technically would not be able to release any of those
tile resources any other way (this tile is no longer accessible via
AssignGpuMemory).

I don't really think it's that much of a difference, since the task will be
cancelled or finish raster and then be cleaned up, but I prefer this approach as
it states a bit more explicitly that all tile resources at this point are
released (the fact that we hold on to the tile has nothing to do with the
resources, but with the task). 

[reveman, 2014/07/09 20:09:43]

Maybe we should make this a bit more explicit by adding another function called
something like FreeResourcesForReleasedTiles() that takes care of this. This
function would be responsible for freeing resources and the existing
CleanUpReleasedTiles() function would simply cleanup tiles and DCHECK to ensure
that all resources have already been freed.
What I find a bit awkward is that we still need to make another one of these
loops that free all resources later when all pending raster tasks have finished
as you then have more resources on the tile than before. It all feels a bit too
subtle to me and while this might not be so fragile right now, I'm worried
future changes could change that.

I'd prefer if we only released resources when the tile is ready to be cleaned up
or add some properly named function that makes it more clear that we're
releasing resources as a separate step from actually cleaning up the tiles.

[vmpstr, 2014/07/09 23:15:14]

That sounds good to me. 

[sohanjg, 2014/07/10 13:29:45]

Done.

-      orphan_raster_tasks_.push_back(mts.tile_versions[mode].raster_task_);
+      if (mts.tile_versions[mode].raster_task_)
+        tile_has_raster_task = true;
+    }
+
+    if (tile_has_raster_task) {
+      ++it;
+      continue;
     }
 
     DCHECK(tiles_.find(tile->id()) != tiles_.end());

[vmpstr, 2014/07/09 23:15:14]

Sorry to backtrack a bit. My previous comment already has the iterators patch in
mind. In this case, the tile is still accessible in AssignGpuMem. Maybe it's
worth it to always erase it from tiles_ here, and only keep it in
released_tiles_? That way we will avoid scheduling this tile again if we run
assignmem again. When the raster task completes then, it won't find tiles_
member corresponding to the task and release the resource immediately. 

[reveman, 2014/07/10 11:23:03]

I prefer to keep it in |tiles_| and change the "it == tiles_.end()" check in
OnRasterTaskCompleted to a DCHECK. It shouldn't make a difference in terms of
how quickly we release resources as OnRasterTaskCompleted will only be called
when we check for completed tasks and we should clean up released tiles after
each of those calls.

[sohanjg, 2014/07/10 13:29:45]

Done.

@@ -442,9 +446,8 @@ void TileManager::CleanUpReleasedTiles() {
     }
 
     delete tile;
+    it = released_tiles_.erase(it);
   }
-
-  released_tiles_.clear();
 }
 
 void TileManager::UpdatePrioritizedTileSetIfNeeded() {
@@ -970,11 +973,6 @@ void TileManager::ScheduleTasks(
   // in |raster_tasks_|.
   rasterizer_->ScheduleTasks(&raster_queue_);
 
-  // It's now safe to clean up orphan tasks as raster worker pool is not
-  // allowed to keep around unreferenced raster tasks after ScheduleTasks() has
-  // been called.
-  orphan_raster_tasks_.clear();
-
   did_check_for_completed_tasks_since_last_schedule_tasks_ = false;
 }
 
@@ -1078,8 +1076,6 @@ void TileManager::OnRasterTaskCompleted(
   Tile* tile = it->second;
   ManagedTileState& mts = tile->managed_state();
   ManagedTileState::TileVersion& tile_version = mts.tile_versions[raster_mode];
-  DCHECK(tile_version.raster_task_);
-  orphan_raster_tasks_.push_back(tile_version.raster_task_);
   tile_version.raster_task_ = NULL;
 
   if (was_canceled) {