Move the mapped memory limit from the compositor to the GLHelper context

Move the mapped memory limit from the compositor to the GLHelper context

We map memory when doing a readback, but on android we always get
texture copies which don't map memory in the compositor, then do the
actual map/readback in the GLHelper.

R=sievers@chromium.org
BUG=584497
Committed: https://crrev.com/46c85f28176eaf3cbb5fa40ed4327a1b603f8b3a
Cr-Commit-Position: refs/heads/master@{#389240}

[danakj, 2016-04-20 02:18:02]

Is this the right number? Why not 4 * viewport size?

[danakj, 2016-04-20 02:18:09]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-20 02:18:29]

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

[commit-bot: I haz the power, 2016-04-20 03:32:40]

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

[commit-bot: I haz the power, 2016-04-20 03:32:41]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-04-20 22:12:15]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-04-20 22:12:17]

The CQ bit was unchecked by danakj@chromium.org

[no sievers, 2016-04-20 23:41:47]

lgtm with comment

https://codereview.chromium.org/1896223003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/compositor_impl_android.cc (left):

https://codereview.chromium.org/1896223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/compositor_impl_android.cc:548:
limits.mapped_memory_reclaim_limit = 2 * 1024 * 1024;
I'm wondering if we should still leave some limit here. texImage2D() might still
use this in the fast-path and maybe other stuff that I missed.
And using kNoLimit just means that we can potentially get very high spikes in
shared memory usage which can trigger OOM killing.

[danakj, 2016-04-20 23:45:37]

https://codereview.chromium.org/1896223003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/compositor_impl_android.cc (left):

https://codereview.chromium.org/1896223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/compositor_impl_android.cc:548:
limits.mapped_memory_reclaim_limit = 2 * 1024 * 1024;
On 2016/04/20 23:41:47, sievers wrote:
> I'm wondering if we should still leave some limit here. texImage2D() might
still
> use this in the fast-path and maybe other stuff that I missed.
> And using kNoLimit just means that we can potentially get very high spikes in
> shared memory usage which can trigger OOM killing.

Sure what limit would you like?

[no sievers, 2016-04-21 00:16:50]

On 2016/04/20 23:45:37, danakj wrote:
>
https://codereview.chromium.org/1896223003/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/compositor_impl_android.cc (left):
> 
>
https://codereview.chromium.org/1896223003/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/compositor_impl_android.cc:548:
> limits.mapped_memory_reclaim_limit = 2 * 1024 * 1024;
> On 2016/04/20 23:41:47, sievers wrote:
> > I'm wondering if we should still leave some limit here. texImage2D() might
> still
> > use this in the fast-path and maybe other stuff that I missed.
> > And using kNoLimit just means that we can potentially get very high spikes
in
> > shared memory usage which can trigger OOM killing.
> 
> Sure what limit would you like?

How about |full_screen_texture_size_in_bytes|?

And on second thought also for RWHVA: GLHelper should be mostly doing async
readbacks, so this now would use mapped memory rather than the transfer buffer.
Maybe we can just put something similar?

limits.mapped_memory_reclaim_limit = full_screen_texture_size_in_bytes;

I think the '*3' was probably copy-pasted form compositor_impl_android.cc and
further was probably concerned with texture uploads. I don't think we need to
support 3 fullscreen readbacks in flight without waiting.

(Also note that I think we usually scale down to quarter res or so, so
full_screen is still pretty generous.)

[danakj, 2016-04-21 01:01:34]

On 2016/04/21 00:16:50, sievers wrote:
> On 2016/04/20 23:45:37, danakj wrote:
> >
>
https://codereview.chromium.org/1896223003/diff/1/content/browser/renderer_ho...
> > File content/browser/renderer_host/compositor_impl_android.cc (left):
> > 
> >
>
https://codereview.chromium.org/1896223003/diff/1/content/browser/renderer_ho...
> > content/browser/renderer_host/compositor_impl_android.cc:548:
> > limits.mapped_memory_reclaim_limit = 2 * 1024 * 1024;
> > On 2016/04/20 23:41:47, sievers wrote:
> > > I'm wondering if we should still leave some limit here. texImage2D() might
> > still
> > > use this in the fast-path and maybe other stuff that I missed.
> > > And using kNoLimit just means that we can potentially get very high spikes
> in
> > > shared memory usage which can trigger OOM killing.
> > 
> > Sure what limit would you like?
> 
> How about |full_screen_texture_size_in_bytes|?
> 
> And on second thought also for RWHVA: GLHelper should be mostly doing async
> readbacks, so this now would use mapped memory rather than the transfer
buffer.
> Maybe we can just put something similar?
> 
> limits.mapped_memory_reclaim_limit = full_screen_texture_size_in_bytes;

Maybe we should reduce the transfer buffer start/min/max size then also for the
GLHelper context? And the command buffer size is the same as for CompositorImpl
which seems excessive? What about (with questions inline)

For GLHelper..
  // GLHelper is mostly used for readbacks, and shouldn't need space for a lot
of commands in the pipe.
  limits.command_buffer_size = 1024;
  // What is the transfer buffer used for?  <---------
  limits.start_transfer_buffer_size = 1024;  // Or some larger size? But doesn't
need to be as large as the Display or LTH contexts? <------
  limits.min_transfer_buffer_size = 1024;
  limits.max_transfer_buffer_size = ???  <---------- 
  limits.mapped_memory_reclaim_limit = full_screen_texture_size_in_bytes;

For CompositorImpl:
  // This limit is meant to hold the contents of the display compositor         
                                                        
  // drawing the scene. See discussion here:                                    
                                                        
  //
https://codereview.chromium.org/1900993002/diff/90001/content/browser/rendere...
th=80&tab_spaces=8                                                              
                                                        
  limits.command_buffer_size = 64 * 1024;
  // These limits are meant to hold the uploads for the browser UI without      
                                                        
  // any excess space.                                                          
                                                        
  limits.start_transfer_buffer_size = 64 * 1024;
  limits.min_transfer_buffer_size = 64 * 1024;
  limits.max_transfer_buffer_size = full_screen_texture_size_in_bytes;
  limits.mapped_memory_reclaim_limit = full_screen_texture_size_in_bytes;
 
Or do you want to keep the 3* in some of these cases?

> I think the '*3' was probably copy-pasted form compositor_impl_android.cc and
> further was probably concerned with texture uploads. I don't think we need to
> support 3 fullscreen readbacks in flight without waiting.
> 
> (Also note that I think we usually scale down to quarter res or so, so
> full_screen is still pretty generous.)

[no sievers, 2016-04-21 23:18:55]

On 2016/04/21 01:01:34, danakj wrote:
> On 2016/04/21 00:16:50, sievers wrote:
> > On 2016/04/20 23:45:37, danakj wrote:
> > >
> >
>
https://codereview.chromium.org/1896223003/diff/1/content/browser/renderer_ho...
> > > File content/browser/renderer_host/compositor_impl_android.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/1896223003/diff/1/content/browser/renderer_ho...
> > > content/browser/renderer_host/compositor_impl_android.cc:548:
> > > limits.mapped_memory_reclaim_limit = 2 * 1024 * 1024;
> > > On 2016/04/20 23:41:47, sievers wrote:
> > > > I'm wondering if we should still leave some limit here. texImage2D()
might
> > > still
> > > > use this in the fast-path and maybe other stuff that I missed.
> > > > And using kNoLimit just means that we can potentially get very high
spikes
> > in
> > > > shared memory usage which can trigger OOM killing.
> > > 
> > > Sure what limit would you like?
> > 
> > How about |full_screen_texture_size_in_bytes|?
> > 
> > And on second thought also for RWHVA: GLHelper should be mostly doing async
> > readbacks, so this now would use mapped memory rather than the transfer
> buffer.
> > Maybe we can just put something similar?
> > 
> > limits.mapped_memory_reclaim_limit = full_screen_texture_size_in_bytes;
> 
> Maybe we should reduce the transfer buffer start/min/max size then also for
the
> GLHelper context? And the command buffer size is the same as for
CompositorImpl
> which seems excessive? What about (with questions inline)
> 
> For GLHelper..
>   // GLHelper is mostly used for readbacks, and shouldn't need space for a lot
> of commands in the pipe.
>   limits.command_buffer_size = 1024;

Sounds good.

>   // What is the transfer buffer used for?  <---------

In CompositorImpl I think for UIResource uploads.
In RWHVA's helper context... for nothing?

>   limits.start_transfer_buffer_size = 1024;  // Or some larger size? But
doesn't
> need to be as large as the Display or LTH contexts? <------

Assuming it's used for nothing... yea sounds good.

>   limits.min_transfer_buffer_size = 1024;
>   limits.max_transfer_buffer_size = ???  <---------- 

Maybe |full_screen_texture_size_in_bytes| just in case?

>   limits.mapped_memory_reclaim_limit = full_screen_texture_size_in_bytes;
> 
> For CompositorImpl:
>   // This limit is meant to hold the contents of the display compositor       
 
>                                                         
>   // drawing the scene. See discussion here:                                  
 
>                                                         
>   //
>
https://codereview.chromium.org/1900993002/diff/90001/content/browser/rendere...
> th=80&tab_spaces=8                                                            
 
>                                                         
>   limits.command_buffer_size = 64 * 1024;
>   // These limits are meant to hold the uploads for the browser UI without    
 
>                                                         
>   // any excess space.                                                        
 
>                                                         
>   limits.start_transfer_buffer_size = 64 * 1024;
>   limits.min_transfer_buffer_size = 64 * 1024;
>   limits.max_transfer_buffer_size = full_screen_texture_size_in_bytes;
>   limits.mapped_memory_reclaim_limit = full_screen_texture_size_in_bytes;
>  
> Or do you want to keep the 3* in some of these cases?

Nah.

[no sievers, 2016-04-21 23:25:48]

> >   limits.start_transfer_buffer_size = 1024;  // Or some larger size? But
> >   limits.min_transfer_buffer_size = 1024;

piman@ just mentioned shaders going through this, so maybe 4k is better, just in
case.
(Actually, bigger out-of-band arguments to gl functions in the cmdbuffer would
use the transfer buffer in general.)

[danakj, 2016-04-22 00:45:33]

ok PTAL

[no sievers, 2016-04-22 00:52:07]

lgtm thanks!

[danakj, 2016-04-22 00:59:42]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2016-04-22 01:00:08]

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

[commit-bot: I haz the power, 2016-04-22 02:03:40]

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

[commit-bot: I haz the power, 2016-04-22 02:03:41]

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_...)

[danakj, 2016-04-22 20:23:40]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2016-04-22 20:24:10]

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

[commit-bot: I haz the power, 2016-04-22 21:17:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-22 21:18:55]

Description was changed from

==========
Move the mapped memory limit from the compositor to the GLHelper context

We map memory when doing a readback, but on android we always get
texture copies which don't map memory in the compositor, then do the
actual map/readback in the GLHelper.

R=sievers@chromium.org
BUG=584497
==========

to

==========
Move the mapped memory limit from the compositor to the GLHelper context

We map memory when doing a readback, but on android we always get
texture copies which don't map memory in the compositor, then do the
actual map/readback in the GLHelper.

R=sievers@chromium.org
BUG=584497

Committed: https://crrev.com/46c85f28176eaf3cbb5fa40ed4327a1b603f8b3a
Cr-Commit-Position: refs/heads/master@{#389240}
==========

[commit-bot: I haz the power, 2016-04-22 21:18:56]

Patchset 4 (id:??) landed as
https://crrev.com/46c85f28176eaf3cbb5fa40ed4327a1b603f8b3a
Cr-Commit-Position: refs/heads/master@{#389240}