Improve DamageTracker performance.

cc/trees/damage_tracker.cc

Index: cc/trees/damage_tracker.cc
diff --git a/cc/trees/damage_tracker.cc b/cc/trees/damage_tracker.cc
index 49bb634c1f6252425c37d7af4c41ddda9a61c8e1..5d61265ea5a58d384dfec53a6be8666d0f6b99d3 100644
--- a/cc/trees/damage_tracker.cc
+++ b/cc/trees/damage_tracker.cc
@@ -20,9 +20,7 @@ scoped_ptr<DamageTracker> DamageTracker::Create() {
   return make_scoped_ptr(new DamageTracker());
 }
 
-DamageTracker::DamageTracker()
-    : current_rect_history_(new RectMap),
-      next_rect_history_(new RectMap) {}
+DamageTracker::DamageTracker() {}
 
 DamageTracker::~DamageTracker() {}
 
@@ -107,21 +105,21 @@ void DamageTracker::UpdateDamageTrackingState(
   //
   // - To correctly manage exposed rects, two RectMaps are maintained:

[shawnsingh, 2013/11/21 09:56:03]

"two RectMaps" -- this line of comment should also be updated.

[ostap, 2013/11/21 17:06:23]

Done.

   //
-  //      1. The "current" map contains all the layer bounds that contributed to
+  //      1. All existing from the previous fram regions in map history are

[shawnsingh, 2013/11/21 09:56:03]

Some typos and clarifications - let's say "All existing rects from the previous
frame are marked as not updated"

[ostap, 2013/11/21 17:06:23]

Done.

+  //         marked not updated.
+  //      2. The map contains all the layer bounds that contributed to
   //         the previous frame (even outside the previous damaged area). If a
   //         layer changes or does not exist anymore, those regions are then
   //         exposed and damage the target surface. As the algorithm progresses,
-  //         entries are removed from the map until it has only leftover layers
-  //         that no longer exist.
+  //         entries are updated in the map until only leftover layers
+  //         that no longer exist stay marked not updated.
   //
-  //      2. The "next" map starts out empty, and as the algorithm progresses,
-  //         every layer/surface that contributes to the surface is added to the
-  //         map.
-  //
-  //      3. After the damage rect is computed, the two maps are swapped, so
-  //         that the damage tracker is ready for the next frame.
+  //      3. After the damage rect is computed, the leftover not marked regions
+  //         in a map are used to compute are damaged by deleted layers and
+  //         erased from map.
   //
 
+  PrepareRectHistoryForUpdate();
   // These functions cannot be bypassed with early-exits, even if we know what
   // the damage will be for this frame, because we need to update the damage
   // tracker state to correctly track the next frame.
@@ -154,31 +152,16 @@ void DamageTracker::UpdateDamageTrackingState(
   // Damage accumulates until we are notified that we actually did draw on that
   // frame.
   current_damage_rect_.Union(damage_rect_for_this_update);
-
-  // The next history map becomes the current map for the next frame. Note this
-  // must happen every frame to correctly track changes, even if damage
-  // accumulates over multiple frames before actually being drawn.
-  swap(current_rect_history_, next_rect_history_);
 }
 
-gfx::RectF DamageTracker::RemoveRectFromCurrentFrame(int layer_id,
-                                                     bool* layer_is_new) {
-  RectMap::iterator iter = current_rect_history_->find(layer_id);
-  *layer_is_new = iter == current_rect_history_->end();
+DamageTracker::RectMapData& DamageTracker::RectDataForLayer(
+                              int layer_id, bool* layer_is_new) {
+  RectMap::iterator iter = rect_history_.find(layer_id);
+  *layer_is_new = iter == rect_history_.end();

[shawnsingh, 2013/11/21 09:56:03]

For clarity here let's use parentheses around the boolean condition.

[ostap, 2013/11/21 17:06:23]

Done.

   if (*layer_is_new)
-    return gfx::RectF();
+    return rect_history_[layer_id]; // force element creation
 
-  gfx::RectF ret = iter->second;
-  current_rect_history_->erase(iter);
-  return ret;
-}
-
-void DamageTracker::SaveRectForNextFrame(int layer_id,
-                                         const gfx::RectF& target_space_rect) {
-  // This layer should not yet exist in next frame's history.
-  DCHECK_GT(layer_id, 0);
-  DCHECK(next_rect_history_->find(layer_id) == next_rect_history_->end());
-  (*next_rect_history_)[layer_id] = target_space_rect;
+  return iter->second;
 }
 
 gfx::RectF DamageTracker::TrackDamageFromActiveLayers(
@@ -225,6 +208,14 @@ gfx::RectF DamageTracker::TrackDamageFromSurfaceMask(
   return damage_rect;
 }
 
+void DamageTracker::PrepareRectHistoryForUpdate() {

[shawnsingh, 2013/11/21 09:56:03]

Let's use an integer RectMapData::mailboxId_  instead of a boolean
RectMapData.updated_  - that way, we can avoid this entire loop that exists
simply to invalidate each rect.  Instead, incrementing the mailbox ID would be
equivalent to automatically dirtying all entries in the map.

+  for (RectMap::iterator it = rect_history_.begin();
+       it != rect_history_.end();
+       ++it) {
+    it->second.updated_ = false;
+  }
+}
+
 gfx::RectF DamageTracker::TrackDamageFromLeftoverRects() {
   // After computing damage for all active layers, any leftover items in the
   // current rect history correspond to layers/surfaces that no longer exist.
@@ -232,12 +223,18 @@ gfx::RectF DamageTracker::TrackDamageFromLeftoverRects() {
 
   gfx::RectF damage_rect = gfx::RectF();
 
-  for (RectMap::iterator it = current_rect_history_->begin();
-       it != current_rect_history_->end();
-       ++it)
-    damage_rect.Union(it->second);
+  for (RectMap::iterator it = rect_history_.begin();
+       it != rect_history_.end(); ) {
 
-  current_rect_history_->clear();
+    if (it->second.updated_)

[danakj, 2013/11/21 00:13:40]

{} here, since else has them

[ostap, 2013/11/21 17:06:23]

Done.

+      ++it;
+    else {
+      damage_rect.Union(it->second.rect_);
+      RectMap::iterator erase_it = it;
+      ++it;
+      rect_history_.erase(erase_it);

[shawnsingh, 2013/11/21 09:56:03]

I don't know about base::HashMap, but for std:: containers, we cannot continue
to use an iterator after adding/removing elements of a list.  There is a good
chance we will want to change container types some time for crbug.com/230050 -
probably a std::vector, so unfortunately I think this code needs to change.

Right now I can't think of how to implement this more robustly without adding
more queries to the container.  Let's discuss ideas if you have any =)

[ostap, 2013/11/21 17:06:23]

Done.

+    }
+  }
 
   return damage_rect;
 }
@@ -263,13 +260,13 @@ void DamageTracker::ExtendDamageForLayer(LayerImpl* layer,
   // ancestor surface, ExtendDamageForRenderSurface() must be called instead.
 
   bool layer_is_new = false;
-  gfx::RectF old_rect_in_target_space =
-      RemoveRectFromCurrentFrame(layer->id(), &layer_is_new);
+  RectMapData& data = RectDataForLayer(layer->id(), &layer_is_new);
+  gfx::RectF old_rect_in_target_space = data.rect_;
 
   gfx::RectF rect_in_target_space = MathUtil::MapClippedRect(
       layer->draw_transform(),
       gfx::RectF(gfx::PointF(), layer->content_bounds()));
-  SaveRectForNextFrame(layer->id(), rect_in_target_space);
+  data.Update(rect_in_target_space);
 
   if (layer_is_new || layer->LayerPropertyChanged()) {
     // If a layer is new or has changed, then its entire layer rect affects the
@@ -310,13 +307,13 @@ void DamageTracker::ExtendDamageForRenderSurface(
   RenderSurfaceImpl* render_surface = layer->render_surface();
 
   bool surface_is_new = false;
-  gfx::RectF old_surface_rect = RemoveRectFromCurrentFrame(layer->id(),
-                                                           &surface_is_new);
+  RectMapData& data = RectDataForLayer(layer->id(), &surface_is_new);
+  gfx::RectF old_surface_rect = data.rect_;
 
   // The drawableContextRect() already includes the replica if it exists.
   gfx::RectF surface_rect_in_target_space =
       render_surface->DrawableContentRect();
-  SaveRectForNextFrame(layer->id(), surface_rect_in_target_space);
+  data.Update(surface_rect_in_target_space);
 
   gfx::RectF damage_rect_in_local_space;
   if (surface_is_new || render_surface->SurfacePropertyChanged()) {
@@ -353,14 +350,15 @@ void DamageTracker::ExtendDamageForRenderSurface(
     LayerImpl* replica_mask_layer = layer->replica_layer()->mask_layer();
 
     bool replica_is_new = false;
-    RemoveRectFromCurrentFrame(replica_mask_layer->id(), &replica_is_new);
+    RectMapData& data = RectDataForLayer(replica_mask_layer->id(),
+                              &replica_is_new);

[danakj, 2013/11/21 00:13:40]

indenting off, git cl format please

[ostap, 2013/11/21 17:06:23]

Done.

 
     const gfx::Transform& replica_draw_transform =
         render_surface->replica_draw_transform();
     gfx::RectF replica_mask_layer_rect = MathUtil::MapClippedRect(
         replica_draw_transform,
         gfx::RectF(gfx::PointF(), replica_mask_layer->bounds()));
-    SaveRectForNextFrame(replica_mask_layer->id(), replica_mask_layer_rect);
+    data.Update(replica_mask_layer_rect);
 
     // In the current implementation, a change in the replica mask damages the
     // entire replica region.