Re-land: cc: Add raster finished signals to RasterWorkerPool.

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 f313bf66b0ab64d6838475e7990919bfd23eeccb..a4db6b7af0d15c0205f12d54d24a93805f15f3f3 100644
--- a/cc/resources/tile_manager.cc
+++ b/cc/resources/tile_manager.cc
@@ -170,6 +170,7 @@ void TileManager::UnregisterTile(Tile* tile) {
     tiles_that_need_to_be_rasterized_.erase(raster_iter);
 
   tiles_that_need_to_be_initialized_for_activation_.erase(tile);
+  oom_tiles_that_need_to_be_initialized_for_activation_.erase(tile);
 
   DCHECK(std::find(tiles_.begin(), tiles_.end(), tile) != tiles_.end());
   FreeResourcesForTile(tile);
@@ -180,6 +181,55 @@ bool TileManager::ShouldForceTasksRequiredForActivationToComplete() const {
   return client_->ShouldForceTileUploadsRequiredForActivationToComplete();
 }
 
+void TileManager::DidFinishedRunningTasks() {
+  // When OOM, keep re-assigning memory until we reach a steady state
+  // where top-priority tiles are initialized.
+  if (!memory_stats_from_last_assign_.bytes_over)
+    return;
+
+  raster_worker_pool_->CheckForCompletedTasks();
+
+  AssignGpuMemoryToTiles();
+
+  if (!oom_tiles_that_need_to_be_initialized_for_activation_.empty())
+    ReassignGpuMemoryToOOMTilesRequiredForActivation();
+
+  // |tiles_that_need_to_be_rasterized_| will be empty when we reach a
+  // steady memory state. Keep scheduling tasks until we reach this state.
+  if (!tiles_that_need_to_be_rasterized_.empty()) {
+    ScheduleTasks();
+    return;
+  }
+
+  // Use on-demand raster for any tiles that have not been been assigned
+  // memory after reaching a steady memory state.
+  for (TileSet::iterator it =
+           oom_tiles_that_need_to_be_initialized_for_activation_.begin();
+       it != oom_tiles_that_need_to_be_initialized_for_activation_.end();
+       ++it) {
+    Tile* tile = *it;
+    ManagedTileState& mts = tile->managed_state();
+    mts.tile_versions[mts.raster_mode].set_rasterize_on_demand();
+  }
+  oom_tiles_that_need_to_be_initialized_for_activation_.clear();
+
+  client_->NotifyReadyToActivate();
+}
+
+void TileManager::DidFinishedRunningTasksRequiredForActivation() {
+  // This is only a true indication that all tiles required for
+  // activation are initialized when no tiles are OOM. We need to
+  // wait for DidFinishRunningTasks() to be called, try to re-assign
+  // memory and in worst case use on-demand raster when tiles
+  // required for activation are OOM.
+  if (!oom_tiles_that_need_to_be_initialized_for_activation_.empty())
+    return;
+
+  // TODO(reveman): Push the responsibility of calling CheckForCompletedTasks()
+  // to the LTHI where it can be delayed until it's time to draw.
+  raster_worker_pool_->CheckForCompletedTasks();
+}
+
 class BinComparator {
  public:
   bool operator() (const Tile* a, const Tile* b) const {
@@ -306,10 +356,6 @@ void TileManager::ManageTiles() {
   AssignGpuMemoryToTiles();
   CleanUpUnusedImageDecodeTasks();
 
-  // This could have changed after AssignGpuMemoryToTiles.
-  if (AreTilesRequiredForActivationReady())
-    client_->NotifyReadyToActivate();
-
   TRACE_EVENT_INSTANT1(
       "cc", "DidManage", TRACE_EVENT_SCOPE_THREAD,
       "state", TracedValue::FromValue(BasicStateAsValue().release()));
@@ -414,6 +460,7 @@ void TileManager::AssignGpuMemoryToTiles() {
   // the needs-to-be-rasterized queue.
   tiles_that_need_to_be_rasterized_.clear();
   tiles_that_need_to_be_initialized_for_activation_.clear();
+  oom_tiles_that_need_to_be_initialized_for_activation_.clear();
 
   size_t bytes_releasable = 0;
   for (TileVector::const_iterator it = tiles_.begin();
@@ -436,10 +483,8 @@ void TileManager::AssignGpuMemoryToTiles() {
   size_t bytes_allocatable =
       std::max(static_cast<int64>(0), bytes_available);
 
-  size_t bytes_that_exceeded_memory_budget_in_now_bin = 0;
+  size_t bytes_that_exceeded_memory_budget = 0;
   size_t bytes_left = bytes_allocatable;
-  size_t bytes_oom_in_now_bin_on_pending_tree = 0;
-  TileVector tiles_requiring_memory_but_oomed;
   bool higher_priority_tile_oomed = false;
   for (TileVector::iterator it = tiles_.begin();
        it != tiles_.end();
@@ -479,13 +524,11 @@ void TileManager::AssignGpuMemoryToTiles() {
 
     // Tile is OOM.
     if (tile_bytes > bytes_left) {
-      mts.tile_versions[mts.raster_mode].set_rasterize_on_demand();
-      if (mts.tree_bin[PENDING_TREE] == NOW_BIN) {
-        tiles_requiring_memory_but_oomed.push_back(tile);
-        bytes_oom_in_now_bin_on_pending_tree += tile_bytes;
-      }
+      if (tile->required_for_activation())
+        oom_tiles_that_need_to_be_initialized_for_activation_.insert(tile);
       FreeResourcesForTile(tile);
       higher_priority_tile_oomed = true;
+      bytes_that_exceeded_memory_budget += tile_bytes;
       continue;
     }
 
@@ -510,85 +553,104 @@ void TileManager::AssignGpuMemoryToTiles() {
     }
   }
 
+  ever_exceeded_memory_budget_ |= bytes_that_exceeded_memory_budget > 0;
+  if (ever_exceeded_memory_budget_) {
+      TRACE_COUNTER_ID2("cc", "over_memory_budget", this,
+                        "budget", global_state_.memory_limit_in_bytes,
+                        "over", bytes_that_exceeded_memory_budget);
+  }
+  memory_stats_from_last_assign_.total_budget_in_bytes =
+      global_state_.memory_limit_in_bytes;
+  memory_stats_from_last_assign_.bytes_allocated =
+      bytes_allocatable - bytes_left;
+  memory_stats_from_last_assign_.bytes_unreleasable =
+      bytes_allocatable - bytes_releasable;
+  memory_stats_from_last_assign_.bytes_over =
+      bytes_that_exceeded_memory_budget;
+}
+
+void TileManager::ReassignGpuMemoryToOOMTilesRequiredForActivation() {
+  TRACE_EVENT0(
+      "cc", "TileManager::ReassignGpuMemoryToOOMTilesRequiredForActivation");
+
+  size_t bytes_oom_for_required_tiles = 0;
+  TileVector tiles_requiring_memory_but_oomed;
+  for (TileVector::iterator it = tiles_.begin(); it != tiles_.end(); ++it) {
+    Tile* tile = *it;
+    if (oom_tiles_that_need_to_be_initialized_for_activation_.find(tile) ==
+        oom_tiles_that_need_to_be_initialized_for_activation_.end())
+      continue;
+
+    tiles_requiring_memory_but_oomed.push_back(tile);
+    bytes_oom_for_required_tiles += tile->bytes_consumed_if_allocated();
+  }
+
+  if (tiles_requiring_memory_but_oomed.empty())
+    return;
+
   // In OOM situation, we iterate tiles_, remove the memory for active tree
-  // and not the now bin. And give them to bytes_oom_in_now_bin_on_pending_tree
-  if (!tiles_requiring_memory_but_oomed.empty()) {
-    size_t bytes_freed = 0;
-    for (TileVector::iterator it = tiles_.begin(); it != tiles_.end(); ++it) {

[reveman, 2013/06/24 19:05:17]

I changed this to a reverse iterator so we try to steal memory from low priority
tiles in the active tree before higher priority tiles.

-      Tile* tile = *it;
-      ManagedTileState& mts = tile->managed_state();
-      ManagedTileState::TileVersion& tile_version =
-          mts.tile_versions[mts.raster_mode];
-      if (mts.tree_bin[PENDING_TREE] == NEVER_BIN &&
-          mts.tree_bin[ACTIVE_TREE] != NOW_BIN) {
-        size_t bytes_that_can_be_freed = 0;
-
-        // If the tile is in the to-rasterize list, but it has no task,
-        // then it means that we have assigned memory for it.

[reveman, 2013/06/24 16:04:04]

I think there's a bug here. A tile will still be in the to-rasterize list after
completing and raster_task will then be null. We might want to land a fix for
this separately.

[vmpstr, 2013/06/24 16:34:03]

I don't think that's possible with the current code. to-rasterize list is
cleared at the top of the function, and the only items put into this are tasks
that actually need a raster (ie not completed). Unless you mean that it was
completed but we were not notified of that yet? I'm not sure what we can do
about that, short of forcing completion check

I think with the refactor that you have done here, it might be more possible? :P
I'm not sure.

[reveman, 2013/06/24 19:05:17]

You're right. It's not possible as tiles_that_need_to_be_rasterized_ is a
temporary variable in practice. We should make that a real temporary variable to
avoid confusion.

I changed back to your original code but simplified it a bit. computing
bytes_that_can_be_freed before actually freeing things doesn't seem to serve a
purpose. Latest patch just frees and adjusts bytes_freed directly. PTAL.

-        TileVector::iterator raster_it =
-            std::find(tiles_that_need_to_be_rasterized_.begin(),
-                      tiles_that_need_to_be_rasterized_.end(),
-                      tile);
-        if (raster_it != tiles_that_need_to_be_rasterized_.end() &&
-            tile_version.raster_task_.is_null()) {
-          bytes_that_can_be_freed += tile->bytes_consumed_if_allocated();
-        }
+  // and not the now bin. And give them to bytes_oom_for_required_tiles
+  size_t bytes_freed = 0;
+  for (TileVector::iterator it = tiles_.begin(); it != tiles_.end(); ++it) {
+    Tile* tile = *it;
+    ManagedTileState& mts = tile->managed_state();
+    if (mts.tree_bin[PENDING_TREE] == NEVER_BIN &&
+        mts.tree_bin[ACTIVE_TREE] != NOW_BIN) {
+      size_t bytes_that_can_be_freed = 0;
 
-        // Also consider all of the completed resources for freeing.
-        for (int mode = 0; mode < NUM_RASTER_MODES; ++mode) {
-          ManagedTileState::TileVersion& tile_version =
-              mts.tile_versions[mode];
-          if (tile_version.resource_) {
-            DCHECK(!tile->required_for_activation());
-            bytes_that_can_be_freed += tile->bytes_consumed_if_allocated();
-          }
+      for (int mode = 0; mode < NUM_RASTER_MODES; ++mode) {
+        ManagedTileState::TileVersion& tile_version = mts.tile_versions[mode];
+
+        // Consider all of the completed resources for freeing.
+        if (tile_version.resource_) {
+          DCHECK(!tile->required_for_activation());
+          bytes_that_can_be_freed += tile->bytes_consumed_if_allocated();
+          continue;
         }
 
-        // If we can free anything, then do so.
-        if (bytes_that_can_be_freed > 0) {
-          FreeResourcesForTile(tile);
-          bytes_freed += bytes_that_can_be_freed;
-          mts.tile_versions[mts.raster_mode].set_rasterize_on_demand();
+        // If the tile is in the to-rasterize list, it means that we
+        // have assigned memory to initialize current raster mode.
+        if (mode == mts.raster_mode) {

[vmpstr, 2013/06/24 16:34:03]

It's a bit awkward to have this in the inner loop. Is it possible to move this
out? Maybe have something like "bool current_raster_mode_had_resource" temporary
variable or something? This is not a strong opinion though, so I'm fine if you
like it this way.

+          TileVector::iterator raster_it =
+              std::find(tiles_that_need_to_be_rasterized_.begin(),
+                        tiles_that_need_to_be_rasterized_.end(),
+                        tile);
           if (raster_it != tiles_that_need_to_be_rasterized_.end())
-              tiles_that_need_to_be_rasterized_.erase(raster_it);
+            bytes_that_can_be_freed += tile->bytes_consumed_if_allocated();
         }
       }
 
-      if (bytes_oom_in_now_bin_on_pending_tree <= bytes_freed)
-        break;
+      // If we can free anything, then do so.
+      if (bytes_that_can_be_freed > 0) {
+        FreeResourcesForTile(tile);
+        bytes_freed += bytes_that_can_be_freed;
+        TileVector::iterator raster_it = std::find(
+            tiles_that_need_to_be_rasterized_.begin(),
+            tiles_that_need_to_be_rasterized_.end(),
+            tile);
+        if (raster_it != tiles_that_need_to_be_rasterized_.end())
+          tiles_that_need_to_be_rasterized_.erase(raster_it);
+      }
     }
 
-    for (TileVector::iterator it = tiles_requiring_memory_but_oomed.begin();
-         it != tiles_requiring_memory_but_oomed.end() && bytes_freed > 0;
-         ++it) {
-      Tile* tile = *it;
-      ManagedTileState& mts = tile->managed_state();
-      size_t bytes_needed = tile->bytes_consumed_if_allocated();
-      if (bytes_needed > bytes_freed)
-        continue;
-      mts.tile_versions[mts.raster_mode].set_use_resource();
-      bytes_freed -= bytes_needed;
-      tiles_that_need_to_be_rasterized_.push_back(tile);
-      if (tile->required_for_activation())
-        AddRequiredTileForActivation(tile);
-    }
+    if (bytes_oom_for_required_tiles <= bytes_freed)
+      break;
   }
 
-  ever_exceeded_memory_budget_ |=
-      bytes_that_exceeded_memory_budget_in_now_bin > 0;
-  if (ever_exceeded_memory_budget_) {
-      TRACE_COUNTER_ID2("cc", "over_memory_budget", this,
-                        "budget", global_state_.memory_limit_in_bytes,
-                        "over", bytes_that_exceeded_memory_budget_in_now_bin);
+  for (TileVector::iterator it = tiles_requiring_memory_but_oomed.begin();
+       it != tiles_requiring_memory_but_oomed.end() && bytes_freed > 0;
+       ++it) {
+    Tile* tile = *it;
+    ManagedTileState& mts = tile->managed_state();
+    size_t bytes_needed = tile->bytes_consumed_if_allocated();
+    if (bytes_needed > bytes_freed)
+      continue;
+    mts.tile_versions[mts.raster_mode].set_use_resource();
+    bytes_freed -= bytes_needed;
+    tiles_that_need_to_be_rasterized_.push_back(tile);
+    AddRequiredTileForActivation(tile);

[vmpstr, 2013/06/24 16:34:03]

DCHECK that it's required for activation, please.

[reveman, 2013/06/24 19:05:17]

Done.

+    oom_tiles_that_need_to_be_initialized_for_activation_.erase(tile);
   }
-  memory_stats_from_last_assign_.total_budget_in_bytes =
-      global_state_.memory_limit_in_bytes;
-  memory_stats_from_last_assign_.bytes_allocated =
-      bytes_allocatable - bytes_left;
-  memory_stats_from_last_assign_.bytes_unreleasable =
-      bytes_allocatable - bytes_releasable;
-  memory_stats_from_last_assign_.bytes_over =
-      bytes_that_exceeded_memory_budget_in_now_bin;
 }
 
 void TileManager::CleanUpUnusedImageDecodeTasks() {
@@ -639,7 +701,8 @@ void TileManager::FreeUnusedResourcesForTile(Tile* tile) {
 }
 
 void TileManager::ScheduleTasks() {
-  TRACE_EVENT0("cc", "TileManager::ScheduleTasks");
+  TRACE_EVENT1("cc", "TileManager::ScheduleTasks",
+               "count", tiles_that_need_to_be_rasterized_.size());
   RasterWorkerPool::RasterTask::Queue tasks;
 
   // Build a new task queue containing all task currently needed. Tasks
@@ -669,8 +732,6 @@ void TileManager::ScheduleTasks() {
 
 RasterWorkerPool::Task TileManager::CreateImageDecodeTask(
     Tile* tile, skia::LazyPixelRef* pixel_ref) {
-  TRACE_EVENT0("cc", "TileManager::CreateImageDecodeTask");

[vmpstr, 2013/06/24 16:34:03]

Can we replace these with INSTANT traces that just log the number of image
decodes and raster tasks that were used?

[reveman, 2013/06/24 19:05:17]

I added a count argument to the ScheduleTasks trace for this purpose. A trace
event for each task allocation seems a bit excessive but I can add that if you
insist?

[vmpstr, 2013/06/24 19:16:46]

No, I just meant to have the count somewhere. I'm happy with what you have now.

-
   return RasterWorkerPool::CreateImageDecodeTask(
       pixel_ref,
       tile->layer_id(),
@@ -695,8 +756,6 @@ RasterTaskMetadata TileManager::GetRasterTaskMetadata(
 }
 
 RasterWorkerPool::RasterTask TileManager::CreateRasterTask(Tile* tile) {
-  TRACE_EVENT0("cc", "TileManager::CreateRasterTask");
-
   ManagedTileState& mts = tile->managed_state();
 
   scoped_ptr<ResourcePool::Resource> resource =