cc: Do not cleanup tiles with raster tasks.

cc/resources/tile_manager.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/resources/tile_manager.h"
 
 #include <algorithm>
 #include <limits>
 #include <string>
 
@@ skipping 371 matching lines @@
           base::Bind(&TileManager::CheckIfReadyToActivate,
                      base::Unretained(this))) {
   rasterizer_->SetClient(this);
 }
 
 TileManager::~TileManager() {
   // Reset global state and manage. This should cause
   // our memory usage to drop to zero.
   global_state_ = GlobalStateThatImpactsTilePriority();
 
   CleanUpReleasedTiles();

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

I think you need to move this below CheckForCompletedTasks() as there's no
guarantee that all tiles can be released otherwise.

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

Done.

   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();
 
   DCHECK_EQ(0u, bytes_releasable_);
   DCHECK_EQ(0u, resources_releasable_);
 }
 
 void TileManager::Release(Tile* tile) {
   prioritized_tiles_dirty_ = true;
   released_tiles_.push_back(tile);
 }
 
 void TileManager::DidChangeTilePriority(Tile* tile) {
   prioritized_tiles_dirty_ = true;
 }
 
 bool TileManager::ShouldForceTasksRequiredForActivationToComplete() const {
   return global_state_.tree_priority != SMOOTHNESS_TAKES_PRIORITY;
 }
 
 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());
 
     LayerCountMap::iterator layer_it =
         used_layer_counts_.find(tile->layer_id());
     DCHECK_GT(layer_it->second, 0);
     if (--layer_it->second == 0) {
       used_layer_counts_.erase(layer_it);
       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() {
   if (!prioritized_tiles_dirty_)
     return;
 
   CleanUpReleasedTiles();

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

This is not the right place to call this anymore. It now needs to be called
after CheckForCompletedTasks.

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

Have moved it to ManageTiles, where it checks for completed tasks.
I dont think we should cleanup released tiles every time we check for completed
tasks ?

 
   prioritized_tiles_.Clear();
   GetTilesWithAssignedBins(&prioritized_tiles_);
   prioritized_tiles_dirty_ = false;
 }
 
 void TileManager::DidFinishRunningTasks() {
   TRACE_EVENT0("cc", "TileManager::DidFinishRunningTasks");
 
   bool memory_usage_above_limit = resource_pool_->total_memory_usage_bytes() >
@@ skipping 498 matching lines @@
 
   // We must reduce the amount of unused resoruces before calling
   // ScheduleTasks to prevent usage from rising above limits.
   resource_pool_->ReduceResourceUsage();
 
   // Schedule running of |raster_tasks_|. This replaces any previously
   // scheduled tasks and effectively cancels all tasks not present
   // 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;
 }
 
 scoped_refptr<ImageDecodeTask> TileManager::CreateImageDecodeTask(
     Tile* tile,
     SkPixelRef* pixel_ref) {
   return make_scoped_refptr(new ImageDecodeTaskImpl(
       pixel_ref,
       tile->layer_id(),
       rendering_stats_instrumentation_,
@@ skipping 83 matching lines @@
   TileMap::iterator it = tiles_.find(tile_id);
   if (it == tiles_.end()) {
     ++update_visible_tiles_stats_.canceled_count;
     resource_pool_->ReleaseResource(resource.Pass());
     return;
   }
 
   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) {
     ++update_visible_tiles_stats_.canceled_count;
     resource_pool_->ReleaseResource(resource.Pass());
     return;
   }
 
   ++update_visible_tiles_stats_.completed_count;
 
@@ skipping 483 matching lines @@
   TRACE_EVENT0("cc", "TileManager::CheckIfReadyToActivate");
 
   rasterizer_->CheckForCompletedTasks();
   did_check_for_completed_tasks_since_last_schedule_tasks_ = true;
 
   if (IsReadyToActivate())
     client_->NotifyReadyToActivate();
 }
 
 }  // namespace cc