Enforce a memory limit on MappedMemoryManager

Enforce a memory limit on MappedMemoryManager

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

[no sievers, 2013-08-14 02:05:51]

https://codereview.chromium.org/23130004/diff/2001/gpu/command_buffer/client/...
File gpu/command_buffer/client/mapped_memory.cc (right):

https://codereview.chromium.org/23130004/diff/2001/gpu/command_buffer/client/...
gpu/command_buffer/client/mapped_memory.cc:30: #if defined(OS_ANDROID)
can we avoid ifdefs? Can't we just pass in the limit like we do it for the
transfer buffer?

can you add a unit test?

[kaanb, 2013-08-15 00:39:26]

On 2013/08/14 02:05:51, sievers wrote:
>
https://codereview.chromium.org/23130004/diff/2001/gpu/command_buffer/client/...
> File gpu/command_buffer/client/mapped_memory.cc (right):
> 
>
https://codereview.chromium.org/23130004/diff/2001/gpu/command_buffer/client/...
> gpu/command_buffer/client/mapped_memory.cc:30: #if defined(OS_ANDROID)
> can we avoid ifdefs? Can't we just pass in the limit like we do it for the
> transfer buffer?
> 
> can you add a unit test?

Done, PTAL

[no sievers, 2013-08-15 01:07:15]

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:156: #if defined(OS_ANDROID)
there is an ifdef android in a different place now.

can''t this limit be in the same places where we deal with the other limits?

[kaanb, 2013-08-15 01:21:04]

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:156: #if defined(OS_ANDROID)
On 2013/08/15 01:07:15, sievers wrote:
> there is an ifdef android in a different place now.
> 
> can''t this limit be in the same places where we deal with the other limits?

If we need to base this on the screen size of the device we need an ifdef.

Another possibility is to maybe create constants in mapped_memory.h, I don't
really want to put them in WGC3D headers as it breaks encapsulation.

[no sievers, 2013-08-15 01:22:47]

On 2013/08/15 01:21:04, kaanb wrote:
>
https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
> File gpu/command_buffer/client/gles2_implementation.cc (right):
> 
>
https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
> gpu/command_buffer/client/gles2_implementation.cc:156: #if defined(OS_ANDROID)
> On 2013/08/15 01:07:15, sievers wrote:
> > there is an ifdef android in a different place now.
> > 
> > can''t this limit be in the same places where we deal with the other limits?
> 
> If we need to base this on the screen size of the device we need an ifdef.
> 
> Another possibility is to maybe create constants in mapped_memory.h, I don't
> really want to put them in WGC3D headers as it breaks encapsulation.

What's different with this from kDefaultStartTransferBufferSize, etc. ?

[kaanb, 2013-08-15 01:30:13]

On 2013/08/15 01:22:47, sievers wrote:
> On 2013/08/15 01:21:04, kaanb wrote:
> >
>
https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
> > File gpu/command_buffer/client/gles2_implementation.cc (right):
> > 
> >
>
https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
> > gpu/command_buffer/client/gles2_implementation.cc:156: #if
defined(OS_ANDROID)
> > On 2013/08/15 01:07:15, sievers wrote:
> > > there is an ifdef android in a different place now.
> > > 
> > > can''t this limit be in the same places where we deal with the other
limits?
> > 
> > If we need to base this on the screen size of the device we need an ifdef.
> > 
> > Another possibility is to maybe create constants in mapped_memory.h, I don't
> > really want to put them in WGC3D headers as it breaks encapsulation.
> 
> What's different with this from kDefaultStartTransferBufferSize, etc. ?

It controls the behavior of a different class. If both piman and you insist that
I move it over there I could do it. The bigger issue is if we want a hardcoded
constant or something based on device screen size. If the latter it needs to be
in an Android only file or needs to be protected by ifdef.

[no sievers, 2013-08-15 01:32:27]

On 2013/08/15 01:30:13, kaanb wrote:
> On 2013/08/15 01:22:47, sievers wrote:
> > On 2013/08/15 01:21:04, kaanb wrote:
> > >
> >
>
https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
> > > File gpu/command_buffer/client/gles2_implementation.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
> > > gpu/command_buffer/client/gles2_implementation.cc:156: #if
> defined(OS_ANDROID)
> > > On 2013/08/15 01:07:15, sievers wrote:
> > > > there is an ifdef android in a different place now.
> > > > 
> > > > can''t this limit be in the same places where we deal with the other
> limits?
> > > 
> > > If we need to base this on the screen size of the device we need an ifdef.
> > > 
> > > Another possibility is to maybe create constants in mapped_memory.h, I
don't
> > > really want to put them in WGC3D headers as it breaks encapsulation.
> > 
> > What's different with this from kDefaultStartTransferBufferSize, etc. ?
> 
> It controls the behavior of a different class. If both piman and you insist
that
> I move it over there I could do it. The bigger issue is if we want a hardcoded
> constant or something based on device screen size. If the latter it needs to
be
> in an Android only file or needs to be protected by ifdef.

We override transfer buffer sizes already in compositor_impl_android.cc and
image_transport_factory_android.cc based on resolution.

And we pass those values into WGC3D.

What's different about this? Why not all in one place?

[no sievers, 2013-08-15 01:43:42]

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory_unittest.cc (right):

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory_unittest.cc:337: ASSERT_TRUE(mem3);
I think this test shows that your patch does not necessarily work as expected.
I assume it's allocating again here instead of reusing.

What you are missing is the case where the mapped memory is IN_USE.

I'd keep this test to specify what the behavior should be for the corresponding
calling sequence (multiple Map calls without Unmap).

Maybe add another that mimicks the Map/Unmap, Map/Unmap, .. pattern. You would
explicitly have to insert the token and eventually signal it, see
FreePendingToken test above.

[no sievers, 2013-08-15 01:49:06]

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory_unittest.cc (right):

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory_unittest.cc:337: ASSERT_TRUE(mem3);
On 2013/08/15 01:43:42, sievers wrote:
> I think this test shows that your patch does not necessarily work as expected.
> I assume it's allocating again here instead of reusing.
> 
> What you are missing is the case where the mapped memory is IN_USE.
> 
> I'd keep this test to specify what the behavior should be for the
corresponding
> calling sequence (multiple Map calls without Unmap).
> 
> Maybe add another that mimicks the Map/Unmap, Map/Unmap, .. pattern. You would
> explicitly have to insert the token and eventually signal it, see
> FreePendingToken test above.

Btw, which usage case are you trying to optimize again?
async uploads or not?

[piman, 2013-08-15 02:49:03]

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:156: #if defined(OS_ANDROID)
On 2013/08/15 01:21:05, kaanb wrote:
> On 2013/08/15 01:07:15, sievers wrote:
> > there is an ifdef android in a different place now.
> > 
> > can''t this limit be in the same places where we deal with the other limits?
> 
> If we need to base this on the screen size of the device we need an ifdef.
> 
> Another possibility is to maybe create constants in mapped_memory.h, I don't
> really want to put them in WGC3D headers as it breaks encapsulation.

Agreed with Daniel, please inject this value instead of hard-coding it inside of
GLES2Implementation.
What if you want a different value between renderer and browser? Or even between
different contexts in the same process? Does DeviceDisplayInfo even work in the
renderer?
What if other platforms want to tweak this value?
Etc.

This breaks encapsulation way worse than injecting it.

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.cc (right):

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:75: }
So, if we get here, that means that we're not going to respect the limit anyway.
We should probably log something (and clarify that the limit is not strict).

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.h (right):

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.h:110: explicit
MappedMemoryManager(CommandBufferHelper* helper,
No explicit. Do we really need 2 constructors? Can we fix callers to always pass
memory_limit?

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory_unittest.cc (right):

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory_unittest.cc:333: // the memory limit
we'll wait tokens on existing chunks.
Wait for what token? You didn't free anything pending token.

[kaanb, 2013-08-16 22:50:43]

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:156: #if defined(OS_ANDROID)
On 2013/08/15 02:49:03, piman wrote:
> On 2013/08/15 01:21:05, kaanb wrote:
> > On 2013/08/15 01:07:15, sievers wrote:
> > > there is an ifdef android in a different place now.
> > > 
> > > can''t this limit be in the same places where we deal with the other
limits?
> > 
> > If we need to base this on the screen size of the device we need an ifdef.
> > 
> > Another possibility is to maybe create constants in mapped_memory.h, I don't
> > really want to put them in WGC3D headers as it breaks encapsulation.
> 
> Agreed with Daniel, please inject this value instead of hard-coding it inside
of
> GLES2Implementation.
> What if you want a different value between renderer and browser? Or even
between
> different contexts in the same process? Does DeviceDisplayInfo even work in
the
> renderer?
> What if other platforms want to tweak this value?
> Etc.
> 
> This breaks encapsulation way worse than injecting it.

Done.

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.cc (right):

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:75: }
On 2013/08/15 02:49:03, piman wrote:
> So, if we get here, that means that we're not going to respect the limit
anyway.
> We should probably log something (and clarify that the limit is not strict).

Done.

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.h (right):

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.h:110: explicit
MappedMemoryManager(CommandBufferHelper* helper,
On 2013/08/15 02:49:03, piman wrote:
> No explicit. Do we really need 2 constructors? Can we fix callers to always
pass
> memory_limit?

Removed the explicit. I'd prefer to keep this patch easily mergeable to M30, we
can cleanup the constructors later on. All of them are in unittests except the
one in GLES2Implementation which is already getting modified.

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory_unittest.cc (right):

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory_unittest.cc:333: // the memory limit
we'll wait tokens on existing chunks.
On 2013/08/15 02:49:03, piman wrote:
> Wait for what token? You didn't free anything pending token.

Done.

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory_unittest.cc:337: ASSERT_TRUE(mem3);
On 2013/08/15 01:43:42, sievers wrote:
> I think this test shows that your patch does not necessarily work as expected.
> I assume it's allocating again here instead of reusing.
> 
> What you are missing is the case where the mapped memory is IN_USE.
> 
> I'd keep this test to specify what the behavior should be for the
corresponding
> calling sequence (multiple Map calls without Unmap).
> 
> Maybe add another that mimicks the Map/Unmap, Map/Unmap, .. pattern. You would
> explicitly have to insert the token and eventually signal it, see
> FreePendingToken test above.

Done.

https://codereview.chromium.org/23130004/diff/11001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory_unittest.cc:337: ASSERT_TRUE(mem3);
On 2013/08/15 01:49:06, sievers wrote:
> On 2013/08/15 01:43:42, sievers wrote:
> > I think this test shows that your patch does not necessarily work as
expected.
> > I assume it's allocating again here instead of reusing.
> > 
> > What you are missing is the case where the mapped memory is IN_USE.
> > 
> > I'd keep this test to specify what the behavior should be for the
> corresponding
> > calling sequence (multiple Map calls without Unmap).
> > 
> > Maybe add another that mimicks the Map/Unmap, Map/Unmap, .. pattern. You
would
> > explicitly have to insert the token and eventually signal it, see
> > FreePendingToken test above.
> 
> Btw, which usage case are you trying to optimize again?
> async uploads or not?

I think this optimization is oblivious to whether we use async uploads or not,
right?

[piman, 2013-08-19 21:44:21]

LGTM + one thing.

https://codereview.chromium.org/23130004/diff/27001/content/common/gpu/client...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/23130004/diff/27001/content/common/gpu/client...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:303:
mapped_memory_limit_ = mapped_memory_limit;
This is set, but never used anywhere. We should either use it when we initialize
the GLES2Implementation below, or remove it from the class.

[kaanb, 2013-08-19 21:56:15]

https://codereview.chromium.org/23130004/diff/27001/content/common/gpu/client...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/23130004/diff/27001/content/common/gpu/client...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:303:
mapped_memory_limit_ = mapped_memory_limit;
On 2013/08/19 21:44:21, piman wrote:
> This is set, but never used anywhere. We should either use it when we
initialize
> the GLES2Implementation below, or remove it from the class.

Done.

[no sievers, 2013-08-19 21:57:44]

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (left):

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:152:
SetSharedMemoryChunkSizeMultiple(1024 * 1024 * 2);
Why are you removing this?

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:147: unsigned int
mapped_memory_limit) {
you can just add it to Initialize, it already has the other limits

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.cc (right):

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:28:
MappedMemoryManager::MappedMemoryManager(CommandBufferHelper* helper,
Let's not add a new constructor for that. It just causes surface for bugs.

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:64: if (memory_limit_ != kNoLimit &&
allocated_memory_ >= memory_limit_) {
What do you all think about looking at the free+free_pending_token memory here?

That way we can make this a well defined hard limit, for example
max_free_or_pending_memory_ instead of something very vague that you wouldn't
really know what to set it to (you would have to make it dependent on another
value in cc).

Together with the client's limit (i.e. cc's limit for pending raster jobs), this
would then allow us to set a hard limit for all shared memory used for texture
uploads in this path.

You could then probably also safely set it to some high limit for all platforms
and wouldn't have to deal with kNoLimit here.

[epenner, 2013-08-19 22:05:52]

Quick drive-by, and one extra meta comment: This limit is probably good to have,
but I think even better would be if we could explicitly specify to not wait for
the token when we don't need to. Then we could use 2X-3X less memory to begin
with and likely never hit this limit.

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.cc (right):

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:63: // is above the limit, try
existing chunks again with waiting.
This should be rare right? Should we add a trace around this potentially-slow
path?

[kaanb, 2013-08-20 01:23:43]

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (left):

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:152:
SetSharedMemoryChunkSizeMultiple(1024 * 1024 * 2);
On 2013/08/19 21:57:44, sievers wrote:
> Why are you removing this?

Done.

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:147: unsigned int
mapped_memory_limit) {
On 2013/08/19 21:57:44, sievers wrote:
> you can just add it to Initialize, it already has the other limits

Done.

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.cc (right):

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:28:
MappedMemoryManager::MappedMemoryManager(CommandBufferHelper* helper,
On 2013/08/19 21:57:44, sievers wrote:
> Let's not add a new constructor for that. It just causes surface for bugs.

Done.

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:63: // is above the limit, try
existing chunks again with waiting.
On 2013/08/19 22:05:52, epenner wrote:
> This should be rare right? Should we add a trace around this potentially-slow
> path?

See line 76

https://codereview.chromium.org/23130004/diff/27001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:64: if (memory_limit_ != kNoLimit &&
allocated_memory_ >= memory_limit_) {
On 2013/08/19 21:57:44, sievers wrote:
> What do you all think about looking at the free+free_pending_token memory
here?
> 
> That way we can make this a well defined hard limit, for example
> max_free_or_pending_memory_ instead of something very vague that you wouldn't
> really know what to set it to (you would have to make it dependent on another
> value in cc).
> 
> Together with the client's limit (i.e. cc's limit for pending raster jobs),
this
> would then allow us to set a hard limit for all shared memory used for texture
> uploads in this path.
> 
> You could then probably also safely set it to some high limit for all
platforms
> and wouldn't have to deal with kNoLimit here.

Done.

[kaanb, 2013-08-20 01:28:54]

+raymes for ppapi/ changes

PTAL

[no sievers, 2013-08-20 02:46:54]

lgtm with a few nits an suggestions. thanks!

I think I like it a bit better having the decision to wait only be a function of
the unused memory. Seems more straightforward to look at it this way with the
client potentially pipelined and always having some memory in use.

https://codereview.chromium.org/23130004/diff/46001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/23130004/diff/46001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:394: 64 * 1024   //
mapped memory limit
Can you put this in a separate change?
We have to think about the value.

https://codereview.chromium.org/23130004/diff/46001/content/browser/renderer_...
File content/browser/renderer_host/image_transport_factory_android.cc (right):

https://codereview.chromium.org/23130004/diff/46001/content/browser/renderer_...
content/browser/renderer_host/image_transport_factory_android.cc:132: 64 * 1024 
 // mapped memory limit
Also, separate change.

https://codereview.chromium.org/23130004/diff/46001/content/common/gpu/client...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/23130004/diff/46001/content/common/gpu/client...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:94: size_t
mapped_memory_limit);
nit: mapped_memory_reclaim_limit

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/fenced_allocator.cc (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/fenced_allocator.cc:8: 
nit: whitespace change

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/fenced_allocator.cc:166: return blocks_.size() != 1 ||
blocks_[0].state != FREE;
Hmm, this function is misleading, right? If you have one free and one
free_pending_token block, it would return true here?

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.cc (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:64: chunk->FreeUnused();
Should we skip the call to FreeUnused() here since we already did it above?

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:73: LOG(INFO) << "Exceeding memory
limit: " << max_free_bytes_;
Hmm that might be spammy. How about a trace event in
FencedAllocator::WaitForTokenAndFreeBlock() instead?

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.h (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.h:118: size_t memory_limit);
nit: 'unused_memory_reclaim_limit'?

And maybe a comment: '// |unused_memory_reclaim_limit|: When exceeded this
causes pending memory to be reclaimed before allocating more memory.'

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory_unittest.cc (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory_unittest.cc:310:
TEST_F(MappedMemoryManagerTest, MemoryLimit) {
nit: (MappedMemoryManagerTest, UnusedMemoryLimit)

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory_unittest.cc:343: // the memory limit was
set to be one chunk.
nit: maybe just alloc 1 chunk and then another chunk in this test to make it
easier to read.
maybe say something like: 'Expect two chunks to be allocated, exceeding the
limit, since all memory is in use'.

[piman, 2013-08-20 02:56:16]

2 nits left for me

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/fenced_allocator.cc (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/fenced_allocator.cc:97: if (blocks_[index].state ==
IN_USE)
nit: only do map lookup once (like in FreePendingToken)

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/fenced_allocator.cc:229: bytes_in_use_ += size;
nit: factor this to before the if?

[raymes, 2013-08-20 03:01:41]

lgtm

On Tue, Aug 20, 2013 at 11:28 AM,  <kaanb@chromium.org> wrote:
> +raymes for ppapi/ changes
>
> PTAL
>
> https://codereview.chromium.org/23130004/

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

[no sievers, 2013-08-20 15:59:17]

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.cc (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:41: GPU_DCHECK(shm_offset);
Can you put 'if (size <= allocated_memory_)' around line 42-75 now?

[epenner, 2013-08-20 18:02:03]

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.cc (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:61: (allocated_memory_ -
total_bytes_in_use) >= max_free_bytes_) {
Please add a trace in this if statement.

[kaanb, 2013-08-20 22:15:46]

https://codereview.chromium.org/23130004/diff/46001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/23130004/diff/46001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:394: 64 * 1024   //
mapped memory limit
On 2013/08/20 02:46:54, sievers wrote:
> Can you put this in a separate change?
> We have to think about the value.

Done.

https://codereview.chromium.org/23130004/diff/46001/content/browser/renderer_...
File content/browser/renderer_host/image_transport_factory_android.cc (right):

https://codereview.chromium.org/23130004/diff/46001/content/browser/renderer_...
content/browser/renderer_host/image_transport_factory_android.cc:132: 64 * 1024 
 // mapped memory limit
On 2013/08/20 02:46:54, sievers wrote:
> Also, separate change.

Done.

https://codereview.chromium.org/23130004/diff/46001/content/common/gpu/client...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/23130004/diff/46001/content/common/gpu/client...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:94: size_t
mapped_memory_limit);
On 2013/08/20 02:46:54, sievers wrote:
> nit: mapped_memory_reclaim_limit

Done.

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/fenced_allocator.cc (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/fenced_allocator.cc:8: 
On 2013/08/20 02:46:54, sievers wrote:
> nit: whitespace change

Whitespace is advised per the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/fenced_allocator.cc:97: if (blocks_[index].state ==
IN_USE)
On 2013/08/20 02:56:16, piman wrote:
> nit: only do map lookup once (like in FreePendingToken)

Done.

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/fenced_allocator.cc:166: return blocks_.size() != 1 ||
blocks_[0].state != FREE;
On 2013/08/20 02:46:54, sievers wrote:
> Hmm, this function is misleading, right? If you have one free and one
> free_pending_token block, it would return true here?

Done.

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/fenced_allocator.cc:229: bytes_in_use_ += size;
On 2013/08/20 02:56:16, piman wrote:
> nit: factor this to before the if?

Done.

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.cc (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:41: GPU_DCHECK(shm_offset);
On 2013/08/20 15:59:17, sievers wrote:
> Can you put 'if (size <= allocated_memory_)' around line 42-75 now?

Done.

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:61: (allocated_memory_ -
total_bytes_in_use) >= max_free_bytes_) {
On 2013/08/20 18:02:03, epenner wrote:
> Please add a trace in this if statement.

Done.

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:64: chunk->FreeUnused();
On 2013/08/20 02:46:54, sievers wrote:
> Should we skip the call to FreeUnused() here since we already did it above?

Done.

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.cc:73: LOG(INFO) << "Exceeding memory
limit: " << max_free_bytes_;
On 2013/08/20 02:46:54, sievers wrote:
> Hmm that might be spammy. How about a trace event in
> FencedAllocator::WaitForTokenAndFreeBlock() instead?

I put the trace event in this method.

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.h (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.h:118: size_t memory_limit);
On 2013/08/20 02:46:54, sievers wrote:
> nit: 'unused_memory_reclaim_limit'?
> 
> And maybe a comment: '// |unused_memory_reclaim_limit|: When exceeded this
> causes pending memory to be reclaimed before allocating more memory.'

Done.

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory_unittest.cc (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory_unittest.cc:310:
TEST_F(MappedMemoryManagerTest, MemoryLimit) {
On 2013/08/20 02:46:54, sievers wrote:
> nit: (MappedMemoryManagerTest, UnusedMemoryLimit)

Done.

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory_unittest.cc:343: // the memory limit was
set to be one chunk.
On 2013/08/20 02:46:54, sievers wrote:
> nit: maybe just alloc 1 chunk and then another chunk in this test to make it
> easier to read.
> maybe say something like: 'Expect two chunks to be allocated, exceeding the
> limit, since all memory is in use'.

Done.

[piman, 2013-08-20 22:26:13]

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/fenced_allocator.cc (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/fenced_allocator.cc:166: return blocks_.size() != 1 ||
blocks_[0].state != FREE;
On 2013/08/20 22:15:46, kaanb wrote:
> On 2013/08/20 02:46:54, sievers wrote:
> > Hmm, this function is misleading, right? If you have one free and one
> > free_pending_token block, it would return true here?
> 
> Done.

Not sure we should change this - I actually don't think the way it's been
changed differs from how it was.

InUse is used by MappedMemoryManager to know when it can safely destroy the
underlying shm. It can only do so when all the blocks are actually FREE (i.e. no
FREE_PENDING_TOKEN either). When they're all FREE, they get coalesced so there's
only 1 FREE block - what the  condition used to test.

The new version would only give a different result if there's >1 block and
they're all FREE, which can't happen due to coalescing. So it's needlessly
slower and more complex.

[no sievers, 2013-08-20 22:39:37]

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/fenced_allocator.cc (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/fenced_allocator.cc:166: return blocks_.size() != 1 ||
blocks_[0].state != FREE;
On 2013/08/20 22:26:13, piman wrote:
> On 2013/08/20 22:15:46, kaanb wrote:
> > On 2013/08/20 02:46:54, sievers wrote:
> > > Hmm, this function is misleading, right? If you have one free and one
> > > free_pending_token block, it would return true here?
> > 
> > Done.
> 
> Not sure we should change this - I actually don't think the way it's been
> changed differs from how it was.
> 
> InUse is used by MappedMemoryManager to know when it can safely destroy the
> underlying shm. It can only do so when all the blocks are actually FREE (i.e.
no
> FREE_PENDING_TOKEN either). When they're all FREE, they get coalesced so
there's
> only 1 FREE block - what the  condition used to test.
> 
> The new version would only give a different result if there's >1 block and
> they're all FREE, which can't happen due to coalescing. So it's needlessly
> slower and more complex.

My bad. Shouldn't have commented on this at all. It was just a very mild
irritation about Kaan was adding the notion of bytes 'in use' vs. how this
InUse() mismatches :)

Just leave as is.

[kaanb, 2013-08-20 22:40:52]

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
File gpu/command_buffer/client/fenced_allocator.cc (right):

https://codereview.chromium.org/23130004/diff/46001/gpu/command_buffer/client...
gpu/command_buffer/client/fenced_allocator.cc:166: return blocks_.size() != 1 ||
blocks_[0].state != FREE;
On 2013/08/20 22:26:13, piman wrote:
> On 2013/08/20 22:15:46, kaanb wrote:
> > On 2013/08/20 02:46:54, sievers wrote:
> > > Hmm, this function is misleading, right? If you have one free and one
> > > free_pending_token block, it would return true here?
> > 
> > Done.
> 
> Not sure we should change this - I actually don't think the way it's been
> changed differs from how it was.
> 
> InUse is used by MappedMemoryManager to know when it can safely destroy the
> underlying shm. It can only do so when all the blocks are actually FREE (i.e.
no
> FREE_PENDING_TOKEN either). When they're all FREE, they get coalesced so
there's
> only 1 FREE block - what the  condition used to test.
> 
> The new version would only give a different result if there's >1 block and
> they're all FREE, which can't happen due to coalescing. So it's needlessly
> slower and more complex.

Done.

[no sievers, 2013-08-20 22:48:10]

lgtm

https://codereview.chromium.org/23130004/diff/42003/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.h (right):

https://codereview.chromium.org/23130004/diff/42003/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.h:118: size_t memory_limit);
nit: also unused_memory_reclaim_limit here and move the documentation from .cc
to .h file

[piman, 2013-08-20 22:48:29]

lgtm

[kaanb, 2013-08-20 22:56:01]

https://codereview.chromium.org/23130004/diff/42003/gpu/command_buffer/client...
File gpu/command_buffer/client/mapped_memory.h (right):

https://codereview.chromium.org/23130004/diff/42003/gpu/command_buffer/client...
gpu/command_buffer/client/mapped_memory.h:118: size_t memory_limit);
On 2013/08/20 22:48:10, sievers wrote:
> nit: also unused_memory_reclaim_limit here and move the documentation from .cc
> to .h file

Done.

[commit-bot: I haz the power, 2013-08-20 22:59:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/23130004/93001

[commit-bot: I haz the power, 2013-08-20 22:59:51]

Failed to apply patch for gpu/command_buffer/client/gles2_implementation.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file gpu/command_buffer/client/gles2_implementation.h
  Hunk #1 FAILED at 21.
  Hunk #2 FAILED at 98.
  2 out of 5 hunks FAILED -- saving rejects to file
gpu/command_buffer/client/gles2_implementation.h.rej

Patch:       gpu/command_buffer/client/gles2_implementation.h
Index: gpu/command_buffer/client/gles2_implementation.h
diff --git a/gpu/command_buffer/client/gles2_implementation.h
b/gpu/command_buffer/client/gles2_implementation.h
index
c2bf49f41fc2a4dfdffceca6e5a82d8fbf0112d2..e6fc1148b11016434e7abbb815b297211eff070b
100644
--- a/gpu/command_buffer/client/gles2_implementation.h
+++ b/gpu/command_buffer/client/gles2_implementation.h
@@ -21,6 +21,7 @@
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "gpu/command_buffer/client/gpu_memory_buffer_tracker.h"
 #include "gpu/command_buffer/client/image_factory.h"
+#include "gpu/command_buffer/client/mapped_memory.h"
 #include "gpu/command_buffer/client/query_tracker.h"
 #include "gpu/command_buffer/client/ref_counted.h"
 #include "gpu/command_buffer/client/ring_buffer.h"
@@ -98,7 +99,6 @@ struct GLUniformDefinitionCHROMIUM;
 
 namespace gpu {
 
-class MappedMemoryManager;
 class ScopedTransferBufferPtr;
 class TransferBufferInterface;
 
@@ -115,6 +115,9 @@ class VertexArrayObjectManager;
 // shared memory and synchronization issues.
 class GLES2_IMPL_EXPORT GLES2Implementation : public GLES2Interface {
  public:
+  enum MappedMemoryLimit {
+    kNoLimit = MappedMemoryManager::kNoLimit,
+  };
   class ErrorMessageCallback {
    public:
     virtual ~ErrorMessageCallback() { }
@@ -184,7 +187,8 @@ class GLES2_IMPL_EXPORT GLES2Implementation : public
GLES2Interface {
   bool Initialize(
       unsigned int starting_transfer_buffer_size,
       unsigned int min_transfer_buffer_size,
-      unsigned int max_transfer_buffer_size);
+      unsigned int max_transfer_buffer_size,
+      unsigned int mapped_memory_limit);
 
   // The GLES2CmdHelper being used by this GLES2Implementation. You can use
   // this to issue cmds at a lower level for certain kinds of optimization.
@@ -215,7 +219,6 @@ class GLES2_IMPL_EXPORT GLES2Implementation : public
GLES2Interface {
       GLuint program, GLuint index, GLsizei bufsize, GLsizei* length,
       GLint* size, GLenum* type, char* name);
 
-  void SetSharedMemoryChunkSizeMultiple(unsigned int multiple);
 
   void FreeUnusedSharedMemory();
   void FreeEverything();

[commit-bot: I haz the power, 2013-08-20 23:10:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/23130004/73001

[commit-bot: I haz the power, 2013-08-21 00:04:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/23130004/76001

[commit-bot: I haz the power, 2013-08-21 01:05:24]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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-08-21 04:43:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/23130004/110001

[commit-bot: I haz the power, 2013-08-21 06:41:10]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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-08-21 07:07:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/23130004/129001

[commit-bot: I haz the power, 2013-08-21 08:04:55]

Failed to trigger a try job on win_x64_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-08-21 08:05:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/23130004/120001

[commit-bot: I haz the power, 2013-08-21 10:51:54]

Change committed as 218693