Send memory management policies to the tile manager

Send memory management policies to the tile manager

Replace the priority cutoff value in ManagedMemoryPolicy
with an enum, which can be converted into a cutoff value
for the PrioritizedResourceManager or a policy for the
TileManager.

Remove ResourcePool functions for dropping memory consumption
because they will leave Resource pointers dangling.

BUG=163029
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174883

[nduca, 2012-12-19 21:46:32]

some assorted shreds of thought

https://codereview.chromium.org/11637022/diff/1/cc/gl_renderer.h
File cc/gl_renderer.h (right):

https://codereview.chromium.org/11637022/diff/1/cc/gl_renderer.h#newcode92
cc/gl_renderer.h:92: static ManagedMemoryPolicy::PriorityCutoff
priorityCutoffValue(WebKit::WebGraphicsMemoryAllocation::PriorityCutoff);
When I looked at this last, i was thinking we had one too many conversions
between data types going on here, once you consider impl side painting. I was
thinking of just passing wgma struct up the renderer client. We only needed the
managed memory policy in old modes, and in impl side painting, we could then
make the tile manager use those wgma types and enums directly...

[observation: we want to move the memory manager callbacks to the output surface
anyway, right? We'd need memory managment in non3d and delegate dmodes...
sharing types is probably useful. And possibly related, output surface might be
easier to set up to use chrome types and avoid the whole web type nightmare]

https://codereview.chromium.org/11637022/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/11637022/diff/1/cc/layer_tree_host_impl.cc#ol...
cc/layer_tree_host_impl.cc:807: m_tileManager->SetGlobalState(new_state);
I think we might want a to just pass the enforce call into tile manager as
something like TemporarilySetGlobalState that uses a provided globalState, calls
manage tiles, but then rolls back to the old state.

https://codereview.chromium.org/11637022/diff/1/cc/managed_memory_policy.h
File cc/managed_memory_policy.h (right):

https://codereview.chromium.org/11637022/diff/1/cc/managed_memory_policy.h#ne...
cc/managed_memory_policy.h:16: enum PriorityCutoff {
Can we use one enum everywhere? Maybe we should make tile manager use whatever
enum is coming from the renderer callback?

https://codereview.chromium.org/11637022/diff/1/cc/tile_manager.cc
File cc/tile_manager.cc (left):

https://codereview.chromium.org/11637022/diff/1/cc/tile_manager.cc#oldcode194
cc/tile_manager.cc:194:
resource_pool_->SetMaxMemoryUsageBytes(global_state_.memory_limit_in_bytes);
why remove this? :)

[ccameron, 2012-12-19 22:22:10]

> When I looked at this last, i was thinking we had one too many conversions
> between data types going on here, once you consider impl side painting. I was
> thinking of just passing wgma struct up the renderer client.

> Can we use one enum everywhere? Maybe we should make tile manager use whatever
> enum is coming from the renderer callback?

I'm totally in favor of consolidating all of the types. But what to? I was under
the impression that we didn't want to be sending WebKit types too deep into the
cc stack.

Of note is that we're doing
content::GpuMemoryAllocationForRenderer::PriorityCutoff ->
WebKit::WebGraphicsMemoryAllocation::PriorityCutoff conversion. Maybe we can
convert back to the original type (we'll have to find a non-layering-violiation
place for it)

> And possibly related, output surface might be
> easier to set up to use chrome types and avoid the whole web type nightmare]

I'd like to cut WebKit out of the loop entirely, but we do have some other
WebKit-embedded clients (which I'm not sure are too effective, but last time I
tried to delete them, it was voted down).

> resource_pool_->SetMaxMemoryUsageBytes(global_state_.memory_limit_in_bytes);
> why remove this? :)

That seemed like a really dangerous call -- it deletes resources that may be
being pointed to by the TileManager. It seems like all enforcement of limits
should be done at the TileManager level.

[nduca, 2012-12-20 18:10:05]

> I'm totally in favor of consolidating all of the types. But what to? I was
under
> the impression that we didn't want to be sending WebKit types too deep into
the
> cc stack.
Yeah I dont quite grok that. But at point, you want GraphicsMemoryAllocation
right and it to be pretty 1:1 with the web type? Dunno thinking out loud.

> I'd like to cut WebKit out of the loop entirely, but we do have some other
> WebKit-embedded clients (which I'm not sure are too effective, but last time I
> tried to delete them, it was voted down).
Yeah we probably need to send some of this to webkit, but it might be less than
the chrome-side type requires now that we're gtfo.

> > resource_pool_->SetMaxMemoryUsageBytes(global_state_.memory_limit_in_bytes);
> > why remove this? :)
> 
> That seemed like a really dangerous call -- it deletes resources that may be
> being pointed to by the TileManager. It seems like all enforcement of limits
> should be done at the TileManager level.

Things in the pool are free. The pool is a freelist. I.e. they're not in use.

[ccameron, 2012-12-20 21:02:47]

Okay, if I can use the WebKit type directly, that removes a lot of the mess. I
still translate to either an int or a TileManager priority in the "enforce"
function.

I have some FIXMEs still left for some issues that need to be addressed, but we
should chat in person about the larger scale design issues involved first.

I removed the Pool stuff cause the change wasn't right.

[enne (OOO), 2012-12-20 23:14:35]

For what it's worth, I think using WebKit types directly is the wrong direction.
 cc is moving away from including anything from WebKit.

[ccameron, 2012-12-26 23:55:54]

Removing the "not ready for review" part of the title, after getting feedback.

We're leaving in the "tons of conversions" between types, with the understanding
that we'd like to converge them all if we get a chance. Some will be collapsed
if we switch to only using the impl-side painting, others will be collapsed if
we take WebKit out of the management loop.

https://codereview.chromium.org/11637022/diff/10001/cc/managed_memory_policy.cc
File cc/managed_memory_policy.cc (right):

https://codereview.chromium.org/11637022/diff/10001/cc/managed_memory_policy....
cc/managed_memory_policy.cc:46: switch (priorityCutoff) {
These are just 1:1 mappings of different types.

https://codereview.chromium.org/11637022/diff/10001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11637022/diff/10001/cc/tile_manager.cc#newcode46
cc/tile_manager.cc:46: if (prio.distance_to_visible_in_pixels >
maximum_paint_distance_pixels)
I discussed this with Nat earlier. If we allow the renderer to use, say, 1GB of
memory, and the page is really long, the tile manager will go ahead and burn
through all of that. This prevents any of that sort of insanity.

There are other options here (like, allow 4k pixels of low-res and 2k pixels of
high-res), or something like that. Let me know if that's preferred.

https://codereview.chromium.org/11637022/diff/10001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:200: ManageTiles();
Be sure to verify whether or not this change is acceptable. This makes it so
that if we say "change your memory policy", then that call immediately has an
effect.

After being told "change your memory policy", we report the statistics of the
new policy to the GPU memory manager -- without doing this change immediately,
we will just report the old statistics (confusing everyone).

https://codereview.chromium.org/11637022/diff/10001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:294: mts.gpu_memmgr_stats_bin =
BinFromTilePriority(tile->combined_priority());
I keep this bin around permanently so that we may compute statistics from it --
this is the behavior if the GPU memory manager were maximally permissive.

[enne (OOO), 2012-12-27 22:06:39]

I don't think I feel comfortable reviewing this patch in isolation without
giving Nat a chance to look at this final version first.

https://codereview.chromium.org/11637022/diff/10001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11637022/diff/10001/cc/tile_manager.cc#newcode46
cc/tile_manager.cc:46: if (prio.distance_to_visible_in_pixels >
maximum_paint_distance_pixels)
On 2012/12/26 23:55:54, ccameron1 wrote:
> I discussed this with Nat earlier. If we allow the renderer to use, say, 1GB
of
> memory, and the page is really long, the tile manager will go ahead and burn
> through all of that. This prevents any of that sort of insanity.
> 
> There are other options here (like, allow 4k pixels of low-res and 2k pixels
of
> high-res), or something like that. Let me know if that's preferred.

This seems pretty reasonable to me for the moment.  We don't even have low res
tiles being generated by default yet (ack).  I think this is something that will
probably need tuning in the future, so am fine with an initial heuristic.

https://codereview.chromium.org/11637022/diff/10001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:200: ManageTiles();
On 2012/12/26 23:55:54, ccameron1 wrote:
> Be sure to verify whether or not this change is acceptable. This makes it so
> that if we say "change your memory policy", then that call immediately has an
> effect.
> 
> After being told "change your memory policy", we report the statistics of the
> new policy to the GPU memory manager -- without doing this change immediately,
> we will just report the old statistics (confusing everyone).

This feels a little heavy handed.  I think I would expect this to
ScheduleManageTiles but not to ManageTiles.    Is there a better place to be
sending stats from in impl-side painting?

[ccameron, 2012-12-27 22:42:59]

Thanks!

Yes, I'll wait for Nat to take a look.

https://codereview.chromium.org/11637022/diff/10001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11637022/diff/10001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:200: ManageTiles();
Yes, this wasn't the right way to do it. I changed it to add a "send stats"
method to the tile manager's proxy, so we can send stats (if sufficiently
changed) automatically after ManageTiles gets executed, and I changed the other
send-stats function to early-out if there exists a tile manager.

[enne (OOO), 2012-12-27 23:02:00]

https://codereview.chromium.org/11637022/diff/10001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11637022/diff/10001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:200: ManageTiles();
On 2012/12/27 22:42:59, ccameron1 wrote:
> Yes, this wasn't the right way to do it. I changed it to add a "send stats"
> method to the tile manager's proxy, so we can send stats (if sufficiently
> changed) automatically after ManageTiles gets executed, and I changed the
other
> send-stats function to early-out if there exists a tile manager.

That sounds a lot better.  :)

[nduca, 2012-12-28 02:16:03]

This looks really good. Some comments about the SendStats call but overall, I'm
happy. Enne, if you see the next iteration on this and think things are good,
feel free to lg.

https://codereview.chromium.org/11637022/diff/15001/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/11637022/diff/15001/cc/thread_proxy.cc#newcod...
cc/thread_proxy.cc:402: if (m_layerTreeHostImpl->tileManager())
Shoud we use settings.implSidePainting? I dont have a strong opinion...

https://codereview.chromium.org/11637022/diff/15001/cc/tile_manager.h
File cc/tile_manager.h (right):

https://codereview.chromium.org/11637022/diff/15001/cc/tile_manager.h#newcode30
cc/tile_manager.h:30: virtual void SendTileManagerMemoryStats(
Lets flip this to GetMemoryStats and just have the caller of ManageTiles check
this. It'll be easier to test.

[ccameron, 2012-12-28 19:55:59]

Thanks!!

https://codereview.chromium.org/11637022/diff/15001/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/11637022/diff/15001/cc/thread_proxy.cc#newcod...
cc/thread_proxy.cc:402: if (m_layerTreeHostImpl->tileManager())
Yes, that's the right signal.

https://codereview.chromium.org/11637022/diff/15001/cc/tile_manager.h
File cc/tile_manager.h (right):

https://codereview.chromium.org/11637022/diff/15001/cc/tile_manager.h#newcode30
cc/tile_manager.h:30: virtual void SendTileManagerMemoryStats(
On 2012/12/28 02:16:03, nduca wrote:
> Lets flip this to GetMemoryStats and just have the caller of ManageTiles check
> this. It'll be easier to test.

Done.

[ccameron, 2013-01-02 18:44:06]

Ping for lg.

[enne (OOO), 2013-01-02 18:49:33]

On 2013/01/02 18:44:06, ccameron1 wrote:
> Ping for lg.

Ack! I'm sorry; I glanced at this earlier and mistakenly thought you hadn't
addressed Nat's changes yet.

lgtm

[ccameron, 2013-01-02 18:50:13]

Thanks!!

[commit-bot: I haz the power, 2013-01-02 18:50:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/11637022/21001

[commit-bot: I haz the power, 2013-01-02 18:50:32]

Failed to apply patch for cc/layer_tree_host_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/layer_tree_host_impl.cc
  Hunk #4 FAILED at 784.
  Hunk #5 succeeded at 813 (offset -1 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
cc/layer_tree_host_impl.cc.rej

Patch:       cc/layer_tree_host_impl.cc
Index: cc/layer_tree_host_impl.cc
diff --git a/cc/layer_tree_host_impl.cc b/cc/layer_tree_host_impl.cc
index
408e8417971f300adfce4cb83a855ddaf80153bc..19da7af7266392e5776d790275f90b7054e5fe38
100644
--- a/cc/layer_tree_host_impl.cc
+++ b/cc/layer_tree_host_impl.cc
@@ -216,9 +216,9 @@ LayerTreeHostImpl::LayerTreeHostImpl(const
LayerTreeSettings& settings, LayerTre
     , m_visible(true)
     , m_contentsTexturesPurged(false)
     ,
m_managedMemoryPolicy(PrioritizedResourceManager::defaultMemoryAllocationLimit(),
-                            PriorityCalculator::allowEverythingCutoff(),
+                            ManagedMemoryPolicy::CUTOFF_ALLOW_EVERYTHING,
                             0,
-                            PriorityCalculator::allowNothingCutoff())
+                            ManagedMemoryPolicy::CUTOFF_ALLOW_NOTHING)
     , m_backgroundColor(0)
     , m_hasTransparentBackground(false)
     , m_needsUpdateDrawProperties(false)
@@ -326,6 +326,16 @@ void LayerTreeHostImpl::manageTiles()
 {
     DCHECK(m_tileManager);
     m_tileManager->ManageTiles();
+
+    size_t memoryRequiredBytes;
+    size_t memoryNiceToHaveBytes;
+    size_t memoryUsedBytes;
+    m_tileManager->GetMemoryStats(&memoryRequiredBytes,
+                                  &memoryNiceToHaveBytes,
+                                  &memoryUsedBytes);
+    sendManagedMemoryStats(memoryRequiredBytes,
+                           memoryNiceToHaveBytes,
+                           memoryUsedBytes);
 }
 
 void LayerTreeHostImpl::startPageScaleAnimation(gfx::Vector2d targetOffset,
bool anchorPoint, float pageScale, base::TimeTicks startTime, base::TimeDelta
duration)
@@ -764,7 +774,8 @@ void LayerTreeHostImpl::enforceManagedMemoryPolicy(const
ManagedMemoryPolicy& po
 {
     bool evictedResources = m_client->reduceContentsTextureMemoryOnImplThread(
         m_visible ? policy.bytesLimitWhenVisible :
policy.bytesLimitWhenNotVisible,
-        m_visible ? policy.priorityCutoffWhenVisible :
policy.priorityCutoffWhenNotVisible);
+        ManagedMemoryPolicy::priorityCutoffToValue(
+            m_visible ? policy.priorityCutoffWhenVisible :
policy.priorityCutoffWhenNotVisible));
     if (evictedResources) {
         setContentsTexturesPurged();
         m_client->setNeedsCommitOnImplThread();
@@ -773,11 +784,11 @@ void LayerTreeHostImpl::enforceManagedMemoryPolicy(const
ManagedMemoryPolicy& po
     m_client->sendManagedMemoryStats();
 
     if (m_tileManager) {
-      // TODO(nduca): Pass something useful into the memory manager.
       LOG(INFO) << "Setting up initial tile manager policy";
       GlobalStateThatImpactsTilePriority
new_state(m_tileManager->GlobalState());
-      new_state.memory_limit_in_bytes =
PrioritizedResourceManager::defaultMemoryAllocationLimit();
-      new_state.memory_limit_policy = ALLOW_ANYTHING;
+      new_state.memory_limit_in_bytes = m_visible ?
policy.bytesLimitWhenVisible : policy.bytesLimitWhenNotVisible;
+      new_state.memory_limit_policy =
ManagedMemoryPolicy::priorityCutoffToTileMemoryLimitPolicy(
+          m_visible ? policy.priorityCutoffWhenVisible :
policy.priorityCutoffWhenNotVisible);
       m_tileManager->SetGlobalState(new_state);
     }
 }
@@ -803,6 +814,14 @@ void
LayerTreeHostImpl::ScheduleCheckForCompletedSetPixels()
       m_client->setNeedsRedrawOnImplThread();
 }
 
+void LayerTreeHostImpl::SendTileManagerMemoryStats(
+    size_t memoryRequiredBytes,
+    size_t memoryNiceToHaveBytes,
+    size_t memoryUsedBytes)
+{
+    sendManagedMemoryStats(memoryRequiredBytes, memoryNiceToHaveBytes,
memoryUsedBytes);
+}
+
 bool LayerTreeHostImpl::shouldClearRootRenderPass() const
 {
     return m_settings.shouldClearRootRenderPass;

[commit-bot: I haz the power, 2013-01-02 19:04:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/11637022/27002

[commit-bot: I haz the power, 2013-01-02 19:25:48]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-02 20:43:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/11637022/23002

[commit-bot: I haz the power, 2013-01-02 22:47:32]

Change committed as 174883