CC/GPU: Add a soft limit to the compositor.

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 4f9f0aed6545b1792c93111f34313194c1268288..08a1e22d7a6bbb139af41c5ec6cb5e9f984c44a0 100644
--- a/cc/resources/tile_manager.cc
+++ b/cc/resources/tile_manager.cc
@@ -477,12 +477,13 @@ void TileManager::GetTilesWithAssignedBins(PrioritizedTileSet* tiles) {
 void TileManager::ManageTiles(const GlobalStateThatImpactsTilePriority& state) {
   TRACE_EVENT0("cc", "TileManager::ManageTiles");
 
-  // Update internal state.
+  // Update internal state. We use the soft-limit for the resource-pool
+  // such that we will reduce to that limit after going over.
   if (state != global_state_) {
     global_state_ = state;
     prioritized_tiles_dirty_ = true;
     resource_pool_->SetResourceUsageLimits(
-        global_state_.memory_limit_in_bytes,
+        global_state_.soft_memory_limit_in_bytes,
         global_state_.unused_memory_limit_in_bytes,
         global_state_.num_resources_limit);
   }
@@ -618,21 +619,30 @@ void TileManager::AssignGpuMemoryToTiles(
   all_tiles_required_for_activation_have_memory_ = true;
 
   // Cast to prevent overflow.
-  int64 bytes_available =
+  int64 soft_bytes_available =

[vmpstr, 2014/02/12 22:19:36]

maybe visible_bytes_available and repaint_bytes_available?

[vmpstr, 2014/02/12 22:20:20]

s/repaint/prepaint/

[epennerAtGoogle, 2014/02/12 23:08:46]

Arg! That's what I called it before! ;)  I can change it back if that's the
preference, but actually there is some details that makes 'visible' not quite
what we are after (at least at the moment). See below.

[vmpstr, 2014/02/12 23:21:04]

Yeah, I see the problem. I guess soft/hard is OK, I just have to get used to it
a little bit as the mapping of soft to prepaint and hard to visible isn't very
well established in my brain :P

       static_cast<int64>(bytes_releasable_) +
-      static_cast<int64>(global_state_.memory_limit_in_bytes) -
+      static_cast<int64>(global_state_.soft_memory_limit_in_bytes) -
+      static_cast<int64>(resource_pool_->acquired_memory_usage_bytes());
+  int64 hard_bytes_available =
+      static_cast<int64>(bytes_releasable_) +
+      static_cast<int64>(global_state_.hard_memory_limit_in_bytes) -
       static_cast<int64>(resource_pool_->acquired_memory_usage_bytes());
   int resources_available = resources_releasable_ +
                             global_state_.num_resources_limit -
                             resource_pool_->acquired_resource_count();
-
-  size_t bytes_allocatable = std::max(static_cast<int64>(0), bytes_available);
+  size_t soft_bytes_allocatable =
+      std::max(static_cast<int64>(0), soft_bytes_available);
+  size_t hard_bytes_allocatable =
+      std::max(static_cast<int64>(0), hard_bytes_available);
   size_t resources_allocatable = std::max(0, resources_available);
 
   size_t bytes_that_exceeded_memory_budget = 0;
-  size_t bytes_left = bytes_allocatable;
+  int64 soft_bytes_left = soft_bytes_allocatable;
+  int64 hard_bytes_left = hard_bytes_allocatable;
+
   size_t resources_left = resources_allocatable;
-  bool oomed = false;
+  bool oomed_soft = false;
+  bool oomed_hard = false;
 
   // Memory we assign to raster tasks now will be deducted from our memory
   // in future iterations if priorities change. By assigning at most half
@@ -664,12 +674,15 @@ void TileManager::AssignGpuMemoryToTiles(
       continue;
     }
 
-    size_t bytes_if_allocated = BytesConsumedIfAllocated(tile);
-    size_t raster_bytes_if_rastered = raster_bytes + bytes_if_allocated;
+    const size_t bytes_if_allocated = BytesConsumedIfAllocated(tile);
+    const size_t raster_bytes_if_rastered = raster_bytes + bytes_if_allocated;
+    const size_t tile_bytes_left =
+        (mts.bin == NOW_BIN) ? hard_bytes_left : soft_bytes_left;

[vmpstr, 2014/02/12 22:19:36]

So in SMOOTHNESS_TAKES_PRIORITY, we prioritize the active tree. However, it is
possible that the pending tree has this tile in the NOW_BIN (even though active
tree has it as SOON or EVENTUALLY or even NEVER). I think this means that the
mts.bin == NOW_BIN is not sufficient to check visibility. Also, it means that
"visible" tiles might come after prepaint tiles in the priority ordering,
something that I think your patch assumes doesn't happen.

I think we should still be judging visibility as something that is visible on
either tree, wdyt? In fact, I think that should be some sort of a tile setting
like "visible on either tree"

Also, that's from me looking at the code in GetTilesWithAssignedBins, which is
kinda complicated. Feel free to correct me if I got it wrong.

[epennerAtGoogle, 2014/02/12 23:08:46]

I had to deal with this sticky case before when we were trying to avoid
raster-on-demand at low memory limits. That was a problem of assuming that
visible 'required-to-activate' tiles always come first. I believe this code is
okay, but we should be double-sure, so please let me know if there is anything
wrong with my thinking:

In GetTilesWithAssignedBins, when we are in SMOOTHNESS we use the active-tree's
priority for all tiles:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/tile_...

What this means is that some visible 'required-to-activate' tiles' mts.bin will
not be set to NOW_BIN. However, similar to raster-on-demand, we don't actually
strictly need those tiles when we are near the soft limit. We can simply delay
activation instead, as we do here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/tile_...

So visibility isn't quite the indicator of OOM when we are in smoothness mode.
Once the user exits smoothness mode, all the tiles will be in the NOW_BIN and
we'll need more memory at that time, and will use the hard-limit temporarily to
activate.

I actually don't like this policy in the long run, and I think we should always
reserve memory for visible tiles first. This requires us to be able to reserve
memory using one ordering, while painting that memory with a different
ordering... I haven't thought about how this will work in the future design, but
it's probably worth going over it.

OH! And it should be mts.bin <= NOW_BIN by the looks of it. I'll change that.

[vmpstr, 2014/02/12 23:21:04]

I guess the exception is NEW_CONTENT_TAKES_PRIORITY, but tbh I have no idea when
we enter this mode. However, in there we do want active tree now bin tiles to
use the hard memory limit, since otherwise we might checkerboard.

Overall, I can buy that we only want hard limit to be used for what the tile
manager itself decides is now bin (which might exclude some pending visible
tiles). In that spirit though, could you add an explicit comment somewhere
around here to explain this discussion? It almost looks like we care about
visible tiles, and I want to make it clear that we know that it's just NOW_BIN
(or less) that we care about.
Yeah, I think in the future I want tile manager to "know" that a tile is
visible. Period. It doesn't matter which tree it comes from or whether it exists
on one tree or both, just the fact that it's visible somewhere should be an
indicator of the tile's importance. But maybe that's simplistic. 
Good catch :)

 
     size_t tile_bytes = 0;
     size_t tile_resources = 0;
 
+

[reveman, 2014/02/12 20:08:02]

nit: no need for this line. I'm surprised the presubmit check didn't complain
about this..

[epennerAtGoogle, 2014/02/12 21:54:34]

Sorry I didn't rerun git cl format. Done.

     // It costs to maintain a resource.
     for (int mode = 0; mode < NUM_RASTER_MODES; ++mode) {
       if (mts.tile_versions[mode].resource_) {
@@ -691,7 +704,7 @@ void TileManager::AssignGpuMemoryToTiles(
     }
 
     // Tile is OOM.
-    if (tile_bytes > bytes_left || tile_resources > resources_left) {
+    if (tile_bytes > tile_bytes_left || tile_resources > resources_left) {
       FreeResourcesForTile(tile);
 
       // This tile was already on screen and now its resources have been
@@ -700,10 +713,16 @@ void TileManager::AssignGpuMemoryToTiles(
       if (mts.visible_and_ready_to_draw)
         tile_version.set_rasterize_on_demand();
 
-      oomed = true;
-      bytes_that_exceeded_memory_budget += tile_bytes;
+      oomed_soft = true;
+      if (mts.bin == NOW_BIN) {
+        oomed_hard = true;
+        bytes_that_exceeded_memory_budget += tile_bytes;
+      }
     } else {
-      bytes_left -= tile_bytes;
+      hard_bytes_left -= tile_bytes;
+      soft_bytes_left -= tile_bytes;
+      soft_bytes_left = std::max<int64>(0, soft_bytes_left);

[reveman, 2014/02/12 20:08:02]

does it hurt to let this go negative? in that case, can you adjust the types
above so it doesn't instead of having to do std::max here?

[epennerAtGoogle, 2014/02/12 21:54:34]

Do you mean make it unsigned and add a if-statement here to avoid overflow here
instead of std::max()?

I was going to use size_t but I changed to signed since this is more concise
(less code than preventing overflow would be). I totally don't mind changing it
to size_t if that is the normal pattern though. 

[reveman, 2014/02/12 22:14:52]

I was thinking you'd keep this signed and just let it become negative instead of
doing std::max. I think that'd be the cleanest but you should probably change a
few of the types above to signed too in the case.

+
       resources_left -= tile_resources;
 
       if (tile_version.resource_)
@@ -720,7 +739,7 @@ void TileManager::AssignGpuMemoryToTiles(
     // 1. Tile size should not impact raster priority.
     // 2. Tiles with existing raster task could otherwise incorrectly
     //    be added as they are not affected by |bytes_allocatable|.
-    if (oomed || raster_bytes_if_rastered > max_raster_bytes) {
+    if (oomed_soft || raster_bytes_if_rastered > max_raster_bytes) {
       all_tiles_that_need_to_be_rasterized_have_memory_ = false;
       if (tile->required_for_activation())
         all_tiles_required_for_activation_have_memory_ = false;
@@ -732,22 +751,23 @@ void TileManager::AssignGpuMemoryToTiles(
     tiles_that_need_to_be_rasterized->push_back(tile);
   }
 
-  ever_exceeded_memory_budget_ |= bytes_that_exceeded_memory_budget > 0;
+  // OOM reporting uses hard-limit, soft-OOM is normal depending on limit.
+  ever_exceeded_memory_budget_ |= oomed_hard;
   if (ever_exceeded_memory_budget_) {
     TRACE_COUNTER_ID2("cc",
                       "over_memory_budget",
                       this,
                       "budget",
-                      global_state_.memory_limit_in_bytes,
+                      global_state_.hard_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;
+      global_state_.hard_memory_limit_in_bytes;
   memory_stats_from_last_assign_.bytes_allocated =
-      bytes_allocatable - bytes_left;
+      hard_bytes_allocatable - hard_bytes_left;
   memory_stats_from_last_assign_.bytes_unreleasable =
-      bytes_allocatable - bytes_releasable_;
+      hard_bytes_allocatable - bytes_releasable_;
   memory_stats_from_last_assign_.bytes_over = bytes_that_exceeded_memory_budget;
 }