cc: Replace recorded region with direct map lookup

cc/resources/picture_pile_base.cc

Index: cc/resources/picture_pile_base.cc
diff --git a/cc/resources/picture_pile_base.cc b/cc/resources/picture_pile_base.cc
index 2c87f596c800b708d2e6f3218397b420f1942a31..20e85d68eed2a8c16c2c9ea29f96c96f1630cf57 100644
--- a/cc/resources/picture_pile_base.cc
+++ b/cc/resources/picture_pile_base.cc
@@ -44,7 +44,8 @@ PicturePileBase::PicturePileBase()
       slow_down_raster_scale_factor_for_debug_(0),
       contents_opaque_(false),
       show_debug_picture_borders_(false),
-      clear_canvas_with_debug_color_(kDefaultClearCanvasSetting) {
+      clear_canvas_with_debug_color_(kDefaultClearCanvasSetting),
+      has_any_recordings_(false) {
   tiling_.SetMaxTextureSize(gfx::Size(kBasePictureSize, kBasePictureSize));
   tile_grid_info_.fTileInterval.setEmpty();
   tile_grid_info_.fMargin.setEmpty();
@@ -54,7 +55,7 @@ PicturePileBase::PicturePileBase()
 PicturePileBase::PicturePileBase(const PicturePileBase* other)
     : picture_map_(other->picture_map_),
       tiling_(other->tiling_),
-      recorded_region_(other->recorded_region_),
+      recorded_viewport_(other->recorded_viewport_),
       min_contents_scale_(other->min_contents_scale_),
       tile_grid_info_(other->tile_grid_info_),
       background_color_(other->background_color_),
@@ -62,13 +63,13 @@ PicturePileBase::PicturePileBase(const PicturePileBase* other)
           other->slow_down_raster_scale_factor_for_debug_),
       contents_opaque_(other->contents_opaque_),
       show_debug_picture_borders_(other->show_debug_picture_borders_),
-      clear_canvas_with_debug_color_(other->clear_canvas_with_debug_color_) {
-}
+      clear_canvas_with_debug_color_(other->clear_canvas_with_debug_color_),
+      has_any_recordings_(other->has_any_recordings_) {}
 
-PicturePileBase::PicturePileBase(
-    const PicturePileBase* other, unsigned thread_index)
+PicturePileBase::PicturePileBase(const PicturePileBase* other,
+                                 unsigned thread_index)
     : tiling_(other->tiling_),
-      recorded_region_(other->recorded_region_),
+      recorded_viewport_(other->recorded_viewport_),
       min_contents_scale_(other->min_contents_scale_),
       tile_grid_info_(other->tile_grid_info_),
       background_color_(other->background_color_),
@@ -76,7 +77,8 @@ PicturePileBase::PicturePileBase(
           other->slow_down_raster_scale_factor_for_debug_),
       contents_opaque_(other->contents_opaque_),
       show_debug_picture_borders_(other->show_debug_picture_borders_),
-      clear_canvas_with_debug_color_(other->clear_canvas_with_debug_color_) {
+      clear_canvas_with_debug_color_(other->clear_canvas_with_debug_color_),
+      has_any_recordings_(other->has_any_recordings_) {
   for (PictureMap::const_iterator it = other->picture_map_.begin();
        it != other->picture_map_.end();
        ++it) {
@@ -111,6 +113,11 @@ void PicturePileBase::Resize(const gfx::Size& new_size) {
 
   for (size_t i = 0; i < to_erase.size(); ++i)
     picture_map_.erase(to_erase[i]);
+
+  // Don't waste time in Resize figuring out what these hints should be.

[vmpstr, 2014/03/12 23:57:35]

Is this because Resize is (always?) followed by Update? What I mean to say is
that it's not that expensive to recompute this, since we're already walking
through the whole picture_map_, but if we always follow-up with Update, then I
can see how that time is wasted.

[enne (OOO), 2014/03/13 01:35:43]

Well, calculating what the "viewport" here is would require rounding the resized
viewport rect to tile boundaries.  I just didn't want to do the math or figure
out what the right rect was exactly.

+  recorded_viewport_ = gfx::Rect();
+  if (new_size.IsEmpty())
+    has_any_recordings_ = false;

[vmpstr, 2014/03/12 23:57:35]

This one in particular, I think if you add a has_any_recordings_ |=
it->second.GetPicture() where the continue is on line 110 would eliminate some
false positives.

[enne (OOO), 2014/03/13 01:35:43]

Done.

 }
 
 void PicturePileBase::SetMinContentsScale(float min_contents_scale) {
@@ -166,18 +173,6 @@ void PicturePileBase::Clear() {
   picture_map_.clear();
 }
 
-void PicturePileBase::UpdateRecordedRegion() {
-  recorded_region_.Clear();
-  for (PictureMap::const_iterator it = picture_map_.begin();
-       it != picture_map_.end();
-       ++it) {
-    if (it->second.GetPicture()) {
-      const PictureMapKey& key = it->first;
-      recorded_region_.Union(tile_bounds(key.first, key.second));
-    }
-  }
-}
-
 bool PicturePileBase::HasRecordingAt(int x, int y) {
   PictureMap::const_iterator found = picture_map_.find(PictureMapKey(x, y));
   if (found == picture_map_.end())
@@ -192,7 +187,28 @@ bool PicturePileBase::CanRaster(float contents_scale,
   gfx::Rect layer_rect = gfx::ScaleToEnclosingRect(
       content_rect, 1.f / contents_scale);
   layer_rect.Intersect(gfx::Rect(tiling_.total_size()));
-  return recorded_region_.Contains(layer_rect);
+
+  // Common case inside of viewport to avoid the slower map lookups.
+  if (recorded_viewport_.Contains(layer_rect)) {
+    // Sanity check that there are no false positives in recorded_viewport_.
+    DCHECK(CanRasterSlowTileCheck(layer_rect));
+    return true;
+  }
+
+  return CanRasterSlowTileCheck(layer_rect);
+}
+
+bool PicturePileBase::CanRasterSlowTileCheck(

[vmpstr, 2014/03/12 23:57:35]

I don't really buy that this function is that slow :) At least when compared to
the previous version of "CanRaster"

[enne (OOO), 2014/03/13 01:35:43]

PicturePileBase::CanRasterNotAsSlowAsPreviousVersionButProbablyNotTooSpeedySinceItIsDoingSomeHashMapLookups

[vmpstr, 2014/03/13 16:30:35]

I like it!

+    const gfx::Rect& layer_rect) const {
+  for (TilingData::Iterator tile_iter(&tiling_, layer_rect); tile_iter;
+       ++tile_iter) {
+    PictureMap::const_iterator map_iter = picture_map_.find(tile_iter.index());
+    if (map_iter == picture_map_.end())
+      return false;
+    if (!map_iter->second.GetPicture())
+      return false;
+  }
+  return true;
 }
 
 gfx::Rect PicturePileBase::PaddedRect(const PictureMapKey& key) {