GpuMemoryManager suggests values for renderer Contents Texture Managers' preferred memory limit.

GpuMemoryManager suggests values for renderer Contents Texture Managers' preferred memory limit.


BUG=123382
TEST=Manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134428

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134801

[mmocny, 2012-04-18 20:43:11]

This patch will attempt to send memory allocation values to wk compositor to be
used for contents texture manager preferred memory allocation limits.

Until we have a smarter texture manager which can send memory allocation
requests, all we can do is divide memory equally across visible tabs.

The browser ui stub seems to have a non-0 surface_id, which previously was our
test for "are you a wk compositor surface".  Perhaps the part of this patch that
filters browser ui stubs should be its own patch.  Also, there appear to be two
browser ui stubs with the same surface_id (1) which I cannot explain and perhaps
is a bug? (Antoine, is this expected?)

I must still update unittests as they are failing at the moment, but put this
patch up as is for interested parties to glance at.

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:70: if (surface_id != 0 &&
!handle.is_null())
!is_null() seems insufficient to ignore ui stub.
That is why I added is_ui_stub() but forgot to change it here.

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.h:149: bool is_ui_stub() const {
return handle_.handle; }
This was a quick solution to a problem.  It warrents a discussion, and I'll post
comments soon.

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_manager.cc (right):

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager.cc:23: 512 * 1024 * 1024 -
kResourceSizeMinimumForVisibleTab,
I subtract kResourceSizeMinumumForVisibleTab from desired global upper limit of
512M because when opening a new tab, it will use 64M by default before the
others have a change to release memory.  Alternatively we can start tabs at 0 or
make memory allocations synchronous, but I think none of these choices are
preferable.

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager.cc:137: // Set up allocation values for
possible states for stubs with surfaces.
We divide the total desired global limit by the number of visible tabs, guarding
against division by 0 and having a lower bound.
Once we start sending allocation hints incoming from renderers, this logic will
become more complex+fair.

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager.cc:167: // Set up allocation values for
possible states for stubs without surfaces.
Stubs without surfaces do not receive memory allocations at this point, just a
value of 1 or 0 signaling if they are to hibernate or not.

[piman, 2012-04-18 22:20:14]

I have serious concerns about having the GPU process make UI vs non-UI
decisions, and second guess whether a given context corresponds to the UI or
not.

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.h:149: bool is_ui_stub() const {
return handle_.handle; }
On 2012/04/18 20:43:11, mmocny wrote:
> This was a quick solution to a problem.  It warrents a discussion, and I'll
post
> comments soon.

nak... this is not true on non-aura (FTM even on aura if --ui-use-gpu-process
isn't set).
Besides, the GPU side shouldn't have to know anything about the UI.

[nduca, 2012-04-18 23:16:28]

So how do we deal with the UI compositor's memory?

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.cc (right):

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_channel.cc:131: GpuCommandBufferStub* stub =
it.GetCurrentValue();
Is the idea here that we shouldnt give management callbacks to aura?

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_manager.cc (left):

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager.cc:23: kResourceSizeNonHibernatedTab = 1,
are we still using these?

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager.cc:102: void GpuMemoryManager::Manage() {
im having a really hard time reading these allocation objects... the positional
bool arguments are really hard to read. Can we switch that to an enum instead
that we or together or something?

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_manager.cc (right):

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager.cc:23: 512 * 1024 * 1024 -
kResourceSizeMinimumForVisibleTab,
~shrug~ no strong opinions here. Put a rough explanation in a comment though so
when we see this again later we dont forget why its there.

Your variable name here could be chosen better. MaximumAmountToAllocate or
something.

The whole kResourceSize prefix thing seems to be fighitng you. is that some
convention I'm not aware of? kMaximumToAllocate, kMinimimuAllocationPerTab seem
to be clearish.

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager.cc:171: kResourceSizeHibernatedTab, false,
false);
I'm not loving that this causes us to give "1" to skia etc but megabytes upon
megabytes to compositor.

We should do this right.

Presumably we need to first compute the stubs-with-surfaces that are going toget
front or all buffers. Then from that, we have a total list of stubs that want
memory. Give each one 64.

[piman, 2012-04-18 23:39:37]

Also as a general comment, I hate that we're hard-coding completely
arbitrary memory limits all over the place (some in webkit, some in the GPU
process, etc.), with no good relation to the system.
What if the system has more pixels?
What if the system has more memory?

Antoine

[Nat, 2012-04-18 23:54:07]

Hey Antoine -

What's the context here? The point mmocny's patch is to move the hard coded
values into the Gpumemorymanager in preparation for removing them. A
followup to his patch will be to remove the hard coded insanity in
TextureMemoryManager.cpp


- Nat


On Wed, Apr 18, 2012 at 4:39 PM, Antoine Labour <piman@chromium.org> wrote:

> Also as a general comment, I hate that we're hard-coding completely
> arbitrary memory limits all over the place (some in webkit, some in the GPU
> process, etc.), with no good relation to the system.
> What if the system has more pixels?
> What if the system has more memory?
>
> Antoine
>

[piman, 2012-04-19 00:05:50]

On Wed, Apr 18, 2012 at 4:54 PM, Nat Duca <nduca@google.com> wrote:

> Hey Antoine -
>
> What's the context here? The point mmocny's patch is to move the hard
> coded values into the Gpumemorymanager in preparation for removing them. A
> followup to his patch will be to remove the hard coded insanity in
> TextureMemoryManager.cpp
>

Ok, if the goal is to move the hard-coded values, then great.
But if we're going to add complexity, let's make it right, not completely
hacked up.


>
>
> - Nat
>
>
>
> On Wed, Apr 18, 2012 at 4:39 PM, Antoine Labour <piman@chromium.org>wrote:
>
>> Also as a general comment, I hate that we're hard-coding completely
>> arbitrary memory limits all over the place (some in webkit, some in the GPU
>> process, etc.), with no good relation to the system.
>> What if the system has more pixels?
>> What if the system has more memory?
>>
>> Antoine
>>
>
>

[mmocny, 2012-04-20 18:37:55]

Refactored the allocations and updated the unittests so they pass.

I have yet to write unittests to cover the new functionality (re: bonuses).

I haven't removed the tests for "is_ui" yet, because I would like to find an
elegant way to make sure it doesn't get the bulk of the share of memory.

Perhaps I need to implement memory allocation hints and hard code values coming
from the ui, but I was hoping to have a simpler patch up before we get to that
point.

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.cc (right):

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_channel.cc:131: GpuCommandBufferStub* stub =
it.GetCurrentValue();
Yes, until we get allocation hints I didn't want to give aura a large share of
the artificial limit.
Maybe this is not necessary but at the moment if you have only 1 window open,
aura has 2 stubs and a 3rd for popups etc so the wk renderer gets only 1/4 of
the global limit.
Maybe this is the wrong direction, but I wanted to prioritize wk compositor.
On 2012/04/18 23:16:28, nduca wrote:
> Is the idea here that we shouldnt give management callbacks to aura?

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.h:149: bool is_ui_stub() const {
return handle_.handle; }
Yes, ideally incoming allocation hints are sufficient to prioritize allocations.
 Since we don't have those yet, I was hoping to ignore the UI, but that does
seem to be trouble.
On 2012/04/18 22:20:14, piman wrote:
> On 2012/04/18 20:43:11, mmocny wrote:
> > This was a quick solution to a problem.  It warrents a discussion, and I'll
> post
> > comments soon.
> 
> nak... this is not true on non-aura (FTM even on aura if --ui-use-gpu-process
> isn't set).
> Besides, the GPU side shouldn't have to know anything about the UI.

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_manager.cc (left):

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager.cc:102: void GpuMemoryManager::Manage() {
done.
On 2012/04/18 23:16:28, nduca wrote:
> im having a really hard time reading these allocation objects... the
positional
> bool arguments are really hard to read. Can we switch that to an enum instead
> that we or together or something?

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_manager.cc (right):

https://chromiumcodereview.appspot.com/10083056/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager.cc:171: kResourceSizeHibernatedTab, false,
false);
Did a big re-factoring here to make it all clearer.
skia etc will now the get Minimum allocation instead of 1byte, while wk
compositor will get Minimum+bonus, where bonus is equal division of leftover
allocation after giving relevant stubs the minimum.
On 2012/04/18 23:16:28, nduca wrote:
> I'm not loving that this causes us to give "1" to skia etc but megabytes upon
> megabytes to compositor.
> 
> We should do this right.
> 
> Presumably we need to first compute the stubs-with-surfaces that are going
toget
> front or all buffers. Then from that, we have a total list of stubs that want
> memory. Give each one 64.

[mmocny, 2012-04-20 18:47:30]

The unittest expectations changed since allocations are no longer fixed values. 
The actual gpu resource size in bytes will be [0, 1, minimum, >minimum], but the
exact (>min) value for any stub is not guaranteed to be any specific value.

I will update the tests to make this more clear.

https://chromiumcodereview.appspot.com/10083056/diff/5001/content/common/gpu/...
File content/common/gpu/gpu_memory_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10083056/diff/5001/content/common/gpu/...
content/common/gpu/gpu_memory_manager_unittest.cc:201:
EXPECT_GT(stub1.allocation_.gpu_resource_size_in_bytes, 1ul);
I am already working on making it clearer why and when we expect allocation
values to be GT/EQ 0 etc.  Sorry to have hard coded values for now I just wanted
to make sure Manage function is functional after refactoring.

[nduca, 2012-04-20 19:51:15]

This is looking good.

What about this --- when the renderer_gl gets a callback registered on it, we
ipc to the gpu memory manager saying "this context is asking to be managed."

Store on the stub whether its being asked to be managed.

Then, just ignore "unmanaged" contexts during the memory manager calculation?

[nduca, 2012-04-20 19:55:11]

(then, we set up the UI compositor to use a ccsetting about "use gpu memory
manager" or something...)

~shrug~

Let me know what you think.

[mmocny, 2012-04-20 20:03:08]

That sounds like a good solution.

A couple questions:
- We would need to add this request in wk compositor, skia, webgl etc. .  WebGL
doesn't have a memory allocation callback handler yet, I'll have to find which
context its using.
- your note on using ccsetting is the means to differentiate the browser
compositor, right?

On 2012/04/20 19:55:11, nduca wrote:
> (then, we set up the UI compositor to use a ccsetting about "use gpu memory
> manager" or something...)
> 
> ~shrug~
> 
> Let me know what you think.

[nduca, 2012-04-20 20:18:26]

On 2012/04/20 20:03:08, mmocny wrote:
> That sounds like a good solution.
> 
> A couple questions:
> - We would need to add this request in wk compositor, skia, webgl etc. . 
WebGL
> doesn't have a memory allocation callback handler yet, I'll have to find which
> context its using.

I'm confused, what request? I'm saying,
CommandBufferProxy::setMemoryAllocationCallback should send an ipc to the stub
saying "i have an allocation callback." Use the presence of the callback in the
managment.

I assume the other 2 aura contexts that aren't the compositor context aren't
actually setting callbacks, right?

> - your note on using ccsetting is the means to differentiate the browser
> compositor, right?
You might not even need this once you do the above.  Then for your 1 window case
you'd have aura's compositor and the web compositor == 2 == correct.

[mmocny, 2012-04-20 20:25:11]

Okay, yeah that sounds sweet.  Doing it!

On 2012/04/20 20:18:26, nduca wrote:
> On 2012/04/20 20:03:08, mmocny wrote:
> > That sounds like a good solution.
> > 
> > A couple questions:
> > - We would need to add this request in wk compositor, skia, webgl etc. . 
> WebGL
> > doesn't have a memory allocation callback handler yet, I'll have to find
which
> > context its using.
> 
> I'm confused, what request? I'm saying,
> CommandBufferProxy::setMemoryAllocationCallback should send an ipc to the stub
> saying "i have an allocation callback." Use the presence of the callback in
the
> managment.
> 
> I assume the other 2 aura contexts that aren't the compositor context aren't
> actually setting callbacks, right?
> 
> > - your note on using ccsetting is the means to differentiate the browser
> > compositor, right?
> You might not even need this once you do the above.  Then for your 1 window
case
> you'd have aura's compositor and the web compositor == 2 == correct.

[mmocny, 2012-04-20 21:05:41]

Done, and much more elegant now, but I am still seeing more handlers registered
than I expect.

Investigating further.

[mmocny, 2012-04-23 21:34:19]

Okay, the issue with using two ui graphics contexts is a known one
(http://code.google.com/p/chromium/issues/detail?id=124641&thanks=124641&ts=13...)
but using Nat's suggestion is not a problem.

I was still seeing too many callbacks registered because we adapt a WGC3D
context callback into a GC3D callback even when it was null, so the
gpu_command_buffer_proxy always had a callback, even when its handler did
nothing.  Fixed that and things seem to be working quite well.

I will update unittests to test bonus allocations+remove 0ul/1ul constants, but
curious if this patch is looking okay?

[nduca, 2012-04-23 22:12:18]

I'm happy with this approach.

http://codereview.chromium.org/10083056/diff/15001/content/common/gpu/client/...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

http://codereview.chromium.org/10083056/diff/15001/content/common/gpu/client/...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:679: 
what if they clear clear it? You need to remove the callback on the
command_buffer, too

http://codereview.chromium.org/10083056/diff/15001/content/common/gpu/gpu_com...
File content/common/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/10083056/diff/15001/content/common/gpu/gpu_com...
content/common/gpu/gpu_command_buffer_stub.h:60: virtual bool
handles_memory_allocations() const = 0;
bikeshed man wonders if "client responds to" or "client listens to" would be
more effective here. Or client_has_memory_allocation_changed_callback... then
its super precise.

http://codereview.chromium.org/10083056/diff/15001/content/common/gpu/gpu_mes...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/10083056/diff/15001/content/common/gpu/gpu_mes...
content/common/gpu/gpu_messages.h:449:
IPC_MESSAGE_ROUTED0(GpuCommandBufferMsg_HandlesMemoryAllocations)
Not loving this. Also, how do you set it back to zero? See my overall notes
about making this a longer name that is closely tied to the callback.

Perhaps SetClientHasMemoryAllocationChangedCallback(bool)

[mmocny, 2012-04-26 19:38:01]

Antoine,
I am waiting on the exact numbers to use for limits on android (changing the two
enum values), but other than that I hope this patch is ready.
May I get another review, please?

On 2012/04/23 22:12:18, nduca wrote:
> I'm happy with this approach.
> 
>
http://codereview.chromium.org/10083056/diff/15001/content/common/gpu/client/...
> File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
> (right):
> 
>
http://codereview.chromium.org/10083056/diff/15001/content/common/gpu/client/...
> content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:679: 
> what if they clear clear it? You need to remove the callback on the
> command_buffer, too
> 
>
http://codereview.chromium.org/10083056/diff/15001/content/common/gpu/gpu_com...
> File content/common/gpu/gpu_command_buffer_stub.h (right):
> 
>
http://codereview.chromium.org/10083056/diff/15001/content/common/gpu/gpu_com...
> content/common/gpu/gpu_command_buffer_stub.h:60: virtual bool
> handles_memory_allocations() const = 0;
> bikeshed man wonders if "client responds to" or "client listens to" would be
> more effective here. Or client_has_memory_allocation_changed_callback... then
> its super precise.
> 
>
http://codereview.chromium.org/10083056/diff/15001/content/common/gpu/gpu_mes...
> File content/common/gpu/gpu_messages.h (right):
> 
>
http://codereview.chromium.org/10083056/diff/15001/content/common/gpu/gpu_mes...
> content/common/gpu/gpu_messages.h:449:
> IPC_MESSAGE_ROUTED0(GpuCommandBufferMsg_HandlesMemoryAllocations)
> Not loving this. Also, how do you set it back to zero? See my overall notes
> about making this a longer name that is closely tied to the callback.
> 
> Perhaps SetClientHasMemoryAllocationChangedCallback(bool)

[piman, 2012-04-26 20:15:04]

lgtm

[nduca, 2012-04-26 20:35:12]

lgtm

[commit-bot: I haz the power, 2012-04-27 15:45:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10083056/30001

[commit-bot: I haz the power, 2012-04-28 00:18:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10083056/33003

[commit-bot: I haz the power, 2012-04-28 00:44:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10083056/33003

[commit-bot: I haz the power, 2012-04-28 02:23:48]

Change committed as 134428

[mmocny, 2012-05-01 20:24:40]

On 2012/04/28 02:23:48, I haz the power (commit-bot) wrote:
> Change committed as 134428

This change was reverted.
I've identified the problem (I was assuming command buffer was initialized, and
it was failing to init on mac10.5 which luckily caught the error).  Fix is
trivial, so committing again.

[commit-bot: I haz the power, 2012-05-01 20:25:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10083056/42003

[commit-bot: I haz the power, 2012-05-01 22:21:19]

Change committed as 134801