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..dfeb199353a6a0bc50f52742a689e151af15aa0f 100644
--- a/cc/trees/damage_tracker.cc
+++ b/cc/trees/damage_tracker.cc
@@ -21,8 +21,7 @@ scoped_ptr<DamageTracker> DamageTracker::Create() {
 }
 
 DamageTracker::DamageTracker()
-    : current_rect_history_(new RectMap),
-      next_rect_history_(new RectMap) {}
+  : mailboxId_(0) {}
 
 DamageTracker::~DamageTracker() {}
 
@@ -105,23 +104,23 @@ void DamageTracker::UpdateDamageTrackingState(
   // - See comments in the rest of the code to see what exactly is considered a
   //   "change" in a layer/surface.
   //
-  // - To correctly manage exposed rects, two RectMaps are maintained:
+  // - To correctly manage exposed rects, SortedRectMap is maintained:
   //
-  //      1. The "current" map contains all the layer bounds that contributed to
+  //      1. All existing rects from the previous frame are marked as
+  //         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 +153,23 @@ 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();
-  if (*layer_is_new)
-    return gfx::RectF();
+DamageTracker::RectMapData& DamageTracker::RectDataForLayer(
+    int layer_id,
+    bool* layer_is_new) {
 
-  gfx::RectF ret = iter->second;
-  current_rect_history_->erase(iter);
-  return ret;
-}
+  RectMapData data(layer_id);
+
+  SortedRectMap::iterator it = lower_bound(rect_history_.begin(),

[shawnsingh, 2013/11/23 01:32:01]

let's explicitly write std::lower_bound here, and everywhere else we're using
std:: algorithm stuff.

[ostap, 2013/11/25 16:11:39]

Done.

+    rect_history_.end(), data);
+
+  if (it == rect_history_.end() || it->layer_id_ != layer_id) {
+    *layer_is_new = true;
+    it = rect_history_.insert(it, data);
+  }
 
-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 *it;
 }
 
 gfx::RectF DamageTracker::TrackDamageFromActiveLayers(
@@ -225,19 +216,35 @@ gfx::RectF DamageTracker::TrackDamageFromSurfaceMask(
   return damage_rect;
 }
 
+void DamageTracker::PrepareRectHistoryForUpdate() {
+  mailboxId_++;
+}
+
 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.
   // So, these regions are now exposed on the target surface.
 
   gfx::RectF damage_rect = gfx::RectF();
+  SortedRectMap::iterator copy_pos;

[shawnsingh, 2013/11/23 01:32:01]

Something doesn't seem right here, copy_pos could be uninitialized when it gets
tested for equality and incremented below.

I understand that the intention of this code is to find layers who were outdated
and remove them.  But I am finding it hard to decipher how this code actually
does that.  Could you please add a quick comment inside the for-loop explaining
what each iteration does, and if possible, use static inlined helper functions
whose names can help self-document the code, too?

[ostap, 2013/11/25 16:11:39]

copy_pos is always initialized to rect_history_.begin() .
Probably I should rewrite the assignment.
Done

I changed it a bit to look more like std::remove_if . Might be it is easier to
understand this way.

+  SortedRectMap::iterator cur_pos = copy_pos = rect_history_.begin();
+  for ( ; cur_pos < rect_history_.end(); ++cur_pos) {
+    if (cur_pos->mailboxId_ == mailboxId_) {
+      if (cur_pos != copy_pos)
+        *copy_pos = *cur_pos;
+
+      ++copy_pos;
+    } else {
+      damage_rect.Union(cur_pos->rect_);
+    }
+  }
 
-  for (RectMap::iterator it = current_rect_history_->begin();
-       it != current_rect_history_->end();
-       ++it)
-    damage_rect.Union(it->second);
+  if (copy_pos != rect_history_.end())
+    rect_history_.erase(copy_pos, rect_history_.end());
 
-  current_rect_history_->clear();
+  // If the vector has excessive storage is too big, shrink it
+  if (rect_history_.capacity() > rect_history_.size() * 4)
+    SortedRectMap(rect_history_).swap(rect_history_);
 
   return damage_rect;
 }
@@ -263,13 +270,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, mailboxId_);
 
   if (layer_is_new || layer->LayerPropertyChanged()) {
     // If a layer is new or has changed, then its entire layer rect affects the
@@ -310,13 +317,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, mailboxId_);
 
   gfx::RectF damage_rect_in_local_space;
   if (surface_is_new || render_surface->SurfacePropertyChanged()) {
@@ -353,14 +360,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);
 
     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, mailboxId_);
 
     // In the current implementation, a change in the replica mask damages the
     // entire replica region.