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..b14e58650837d02bdf4e38e8da9fb6fbe76af6e9 100644
--- a/cc/resources/tile_manager.cc
+++ b/cc/resources/tile_manager.cc
@@ -394,7 +394,6 @@ TileManager::~TileManager() {
 
   RasterTaskQueue empty;
   rasterizer_->ScheduleTasks(&empty);
-  orphan_raster_tasks_.clear();
 
   // This should finish all pending tasks and release any uninitialized
   // resources.
@@ -420,16 +419,22 @@ bool TileManager::ShouldForceTasksRequiredForActivationToComplete() const {
 
 void TileManager::CleanUpReleasedTiles() {
   for (std::vector<Tile*>::iterator it = released_tiles_.begin();
-       it != released_tiles_.end();
-       ++it) {
+       it != released_tiles_.end();) {

[reveman, 2014/07/03 18:12:47]

nit: I prefer if you use a while loop when iterating like this

[sohanjg, 2014/07/04 11:56:58]

Done.

     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));
-      orphan_raster_tasks_.push_back(mts.tile_versions[mode].raster_task_);
+      if (!mts.tile_versions[mode].raster_task_)
+        FreeResourceForTile(tile, static_cast<RasterMode>(mode));

[reveman, 2014/07/03 18:12:47]

I think it's a bit confusing that this code could release some resources but not
delete the tile. ie. first tile version without pending task but second with
one. I think it would be better to handle this pending task check in a loop
above this and not free any resources if the tile has a pending task.

[sohanjg, 2014/07/04 11:56:58]

Done.

[vmpstr, 2014/07/05 17:42:19]

I actually think we should always release all resources on the tile... Something
like:

for (int mode = 0; ...) {
  FreeResourcesForTile(tile, static_cast<RasterMode>(mode));
  if (tile_versions[mode].raster_task_)
    tile_has_raster_task = true;
}

That way we free up everything we can free up, but then not delete the tile if
it still has a raster task. I agree with reveman that in this version it's weird
to sometimes release a resource. However, in the new version we don't release
any resources if a tile has a task, which also seems wasteful.

[reveman, 2014/07/07 18:15:16]

Fine with me if actually worthwhile. Would we not free these resources under
memory pressure anyhow?

[sohanjg, 2014/07/08 06:21:15]

Done.

+      else
+        tile_has_raster_task = true;
     }
 
+    if (tile_has_raster_task) {
+      ++it;
+      continue;
+    }

[reveman, 2014/07/03 18:12:47]

nit: blank line here

[sohanjg, 2014/07/04 11:56:58]

Done.

     DCHECK(tiles_.find(tile->id()) != tiles_.end());
     tiles_.erase(tile->id());
 
@@ -441,10 +446,9 @@ void TileManager::CleanUpReleasedTiles() {
       image_decode_tasks_.erase(tile->layer_id());
     }
 
+    it = released_tiles_.erase(it);

[reveman, 2014/07/03 18:12:47]

nit: move this below "delete tile;" to maintain the same order in which the tile
and the map entry are destroyed. doesn't really matter but there's no reason to
change it and I think it's a bit cleaner to update the iterator as the last
thing you do in the loop.

[sohanjg, 2014/07/04 11:56:58]

Done.

     delete tile;
   }
-
-  released_tiles_.clear();
 }
 
 void TileManager::UpdatePrioritizedTileSetIfNeeded() {
@@ -970,11 +974,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 +1077,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) {