Move gpu memory calculations to Compositor.

Move gpu memory calculations to Compositor.

This moves gpu memory policy code from gpumemorymanager to renderer compositor.
It will be useful for renderer to use off screen context.

BUG=526196
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/127a06b45820e30f78079814557cfacd038d6167
Cr-Commit-Position: refs/heads/master@{#352301}

[sohanjg, 2015-09-08 14:47:22]

sohan.jyoti@samsung.com changed reviewers:
+ danakj@chromium.org

[sohanjg, 2015-09-08 14:47:23]

This is not final patch.
Just wanted to know, if this approach is correct.

I have moved the total gpu mem calc from glcontextandroid to
renderwidgetcompositor.
What should be the plan with the gpumemorymanager?should we try and get hold of
client_hard_limit_bytes_ in renderwidgetcompositor or move that code here and
recalculate the value?

thanks.

[danakj, 2015-09-08 18:10:41]

danakj@chromium.org changed reviewers:
+ ccameron@chromium.org

[danakj, 2015-09-08 18:10:41]

chris can you have a look?

[ccameron, 2015-09-11 18:22:58]

I like the approach. What's the plan for the ui::Compositor, though? Should we
ever impose a limit on its memory (I think not).

https://codereview.chromium.org/1324413003/diff/1/content/renderer/gpu/render...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1324413003/diff/1/content/renderer/gpu/render...
content/renderer/gpu/render_widget_compositor.cc:1055: cc::ManagedMemoryPolicy
actual = policy;
This needs an #if android around it.

[sohanjg, 2015-09-14 14:32:39]

i have moved the GpuMemoryManager::UpdateAvailableGpuMemory as part of
RWC::GetGpuMemoryPolicy too.
i think ui compositor doesn't query or set up available gpu memory.
If this looks all-right i will start deleting SetMemoryPolicy code, and re-write
the tests.
please take a look, thanks.

https://codereview.chromium.org/1324413003/diff/1/content/renderer/gpu/render...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1324413003/diff/1/content/renderer/gpu/render...
content/renderer/gpu/render_widget_compositor.cc:1055: cc::ManagedMemoryPolicy
actual = policy;
On 2015/09/11 18:22:58, ccameron wrote:
> This needs an #if android around it.

Done.

[ccameron, 2015-09-14 17:44:51]

Couple more nits, but this is coming along.

https://codereview.chromium.org/1324413003/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1324413003/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:1057: static size_t limit_bytes
= 0;
This local variable shouldn't be used (also shouldn't be static) -- use
actual.bytes_limit_when_visible directly.

https://codereview.chromium.org/1324413003/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:1120: if
(HasExtension("GL_NVX_gpu_memory_info")) {
Oh -- don't bother with the NVX extension -- just go with 512 MB here.

https://codereview.chromium.org/1324413003/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:1130:
actual.bytes_limit_when_visible = limit_bytes;
Don't forget about |priority_cutoff_when_visible| -- it should be set as it is
set here (EVERYTHING for android and NICE_TO_HAVE for all other platforms).

https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...

https://codereview.chromium.org/1324413003/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/1324413003/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.h:82: cc::ManagedMemoryPolicy
GetGpuMemoryPolicy(cc::ManagedMemoryPolicy& policy);
nit: |policy| should be const reference.

[sohanjg, 2015-09-15 10:37:48]

what should be the plan with SetMemoryAllocation, SetMemoryPolicy ? Should we
remove them as part of this CL too ?
please take a look, thanks.

https://codereview.chromium.org/1324413003/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1324413003/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:1057: static size_t limit_bytes
= 0;
On 2015/09/14 17:44:51, ccameron wrote:
> This local variable shouldn't be used (also shouldn't be static) -- use
> actual.bytes_limit_when_visible directly.

Done.

https://codereview.chromium.org/1324413003/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:1120: if
(HasExtension("GL_NVX_gpu_memory_info")) {
On 2015/09/14 17:44:51, ccameron wrote:
> Oh -- don't bother with the NVX extension -- just go with 512 MB here.

Done.

https://codereview.chromium.org/1324413003/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:1130:
actual.bytes_limit_when_visible = limit_bytes;
On 2015/09/14 17:44:51, ccameron wrote:
> Don't forget about |priority_cutoff_when_visible| -- it should be set as it is
> set here (EVERYTHING for android and NICE_TO_HAVE for all other platforms).
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...

Done.

https://codereview.chromium.org/1324413003/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/1324413003/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.h:82: cc::ManagedMemoryPolicy
GetGpuMemoryPolicy(cc::ManagedMemoryPolicy& policy);
On 2015/09/14 17:44:51, ccameron wrote:
> nit: |policy| should be const reference.

Done.

[piman, 2015-09-18 01:43:27]

piman@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2015-09-18 01:43:27]

https://codereview.chromium.org/1324413003/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (left):

https://codereview.chromium.org/1324413003/diff/40001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:244:
context3d_->GetCommandBufferProxy()->SetMemoryAllocationChangedCallback(
Drive-by, this is the only caller of
CommandBufferProxyImpl::SetMemoryAllocationChangedCallback, so if we remove
this, we can also remove the
CommandBufferProxyImpl::SetMemoryAllocationChangedCallback, the
GpuCommandBufferMsg_SetClientHasMemoryAllocationChangedCallback message,
GpuCommandBufferStub::OnSetClientHasMemoryAllocationChangedCallback,
GpuMemoryManager::CreateClientState etc.

Vice versa, we can remove
CommandBufferProxyImpl::memory_allocation_changed_callback_, which means
CommandBufferProxyImpl::OnSetMemoryAllocation is a noop, which means we can
remove the GpuCommandBufferMsg_SetMemoryAllocation message, and
GpuCommandBufferStub::SetMemoryAllocation etc.


Is there much left in GpuMemoryManager then? The client lists would be always
empty, we could remove them and everything that deals with it. I think we don't
need Manage() any more. I guess the only thing left would be the memory tracking
to send the info to the browser (for UMA and task manager), which could probably
be simplified.


I'm surprised we can do all that though. I thought one of the main aspects of
the GpuMemoryManager was to ensure that Mac wouldn't go over its available GPU
memory (by dividing among clients), because that causes hard system crashes. Is
that not the case any more?

[ccameron, 2015-09-18 18:19:57]

On 2015/09/18 01:43:27, piman (slow to review) wrote:
>
https://codereview.chromium.org/1324413003/diff/40001/content/common/gpu/clie...
> File content/common/gpu/client/context_provider_command_buffer.cc (left):
> 
>
https://codereview.chromium.org/1324413003/diff/40001/content/common/gpu/clie...
> content/common/gpu/client/context_provider_command_buffer.cc:244:
> context3d_->GetCommandBufferProxy()->SetMemoryAllocationChangedCallback(
> Drive-by, this is the only caller of
> CommandBufferProxyImpl::SetMemoryAllocationChangedCallback, so if we remove
> this, we can also remove the
> CommandBufferProxyImpl::SetMemoryAllocationChangedCallback, the
> GpuCommandBufferMsg_SetClientHasMemoryAllocationChangedCallback message,
> GpuCommandBufferStub::OnSetClientHasMemoryAllocationChangedCallback,
> GpuMemoryManager::CreateClientState etc.
> 
> Vice versa, we can remove
> CommandBufferProxyImpl::memory_allocation_changed_callback_, which means
> CommandBufferProxyImpl::OnSetMemoryAllocation is a noop, which means we can
> remove the GpuCommandBufferMsg_SetMemoryAllocation message, and
> GpuCommandBufferStub::SetMemoryAllocation etc.
> 
> 
> Is there much left in GpuMemoryManager then? The client lists would be always
> empty, we could remove them and everything that deals with it. I think we
don't
> need Manage() any more. I guess the only thing left would be the memory
tracking
> to send the info to the browser (for UMA and task manager), which could
probably
> be simplified.

I think it's only useful as a "GPU memory tracker" -- it doesn't manage anything
(and hasn't for a while).

> I'm surprised we can do all that though. I thought one of the main aspects of
> the GpuMemoryManager was to ensure that Mac wouldn't go over its available GPU
> memory (by dividing among clients), because that causes hard system crashes.
Is
> that not the case any more?

The GpuMemoryManager was an example of really bad over-engineering and creating
complicated solutions to problems that didn't exist. I'm very happy to see it
being removed.

Mac would stall for a second or two when it had to do tons of paging, but I
don't remember any system hangs. While bad things can happen when you allocate
too much GPU memory, it is basically impossible to tell when that cliff will be
on any platform.

Among the problems with the design was that the GpuMemoryManager was, in effect,
controlling how much rasterization was done (and how much old rasterization was
cached). Which is really strange -- it would make sense for the compositor to
make this decision more locally (which is where things are now). We would end
up, on a long-scrolling page, using ~1GB of memory to store the rasterization of
the whole page (which would cause stalls and paging, etc). So then we made the
limit smaller, but then we found that we'd be flashing missing tiles on heavier
pages because we didn't have enough memory to draw the content of the page
(which is even worse than stalls/paging).

The right thing to do is to let the compositor use as much memory as it needs to
draw what is on-screen, up to a high-ish fixed limit, and allow it to cache
"nearby" tiles (up to a lower limit) for scrolling.

These limits shouldn't depend on how many clients there are (effectively, how
many windows are open) -- if someone is using too much GPU memory because they
have too many windows, they should close some windows -- it's a pretty easy
feedback loop. And it's always better to have performance degrade as they start
paging than to have content just stop appearing.

[piman, 2015-09-18 21:16:14]

On Fri, Sep 18, 2015 at 11:19 AM, <ccameron@chromium.org> wrote:

> On 2015/09/18 01:43:27, piman (slow to review) wrote:
>
>
>
https://codereview.chromium.org/1324413003/diff/40001/content/common/gpu/clie...
>
>> File content/common/gpu/client/context_provider_command_buffer.cc (left):
>>
>
>
>
>
https://codereview.chromium.org/1324413003/diff/40001/content/common/gpu/clie...
>
>> content/common/gpu/client/context_provider_command_buffer.cc:244:
>> context3d_->GetCommandBufferProxy()->SetMemoryAllocationChangedCallback(
>> Drive-by, this is the only caller of
>> CommandBufferProxyImpl::SetMemoryAllocationChangedCallback, so if we
>> remove
>> this, we can also remove the
>> CommandBufferProxyImpl::SetMemoryAllocationChangedCallback, the
>> GpuCommandBufferMsg_SetClientHasMemoryAllocationChangedCallback message,
>> GpuCommandBufferStub::OnSetClientHasMemoryAllocationChangedCallback,
>> GpuMemoryManager::CreateClientState etc.
>>
>
> Vice versa, we can remove
>> CommandBufferProxyImpl::memory_allocation_changed_callback_, which means
>> CommandBufferProxyImpl::OnSetMemoryAllocation is a noop, which means we
>> can
>> remove the GpuCommandBufferMsg_SetMemoryAllocation message, and
>> GpuCommandBufferStub::SetMemoryAllocation etc.
>>
>
>
> Is there much left in GpuMemoryManager then? The client lists would be
>> always
>> empty, we could remove them and everything that deals with it. I think we
>>
> don't
>
>> need Manage() any more. I guess the only thing left would be the memory
>>
> tracking
>
>> to send the info to the browser (for UMA and task manager), which could
>>
> probably
>
>> be simplified.
>>
>
> I think it's only useful as a "GPU memory tracker" -- it doesn't manage
> anything
> (and hasn't for a while).
>
> I'm surprised we can do all that though. I thought one of the main aspects
>> of
>> the GpuMemoryManager was to ensure that Mac wouldn't go over its
>> available GPU
>> memory (by dividing among clients), because that causes hard system
>> crashes.
>>
> Is
>
>> that not the case any more?
>>
>
> The GpuMemoryManager was an example of really bad over-engineering and
> creating
> complicated solutions to problems that didn't exist. I'm very happy to see
> it
> being removed.
>
> Mac would stall for a second or two when it had to do tons of paging, but I
> don't remember any system hangs. While bad things can happen when you
> allocate
> too much GPU memory, it is basically impossible to tell when that cliff
> will be
> on any platform.
>
> Among the problems with the design was that the GpuMemoryManager was, in
> effect,
> controlling how much rasterization was done (and how much old
> rasterization was
> cached). Which is really strange -- it would make sense for the compositor
> to
> make this decision more locally (which is where things are now). We would
> end
> up, on a long-scrolling page, using ~1GB of memory to store the
> rasterization of
> the whole page (which would cause stalls and paging, etc). So then we made
> the
> limit smaller, but then we found that we'd be flashing missing tiles on
> heavier
> pages because we didn't have enough memory to draw the content of the page
> (which is even worse than stalls/paging).
>
> The right thing to do is to let the compositor use as much memory as it
> needs to
> draw what is on-screen, up to a high-ish fixed limit, and allow it to cache
> "nearby" tiles (up to a lower limit) for scrolling.
>
> These limits shouldn't depend on how many clients there are (effectively,
> how
> many windows are open) -- if someone is using too much GPU memory because
> they
> have too many windows, they should close some windows -- it's a pretty easy
> feedback loop. And it's always better to have performance degrade as they
> start
> paging than to have content just stop appearing.
>

Ok, I think it's definitely worth dropping prerendered tiles before making
the system slow to a crawl, but I agree it doesn't mean the
GpuMemoryManager as it is is the best way to achieve that.

Let's make sure we remove dead code with this CL or at least leave
breadcrumbs and a bug to clean up.


>
> https://codereview.chromium.org/1324413003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-09-23 06:27:04]

Any progress? After https://codereview.chromium.org/1366473002/ and
https://codereview.chromium.org/1365563002/ I have a CL that would make the
renderer use offscreen contexts, but it wouldn't work until this lands.

[sohanjg, 2015-09-23 06:44:44]

On 2015/09/23 06:27:04, piman (slow to review) wrote:
> Any progress? After https://codereview.chromium.org/1366473002/ and
> https://codereview.chromium.org/1365563002/ I have a CL that would make the
> renderer use offscreen contexts, but it wouldn't work until this lands.

piman@, sorry i was OOO. I will update this patch at the earliest.

[sohanjg, 2015-09-23 13:01:57]

Patchset #4 (id:60001) has been deleted

[sohanjg, 2015-09-23 13:23:01]

On 2015/09/23 06:44:44, sohanjg wrote:
> On 2015/09/23 06:27:04, piman (slow to review) wrote:
> > Any progress? After https://codereview.chromium.org/1366473002/ and
> > https://codereview.chromium.org/1365563002/ I have a CL that would make the
> > renderer use offscreen contexts, but it wouldn't work until this lands.
> 
> piman@, sorry i was OOO. I will update this patch at the earliest.

I have removed more related code for mem alloc and callback, GpuMemoryManager is
still there. I plan to take care of it in a follow-up.

I will update the tests, since some were using SetMemoryAllocation.
Should we add new content tests in renderer compositor for this? or somewhere
else is more appropriate?
Please take a look. thanks.

[piman, 2015-09-24 21:34:08]

Some more things to remove. I understand you will clean up the GpuMemoryManager
as a follow up, but I think those below should be cleaned up now (unused fields
or functions in other classes).

https://codereview.chromium.org/1324413003/diff/80001/cc/output/context_provi...
File cc/output/context_provider.h (right):

https://codereview.chromium.org/1324413003/diff/80001/cc/output/context_provi...
cc/output/context_provider.h:106: MemoryPolicyChangedCallback;
nit: remove this, since it shouldn't be used anymore

https://codereview.chromium.org/1324413003/diff/80001/cc/test/test_context_pr...
File cc/test/test_context_provider.h (right):

https://codereview.chromium.org/1324413003/diff/80001/cc/test/test_context_pr...
cc/test/test_context_provider.h:81: MemoryPolicyChangedCallback
memory_policy_changed_callback_;
nit: remove

https://codereview.chromium.org/1324413003/diff/80001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1324413003/diff/80001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:196: void
ContextProviderCommandBuffer::OnMemoryAllocationChanged(
nit: remove

https://codereview.chromium.org/1324413003/diff/80001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.h (right):

https://codereview.chromium.org/1324413003/diff/80001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.h:75:
MemoryPolicyChangedCallback memory_policy_changed_callback_;
nit: remove.

https://codereview.chromium.org/1324413003/diff/80001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://codereview.chromium.org/1324413003/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.h:274: gpu::MemoryAllocation
last_memory_allocation_;
nit: remove those 3 fields.

[sohanjg, 2015-09-28 07:35:50]

Please take a look, I will update some tests, if this is final. thanks.

https://codereview.chromium.org/1324413003/diff/80001/cc/output/context_provi...
File cc/output/context_provider.h (right):

https://codereview.chromium.org/1324413003/diff/80001/cc/output/context_provi...
cc/output/context_provider.h:106: MemoryPolicyChangedCallback;
On 2015/09/24 21:34:08, piman (slow to review) wrote:
> nit: remove this, since it shouldn't be used anymore

Done.

https://codereview.chromium.org/1324413003/diff/80001/cc/test/test_context_pr...
File cc/test/test_context_provider.h (right):

https://codereview.chromium.org/1324413003/diff/80001/cc/test/test_context_pr...
cc/test/test_context_provider.h:81: MemoryPolicyChangedCallback
memory_policy_changed_callback_;
On 2015/09/24 21:34:08, piman (slow to review) wrote:
> nit: remove

Done.

https://codereview.chromium.org/1324413003/diff/80001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1324413003/diff/80001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:196: void
ContextProviderCommandBuffer::OnMemoryAllocationChanged(
On 2015/09/24 21:34:08, piman (slow to review) wrote:
> nit: remove

Done.

https://codereview.chromium.org/1324413003/diff/80001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.h (right):

https://codereview.chromium.org/1324413003/diff/80001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.h:75:
MemoryPolicyChangedCallback memory_policy_changed_callback_;
On 2015/09/24 21:34:08, piman (slow to review) wrote:
> nit: remove.

Done.

https://codereview.chromium.org/1324413003/diff/80001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.h (right):

https://codereview.chromium.org/1324413003/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.h:274: gpu::MemoryAllocation
last_memory_allocation_;
On 2015/09/24 21:34:08, piman (slow to review) wrote:
> nit: remove those 3 fields.

Done.

[piman, 2015-09-28 18:32:38]

LGTM in essence, couple of points.

https://codereview.chromium.org/1324413003/diff/100001/cc/output/output_surfa...
File cc/output/output_surface_unittest.cc (left):

https://codereview.chromium.org/1324413003/diff/100001/cc/output/output_surfa...
cc/output/output_surface_unittest.cc:169:
context_provider->SetMemoryAllocation(policy);
I assume the test will need to be updated to pass, or removed.

https://codereview.chromium.org/1324413003/diff/100001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1324413003/diff/100001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:877: void
GpuCommandBufferStub::OnSetSurfaceVisible(bool visible) {
Sounds like we can remove the IPC too then. maybe add a TODO to follow up? I
think that'll lead to cleanup on the client side too.

[sohanjg, 2015-09-29 14:19:26]

The OutputSurface test wont be valid anymore as we are not passing the gpu mem
policy calc from there.
I am not sure whether we should add any test in RWC for this, as its just
picking up from settings and configs.

Please let me know your opinion.thanks.

https://codereview.chromium.org/1324413003/diff/100001/cc/output/output_surfa...
File cc/output/output_surface_unittest.cc (left):

https://codereview.chromium.org/1324413003/diff/100001/cc/output/output_surfa...
cc/output/output_surface_unittest.cc:169:
context_provider->SetMemoryAllocation(policy);
On 2015/09/28 18:32:38, piman (slow to review) wrote:
> I assume the test will need to be updated to pass, or removed.

Done.

https://codereview.chromium.org/1324413003/diff/100001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1324413003/diff/100001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:877: void
GpuCommandBufferStub::OnSetSurfaceVisible(bool visible) {
On 2015/09/28 18:32:38, piman (slow to review) wrote:
> Sounds like we can remove the IPC too then. maybe add a TODO to follow up? I
> think that'll lead to cleanup on the client side too.

Acknowledged.

[sohanjg, 2015-09-29 14:25:12]

The CQ bit was checked by sohan.jyoti@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-29 14:25:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1324413003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1324413003/120001

[commit-bot: I haz the power, 2015-09-29 14:35:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-29 14:35:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[piman, 2015-09-29 16:11:12]

LGTM. I don't think a RWC test per se would be a good bang-for-buck. Where we
may want a test is at the cc level to make sure the policy we set via settings
is correctly applied to the tile manager etc.

[sohanjg, 2015-09-30 12:34:40]

On 2015/09/29 16:11:12, piman (slow to review) wrote:
> LGTM. I don't think a RWC test per se would be a good bang-for-buck. Where we
> may want a test is at the cc level to make sure the policy we set via settings
> is correctly applied to the tile manager etc.

Seems there were some loose ends for MemoryCallback, removed those.
Also, GpuMemoryManagerTest had to be disabled. I have added a test in tile
manager to check for the memory policy,

Please take a look. thanks.

[sohanjg, 2015-09-30 12:36:17]

The CQ bit was checked by sohan.jyoti@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-30 12:37:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1324413003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1324413003/140001

[commit-bot: I haz the power, 2015-09-30 12:41:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-30 12:41:15]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sohanjg, 2015-09-30 14:52:42]

The CQ bit was checked by sohan.jyoti@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-30 14:53:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1324413003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1324413003/160001

[commit-bot: I haz the power, 2015-09-30 15:26:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-30 15:26:50]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[piman, 2015-09-30 19:04:33]

lgtm

https://codereview.chromium.org/1324413003/diff/160001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_manager_unittest.cc (right):

https://codereview.chromium.org/1324413003/diff/160001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager_unittest.cc:191: // TODO(sohanjg): Clean
up as part of crbug.com/537563.
Just delete all of them instead? They don't provide any value any more, and will
disappear anyway.

[sohanjg, 2015-10-01 05:38:44]

The CQ bit was checked by sohan.jyoti@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-01 05:39:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1324413003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1324413003/180001

[commit-bot: I haz the power, 2015-10-01 07:12:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-01 07:12:48]

Dry run: This issue passed the CQ dry run.

[sohanjg, 2015-10-01 07:14:13]

Removed an extra MemoryPolicyChangedCallback occurrence.

https://codereview.chromium.org/1324413003/diff/160001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_manager_unittest.cc (right):

https://codereview.chromium.org/1324413003/diff/160001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager_unittest.cc:191: // TODO(sohanjg): Clean
up as part of crbug.com/537563.
On 2015/09/30 19:04:33, piman (slow to review) wrote:
> Just delete all of them instead? They don't provide any value any more, and
will
> disappear anyway.

Done.

[sohanjg, 2015-10-01 08:53:31]

The CQ bit was checked by sohan.jyoti@samsung.com

[sohanjg, 2015-10-01 08:53:32]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1324413003/#ps180001
(title: "cleanup unused test,code.")

[commit-bot: I haz the power, 2015-10-01 08:54:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1324413003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1324413003/180001

[sohanjg, 2015-10-01 09:00:33]

The CQ bit was unchecked by sohan.jyoti@samsung.com

[sohanjg, 2015-10-01 09:06:20]

sohan.jyoti@samsung.com changed reviewers:
+ blundell@chromium.org, boliu@chromium.org, dtrainor@chromium.org

[sohanjg, 2015-10-01 09:08:21]

On 2015/10/01 08:54:12, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1324413003/180001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1324413003/180001

Looks like we need owner review for aw/ components/ and blimp/ for the clean-up
code.

boliu@, dtrainor@, blundell@ can you please take a look, this would just remove
the memory callback stubs.
thanks.

[blundell, 2015-10-01 09:11:37]

blundell@chromium.org changed reviewers:
+ sky@chromium.org
- blundell@chromium.org

[blundell, 2015-10-01 09:11:38]

blundell -> sky

Prefer to use individual component owners when possible.

[blundell, 2015-10-01 09:12:07]

On 2015/10/01 09:11:38, blundell wrote:
> blundell -> sky
> 
> Prefer to use individual component owners when possible.

Scott, this is for an OWNERS review for //components/mus.

[boliu, 2015-10-01 16:30:16]

a_w rubberstamp lgtm

[David Trainor- moved to gerrit, 2015-10-02 08:08:17]

blimp/ rubberstamp lgtm.

[boliu, 2015-10-02 17:07:18]

boliu@chromium.org changed reviewers:
+ msw@chromium.org

[boliu, 2015-10-02 17:07:19]

+msw for components/mus

[sky, 2015-10-02 17:08:01]

LGTM

[boliu, 2015-10-02 17:11:14]

boliu@chromium.org changed reviewers:
+ dcheng@chromium.org

[boliu, 2015-10-02 17:11:14]

+dcheng for content/common/gpu/gpu_messages.h

[dcheng, 2015-10-02 18:06:01]

ipc changes seem fine but a few other comments

https://codereview.chromium.org/1324413003/diff/180001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1324413003/diff/180001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:1027: base::StringToSizeT(
It seems like you should at least check the return value.

https://codereview.chromium.org/1324413003/diff/180001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:1080:
actual.bytes_limit_when_visible, static_cast<size_t>(8 * 1024 * 1024));
8U * 1024 * 1024 to force signed -> unsigned promotion should probably be
enough, so you don't need to cast.

https://codereview.chromium.org/1324413003/diff/180001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:1082:
std::min(actual.bytes_limit_when_visible,
Ditto.

[sohanjg, 2015-10-05 06:08:44]

Please take a look, thanks.

https://codereview.chromium.org/1324413003/diff/180001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1324413003/diff/180001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:1027: base::StringToSizeT(
On 2015/10/02 18:06:01, dcheng wrote:
> It seems like you should at least check the return value.

Done.

https://codereview.chromium.org/1324413003/diff/180001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:1080:
actual.bytes_limit_when_visible, static_cast<size_t>(8 * 1024 * 1024));
On 2015/10/02 18:06:01, dcheng wrote:
> 8U * 1024 * 1024 to force signed -> unsigned promotion should probably be
> enough, so you don't need to cast.

Done.

https://codereview.chromium.org/1324413003/diff/180001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:1082:
std::min(actual.bytes_limit_when_visible,
On 2015/10/02 18:06:01, dcheng wrote:
> Ditto.

Done.

[sohanjg, 2015-10-05 06:08:56]

The CQ bit was checked by sohan.jyoti@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-05 06:09:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1324413003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1324413003/200001

[dcheng, 2015-10-05 06:31:28]

https://codereview.chromium.org/1324413003/diff/200001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1324413003/diff/200001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:1032: if
(actual.bytes_limit_when_visible)
I meant if (base::StringToSizeT(...)): the fact that *output is zeroed on error
isn't guaranteed.

[commit-bot: I haz the power, 2015-10-05 06:42:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-05 06:42:22]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)

[sohanjg, 2015-10-05 06:56:45]

i had to revert back to casting the limits, because  android_arm64_dbg_recipe
reports error for it :/

please take a look, thanks.

https://codereview.chromium.org/1324413003/diff/180001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1324413003/diff/180001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:1080:
actual.bytes_limit_when_visible, static_cast<size_t>(8 * 1024 * 1024));
On 2015/10/05 06:08:43, sohanjg wrote:
> On 2015/10/02 18:06:01, dcheng wrote:
> > 8U * 1024 * 1024 to force signed -> unsigned promotion should probably be
> > enough, so you don't need to cast.
> 
> Done.

seems, android_arm64_dbg_recipe still needs the cast :/

[render_widget_compositor.cc:1078:67: error: no matching function for call to
'max(size_t&, unsigned int)'
         std::max(actual.bytes_limit_when_visible, 8U * 1024 * 1024);]

https://codereview.chromium.org/1324413003/diff/180001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:1082:
std::min(actual.bytes_limit_when_visible,
On 2015/10/05 06:08:43, sohanjg wrote:
> On 2015/10/02 18:06:01, dcheng wrote:
> > Ditto.
> 
> Done.

seems, android_arm64_dbg_recipe still needs the cast :/

https://codereview.chromium.org/1324413003/diff/200001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1324413003/diff/200001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:1032: if
(actual.bytes_limit_when_visible)
On 2015/10/05 06:31:28, dcheng wrote:
> I meant if (base::StringToSizeT(...)): the fact that *output is zeroed on
error
> isn't guaranteed.

Acknowledged.

[dcheng, 2015-10-05 06:58:21]

lgtm

[sohanjg, 2015-10-05 07:06:52]

The CQ bit was checked by sohan.jyoti@samsung.com

[sohanjg, 2015-10-05 07:06:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, sky@chromium.org, boliu@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1324413003/#ps220001
(title: "review comment + android_arm64_dbg_recipe bot issue.")

[commit-bot: I haz the power, 2015-10-05 07:07:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1324413003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1324413003/220001

[commit-bot: I haz the power, 2015-10-05 08:23:41]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-10-05 08:24:25]

Patchset 11 (id:??) landed as
https://crrev.com/127a06b45820e30f78079814557cfacd038d6167
Cr-Commit-Position: refs/heads/master@{#352301}

[spang, 2015-10-29 17:21:36]

A revert of this CL (patchset #11 id:220001) has been created in
https://codereview.chromium.org/1412923004/ by spang@chromium.org.

The reason for reverting is: Hangs on Chrome OS when rotating the display.

BUG=546653.