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 dab3f5562823f5eadbdac2e7d7488726dee14f14..e01805b132deda41e7dabe066558110f230a24fe 100644
--- a/cc/resources/tile_manager.cc
+++ b/cc/resources/tile_manager.cc
@@ -389,23 +389,24 @@ 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();
 
+  FreeResourcesForReleasedTiles();
+  CleanUpReleasedTiles();
+  DCHECK_EQ(0u, tiles_.size());
+
   DCHECK_EQ(0u, bytes_releasable_);
   DCHECK_EQ(0u, resources_releasable_);
 }
 
 void TileManager::Release(Tile* tile) {
+  DCHECK(TilePriority() == tile->combined_priority());

[vmpstr, 2014/07/21 15:24:14]

Why does this work? I don't think we have code anywhere that would reset the
priority. That is, if we invalidate a NOW tile, it will still have NOW priority
here. If I'm missing something, please add a comment.

[sohanjg, 2014/07/22 14:17:46]

Ideally, we should do SetPriority(TREE, TilePriority()) for active and pending,
but we assume it is already the case, and dcheck if its not ? should we change
this ?

[vmpstr, 2014/07/22 15:17:29]

That's my question... I don't think it's the case right now. Tiles are released
by the tiling during invalidation, among other paths, and I'm pretty sure we
don't touch the priority when we do that. I think this DCHECK has to be
SetPriority(ACTIVE_TREE, TilePriority()); SetPriority(PENDING_TREE,
TilePriority()) instead with a comment saying that this is so that the tile is
not scheduled again.

[reveman, 2014/07/22 16:21:21]

Please give some example usage that justifies having the ability to drop tile
references but not reset the priority. I'm fine resetting it here if needed, I
just prefer to have a concrete example of why it's needed rather than assume it
is.

[vmpstr, 2014/07/22 16:31:02]

Suppose we have some tiles on the active tree with all priorities up to date,
then the live tiles rect moves down and we release a top row of tiles (just
active tree in this case). This just removes references from the tiling without
setting priorities to be empty. It's not really a justification of why we don't
do it, it's just a fact that we don't (at least IIRC). In general, when tilings
remove tiles, priorities aren't affected. From there, it's implicit that
whatever is released is not needed. 

I think invalidation case works, because we have
DidBecomeRecycled/DidBecomeActive things that play around with priorities.
However, on a long enough page, I think we will hit the dcheck once the live
tiles rect starts culling tiles. 

[reveman, 2014/07/22 16:51:08]

So I guess my next question is; does it make more sense to reset the priority
when removing references from a tiling than doing it here? When the live tile
rect moves and we decide to drop the top row of tiles it seems like it's a
change in priority of those tiles that causes us to do this and it might be
appropriate to have the actual tile priority reflect that. Also sounds like
doing it here would often cause us to reset it when not needed.

[vmpstr, 2014/07/22 17:23:34]

I'm not sure I like the approach of clearing the priority when we drop tiles in
the tiling. From the tiling's perspective, the tile is gone. It's kind of akin
to clearing some variables before calling delete on an object: it's a bit
awkward and not obvious why it's required.

The fact that released tiles end up in tile manager is something that tilings
aren't aware of. I think it's a bit of a can of worms, since it's not crystal
clear to me how we clear priorities on those tiles even if they don't end up in
the tile manager (ie, they are still alive on the twin tiling). I _think_ it's
DidBecomeRecycled or DidBecomeActive calls on the twin that end up clearing
these priorities.

I mean, in the end I would just still like to reiterate that I think the fact
that tile manager requires released tiles to have default priority so that it
doesn't schedule the tile again should be the responsibility of the tile
manager. I would hate to see a comment like "Clear priorities so tile manager
doesn't schedule this tile I'm about to delete" in the tiling. 

[reveman, 2014/07/22 18:02:47]

Yes, that would be the wrong reason to reset the priority. Does the tiling code
somehow know that all references to the tile are dropped? If it's just dropping
some reference to the tile but has no idea if more are alive then resetting the
priority makes sense IMO. I think it would be wrong to assume that the tile
priority would implicitly be reset just because a reference to the tile was
dropped.

I think a comment like this would be perfectly valid:
"Reset priority as tile is ref-counted and might still be used even though we no
longer hold a reference to it here anymore."

[sohanjg, 2014/07/23 13:05:41]

Done.
Have reset the priority and removed the dcheck.

   prioritized_tiles_dirty_ = true;
   released_tiles_.push_back(tile);
 }
@@ -418,18 +419,27 @@ bool TileManager::ShouldForceTasksRequiredForActivationToComplete() const {
   return global_state_.tree_priority != SMOOTHNESS_TAKES_PRIORITY;
 }
 
-void TileManager::CleanUpReleasedTiles() {
+void TileManager::FreeResourcesForReleasedTiles() {
   for (std::vector<Tile*>::iterator it = released_tiles_.begin();
        it != released_tiles_.end();
        ++it) {
     Tile* tile = *it;
-    ManagedTileState& mts = tile->managed_state();
+    FreeResourcesForTile(tile);
+  }
+}
 
-    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_);
+void TileManager::CleanUpReleasedTiles() {
+  DCHECK(prioritized_tiles_dirty_);

[vmpstr, 2014/07/21 15:24:14]

This is kind of weird. Why are we requiring this to be true? (please add a small
comment explaining)

[sohanjg, 2014/07/22 14:17:46]

hmm..we had earlier seen some crashes when we cleaned up released tiles from
DidFinishRunningTasks, because maybe prioritized tile set is affected when we
modify the tiles vector.
so we had put this extra protective code here.

[vmpstr, 2014/07/22 15:17:29]

Ah ok. This still makes it a bit fragile. 

The problem is that prioritized set is a view on tiles_ basically. It has no
ownership, it just returns pointers in a certain order. If we remove something
from tiles_ we have to ensure that the next thing we do is rebuild the
prioritized tile set, which I guess explains that DCHECK. The fragile part is
that prioritized_tiles_dirty_ is set elsewhere, so there is no strict guarantee
that we will rebuild the set (although it seems to be the case now). I guess
that's OK. Maybe add a comment in .h above this function saying that we have to
rebuild the set before accessing it?

[reveman, 2014/07/22 16:21:21]

After, right? The idea with this DCHECK is that we need to make sure that the
prioritized tile set is rebuild after calling this as we may otherwise use a set
with deleted tiles. A comment explaining this would be nice.

If this DCHECK fails, it's probably because of the call to this function in the
dtor as the call in ::UpdatePrioritizedTileSetIfNeeded() can of course enver
cause it to fail.

[vmpstr, 2014/07/22 16:31:02]

Yeah, after. My only worry is that we set prioritized_tiles_dirty_ to true in
DidChangeTilePriority for example, which allows this function to be called with
no promise of rebuilding the set. (DidFinishRunningTasks for instance doesn't
rebuild the set, even if this flag is set). 

[reveman, 2014/07/22 16:51:08]

Yea, the DCHECK is not perfect. We could just move all this code to
UpdatePrioritizedTileSetIfNeeded() instead. Just a bit ugly to have to call that
from the dtor..

[sohanjg, 2014/07/23 13:05:41]

Done.
Have kept the dcheck for now, and added a description and todo.

[sohanjg, 2014/07/23 14:16:32]

Avoided the dcheck as it breaks tests, added todo to move it to
UpdatePrioritizedTileSetIfNeeded, where rebuild set is ensured.

+  std::vector<Tile*>::iterator it = released_tiles_.begin();
+  while (it != released_tiles_.end()) {
+    Tile* tile = *it;
+
+    if (tile->HasRasterTask()) {
+      ++it;
+      continue;
     }
 
+    DCHECK(!tile->HasResources());
     DCHECK(tiles_.find(tile->id()) != tiles_.end());
     tiles_.erase(tile->id());
 
@@ -442,15 +452,15 @@ void TileManager::CleanUpReleasedTiles() {
     }
 
     delete tile;
+    it = released_tiles_.erase(it);
   }
-
-  released_tiles_.clear();
 }
 
 void TileManager::UpdatePrioritizedTileSetIfNeeded() {
   if (!prioritized_tiles_dirty_)
     return;
 
+  FreeResourcesForReleasedTiles();
   CleanUpReleasedTiles();
 
   prioritized_tiles_.Clear();
@@ -484,6 +494,8 @@ void TileManager::DidFinishRunningTasks() {
     return;
   }
 
+  FreeResourcesForReleasedTiles();
+
   resource_pool_->ReduceResourceUsage();
 
   // We don't reserve memory for required-for-activation tiles during
@@ -973,11 +985,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;
 }
 
@@ -1071,18 +1078,12 @@ void TileManager::OnRasterTaskCompleted(
     RasterMode raster_mode,
     const PicturePileImpl::Analysis& analysis,
     bool was_canceled) {
-  TileMap::iterator it = tiles_.find(tile_id);
-  if (it == tiles_.end()) {
-    ++update_visible_tiles_stats_.canceled_count;
-    resource_pool_->ReleaseResource(resource.Pass());
-    return;
-  }
+  DCHECK(tiles_.find(tile_id) != tiles_.end());
 
-  Tile* tile = it->second;
+  Tile* tile = tiles_[tile_id];
   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) {