Use mapped memory for uploading large textures.

Use mapped memory for uploading large textures.

When TexImage2D/TexImage3D is called with an image that is larger than
the ring buffer, we had previously fallen back to calling
TexSubImage2D/TexSubImage3D multiple times. This is suboptimal because
we always issue a clear along with TexSubImage calls for security
reasons (the image is not being completely replaced so we do not want
to leak old texture data).

This CL now makes it so it falls back to using mapped memory instead. If
we run out of memory, then we fall back to TexSubImage2D as we did
before.

R=jbauman@chromium.org, piman@chromium.org
BUG=462078
Committed: https://crrev.com/4e297d74fef5371fe545e994aa479c5e7b7eb2f9
Cr-Commit-Position: refs/heads/master@{#333851}

[piman, 2015-06-08 21:12:35]

One thing, but otherwise LGTM in principle.

https://codereview.chromium.org/1168853002/diff/1/gpu/command_buffer/client/g...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1168853002/diff/1/gpu/command_buffer/client/g...
gpu/command_buffer/client/gles2_implementation.cc:2218:
mapped_memory_->FreePendingToken(mem, helper_->InsertToken());
Can you force a shallow flush here? Without it, the risk is that if the context
is never flushed (e.g. offscreen rasterization), we'll grow the mapped memory
unbounded (and then run out of address space or memory or file descriptors).

If we get here, that means we have to upload a large texture and the relative
cost of the flush will be minimal.

[David Yen, 2015-06-08 21:34:06]

I noticed TexImage3D had the same problem so I did the same change. TexImage3D
was a bit more complicated so I thought it would be better to share the upload
section of the code, in order to do that I created a "scoped" version of mapped
memory allocation as well. PTAL.

https://codereview.chromium.org/1168853002/diff/1/gpu/command_buffer/client/g...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1168853002/diff/1/gpu/command_buffer/client/g...
gpu/command_buffer/client/gles2_implementation.cc:2218:
mapped_memory_->FreePendingToken(mem, helper_->InsertToken());
On 2015/06/08 21:12:35, piman (Very slow to review) wrote:
> Can you force a shallow flush here? Without it, the risk is that if the
context
> is never flushed (e.g. offscreen rasterization), we'll grow the mapped memory
> unbounded (and then run out of address space or memory or file descriptors).
> 
> If we get here, that means we have to upload a large texture and the relative
> cost of the flush will be minimal.

Done. Although in order to do this in my new version I had to set a flag to do
the flush after releases in the scoped mapped memory class.

[piman, 2015-06-08 21:57:38]

https://codereview.chromium.org/1168853002/diff/20001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1168853002/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:2203: if
(mapped_alloc.valid()) {
It would be good actually in this case, to free the transfer_alloc immediately
(as opposed to pending token as it will in the ScopedTransferBufferPtr
destructor). Otherwise we just tried to fill up the transfer buffer, finishing
everything just to do that, and we'll force the next call to finish again.
Because we haven't/will not actually write anything to that buffer, we can
release immediately.

Maybe you can add a Cancel() call to ScopedTransferBufferPtr that would do that?

https://codereview.chromium.org/1168853002/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:2222: // Check if we can
allocate from mapped memory.
You don't need this code any more, right? This is handled by the
ScopedMappedMemoryPtr path.

[David Yen, 2015-06-08 22:18:01]

Deleted the old code, but now I have an issue of fixing the unit tests.

Unless you have any suggestions, you can hold off on reviewing until I figured
out how to mock the mapped memory manager.

https://codereview.chromium.org/1168853002/diff/20001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1168853002/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:2203: if
(mapped_alloc.valid()) {
On 2015/06/08 21:57:38, piman (Very slow to review) wrote:
> It would be good actually in this case, to free the transfer_alloc immediately
> (as opposed to pending token as it will in the ScopedTransferBufferPtr
> destructor). Otherwise we just tried to fill up the transfer buffer, finishing
> everything just to do that, and we'll force the next call to finish again.
> Because we haven't/will not actually write anything to that buffer, we can
> release immediately.
> 
> Maybe you can add a Cancel() call to ScopedTransferBufferPtr that would do
that?

Is there a way to just mark blocks within the ring buffer as freed? I only see
functions to mark blocks as a free pending token or free the whole ring buffer.

https://codereview.chromium.org/1168853002/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:2222: // Check if we can
allocate from mapped memory.
On 2015/06/08 21:57:38, piman (Very slow to review) wrote:
> You don't need this code any more, right? This is handled by the
> ScopedMappedMemoryPtr path.

Oops, forgot to delete. Done.

[piman, 2015-06-08 22:32:01]

On Mon, Jun 8, 2015 at 3:18 PM, <dyen@chromium.org> wrote:

> Deleted the old code, but now I have an issue of fixing the unit tests.
>
> Unless you have any suggestions, you can hold off on reviewing until I
> figured
> out how to mock the mapped memory manager.
>
>
>
>
https://codereview.chromium.org/1168853002/diff/20001/gpu/command_buffer/clie...
> File gpu/command_buffer/client/gles2_implementation.cc (right):
>
>
>
https://codereview.chromium.org/1168853002/diff/20001/gpu/command_buffer/clie...
> gpu/command_buffer/client/gles2_implementation.cc:2203: if
> (mapped_alloc.valid()) {
> On 2015/06/08 21:57:38, piman (Very slow to review) wrote:
>
>> It would be good actually in this case, to free the transfer_alloc
>>
> immediately
>
>> (as opposed to pending token as it will in the ScopedTransferBufferPtr
>> destructor). Otherwise we just tried to fill up the transfer buffer,
>>
> finishing
>
>> everything just to do that, and we'll force the next call to finish
>>
> again.
>
>> Because we haven't/will not actually write anything to that buffer, we
>>
> can
>
>> release immediately.
>>
>
>  Maybe you can add a Cancel() call to ScopedTransferBufferPtr that
>>
> would do that?
>
> Is there a way to just mark blocks within the ring buffer as freed? I
> only see functions to mark blocks as a free pending token or free the
> whole ring buffer.


Oh, I see. It would need to be added - deleting the corresponding Block,
and update back free_offset_ assuming the Block was the last one.

>
>
>
>
https://codereview.chromium.org/1168853002/diff/20001/gpu/command_buffer/clie...
> gpu/command_buffer/client/gles2_implementation.cc:2222: // Check if we
> can allocate from mapped memory.
> On 2015/06/08 21:57:38, piman (Very slow to review) wrote:
>
>> You don't need this code any more, right? This is handled by the
>> ScopedMappedMemoryPtr path.
>>
>
> Oops, forgot to delete. Done.
>
> https://codereview.chromium.org/1168853002/
>

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

[David Yen, 2015-06-09 02:03:50]

dyen@chromium.org changed reviewers:
+ jbauman@chromium.org

[David Yen, 2015-06-09 02:03:52]

+jbauman to look at mapped_memory changes as well as the unit tests.

I've fixed all the unit tests and added some new ones to test out the new
TexImage2D functionality. Currently we have no good way to mock shared memory
mapping so I did it in a hacky sort of way by simply allocating and freeing it,
expecting the next memory allocation to reuse the same block. This of course
assumes only 1 shared memory allocation is being called...

The other thing I had to add was a check that the mapped memory was not
allocating more than the limit, before it was actually unbounded. I don't expect
this to cause any issues but it might since the limit was not respected before.

The logic before seemed to only affect situations where the amount of free
memory exceeded the limit, in that case it would prefer to wait for pending data
that are presumably going to be freed shortly. It seems to me that the amount of
free space would be uncorrelated with whether or not we should wait for pending
tokens or not, but maybe I am missing something.

https://codereview.chromium.org/1168853002/diff/20001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1168853002/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:2203: if
(mapped_alloc.valid()) {
On 2015/06/08 22:18:01, David Yen wrote:
> On 2015/06/08 21:57:38, piman (Very slow to review) wrote:
> > It would be good actually in this case, to free the transfer_alloc
immediately
> > (as opposed to pending token as it will in the ScopedTransferBufferPtr
> > destructor). Otherwise we just tried to fill up the transfer buffer,
finishing
> > everything just to do that, and we'll force the next call to finish again.
> > Because we haven't/will not actually write anything to that buffer, we can
> > release immediately.
> > 
> > Maybe you can add a Cancel() call to ScopedTransferBufferPtr that would do
> that?
> 
> Is there a way to just mark blocks within the ring buffer as freed? I only see
> functions to mark blocks as a free pending token or free the whole ring
buffer.

I added a DiscardBlock() function but Maintaining free_offset_ and
in_use_offset_ was a bit tricky, I think just marking the block as PADDING is
probably better since the logic to update those variables are well tested in
FreeOldestBlock(). I inspected the code to make sure PADDING is not assumed to
only exist at the end of the ring buffer.

[David Yen, 2015-06-09 02:17:03]

On 2015/06/09 02:03:52, David Yen wrote:
> +jbauman to look at mapped_memory changes as well as the unit tests.
> 
> I've fixed all the unit tests and added some new ones to test out the new
> TexImage2D functionality. Currently we have no good way to mock shared memory
> mapping so I did it in a hacky sort of way by simply allocating and freeing
it,
> expecting the next memory allocation to reuse the same block. This of course
> assumes only 1 shared memory allocation is being called...
> 
> The other thing I had to add was a check that the mapped memory was not
> allocating more than the limit, before it was actually unbounded. I don't
expect
> this to cause any issues but it might since the limit was not respected
before.
> 
> The logic before seemed to only affect situations where the amount of free
> memory exceeded the limit, in that case it would prefer to wait for pending
data
> that are presumably going to be freed shortly. It seems to me that the amount
of
> free space would be uncorrelated with whether or not we should wait for
pending
> tokens or not, but maybe I am missing something.
> 
>
https://codereview.chromium.org/1168853002/diff/20001/gpu/command_buffer/clie...
> File gpu/command_buffer/client/gles2_implementation.cc (right):
> 
>
https://codereview.chromium.org/1168853002/diff/20001/gpu/command_buffer/clie...
> gpu/command_buffer/client/gles2_implementation.cc:2203: if
> (mapped_alloc.valid()) {
> On 2015/06/08 22:18:01, David Yen wrote:
> > On 2015/06/08 21:57:38, piman (Very slow to review) wrote:
> > > It would be good actually in this case, to free the transfer_alloc
> immediately
> > > (as opposed to pending token as it will in the ScopedTransferBufferPtr
> > > destructor). Otherwise we just tried to fill up the transfer buffer,
> finishing
> > > everything just to do that, and we'll force the next call to finish again.
> > > Because we haven't/will not actually write anything to that buffer, we can
> > > release immediately.
> > > 
> > > Maybe you can add a Cancel() call to ScopedTransferBufferPtr that would do
> > that?
> > 
> > Is there a way to just mark blocks within the ring buffer as freed? I only
see
> > functions to mark blocks as a free pending token or free the whole ring
> buffer.
> 
> I added a DiscardBlock() function but Maintaining free_offset_ and
> in_use_offset_ was a bit tricky, I think just marking the block as PADDING is
> probably better since the logic to update those variables are well tested in
> FreeOldestBlock(). I inspected the code to make sure PADDING is not assumed to
> only exist at the end of the ring buffer.

It looks like that this is only used on the compositor thread. It seems that if
we want this to be a soft limit and prefer waiting as we are using more memory
than the limit, we should be waiting based on (allocated_memory_ + size)
instead. something like this outside of the size <= allocated_memory block?

  if (max_free_bytes_ != kNoLimit &&
      (allocated_memory_ + size) > max_free_bytes_) {
    TRACE_EVENT0("gpu", "MappedMemoryManager::Alloc::wait");
    for (size_t ii = 0; ii < chunks_.size(); ++ii) {
      MemoryChunk* chunk = chunks_[ii];
      if (chunk->GetLargestFreeSizeWithWaiting() >= size) {
        void* mem = chunk->Alloc(size);
        DCHECK(mem);
        *shm_id = chunk->shm_id();
        *shm_offset = chunk->GetOffset(mem);
        return mem;
      }
    }
  }

On the other hand, if we unbound it again I have no way to running unit tests
for the TexSubImage2D case, but I'm ok with just removing that test.

[jbauman, 2015-06-09 02:30:13]

https://codereview.chromium.org/1168853002/diff/100001/gpu/command_buffer/cli...
File gpu/command_buffer/client/mapped_memory.cc (right):

https://codereview.chromium.org/1168853002/diff/100001/gpu/command_buffer/cli...
gpu/command_buffer/client/mapped_memory.cc:85: (allocated_memory_ + size) >
max_free_bytes_) {
This wasn't intended to be a hard limit, but rather to prevent it from using
memory unnecessarily (hence the name unused_memory_reclaim_limit for the
argument to the function). I think you should instead have a flag that treats it
as a hard limit, and only use that flag in the new cases.

[David Yen, 2015-06-09 17:07:08]

I've reverted the mapped memory manager changes and added a new concept which
actually limits the max allocation. I've also added a unit test for that.

PTAL.

https://codereview.chromium.org/1168853002/diff/100001/gpu/command_buffer/cli...
File gpu/command_buffer/client/mapped_memory.cc (right):

https://codereview.chromium.org/1168853002/diff/100001/gpu/command_buffer/cli...
gpu/command_buffer/client/mapped_memory.cc:85: (allocated_memory_ + size) >
max_free_bytes_) {
On 2015/06/09 02:30:13, jbauman wrote:
> This wasn't intended to be a hard limit, but rather to prevent it from using
> memory unnecessarily (hence the name unused_memory_reclaim_limit for the
> argument to the function). I think you should instead have a flag that treats
it
> as a hard limit, and only use that flag in the new cases.

Ok, I've separated out this concept and added max_allocated_bytes_ that limits
the maximum number of bytes we allocate.

[piman, 2015-06-09 17:38:27]

https://codereview.chromium.org/1168853002/diff/120001/gpu/command_buffer/cli...
File gpu/command_buffer/client/ring_buffer.cc (right):

https://codereview.chromium.org/1168853002/diff/120001/gpu/command_buffer/cli...
gpu/command_buffer/client/ring_buffer.cc:119: // updating all the various
offsets.
I think this is close to not doing anything... If used like a RingBuffer (common
case), the freed block will be the last one, at the end of the blocks. The next
allocation will then try to allocate after that block (because we're not
modifying free_offset_), and if necessary will have to wait to free blocks
starting from the front, before even considering this block. So it's effectively
as if the block was FREE_PENDING_TOKEN with the previous block's token.

So I think it's important, if it's the last block, to move back free_offset_ to
offset and delete the block to allow reuse of this buffer. I guess the only
other constraint is wrt in_use_offset_, in that if they're equal we need to make
sure it indicates completely-free rather than completely-in-use (by clearing
blocks_ and resetting both of them to 0, although I think I can convince myself
they probably already are...).

[David Yen, 2015-06-09 20:11:20]

https://codereview.chromium.org/1168853002/diff/120001/gpu/command_buffer/cli...
File gpu/command_buffer/client/ring_buffer.cc (right):

https://codereview.chromium.org/1168853002/diff/120001/gpu/command_buffer/cli...
gpu/command_buffer/client/ring_buffer.cc:119: // updating all the various
offsets.
On 2015/06/09 17:38:27, piman (Very slow to review) wrote:
> I think this is close to not doing anything... If used like a RingBuffer
(common
> case), the freed block will be the last one, at the end of the blocks. The
next
> allocation will then try to allocate after that block (because we're not
> modifying free_offset_), and if necessary will have to wait to free blocks
> starting from the front, before even considering this block. So it's
effectively
> as if the block was FREE_PENDING_TOKEN with the previous block's token.
> 
> So I think it's important, if it's the last block, to move back free_offset_
to
> offset and delete the block to allow reuse of this buffer. I guess the only
> other constraint is wrt in_use_offset_, in that if they're equal we need to
make
> sure it indicates completely-free rather than completely-in-use (by clearing
> blocks_ and resetting both of them to 0, although I think I can convince
myself
> they probably already are...).

That is a fair point. I have added the logic to actually remove the blocks if
they are at the front or back, I also added unit tests for all the various
corner cases.

[piman, 2015-06-09 22:55:06]

LGTM, but I think the code can be simplified a bit.

https://codereview.chromium.org/1168853002/diff/120001/gpu/command_buffer/cli...
File gpu/command_buffer/client/ring_buffer.cc (right):

https://codereview.chromium.org/1168853002/diff/120001/gpu/command_buffer/cli...
gpu/command_buffer/client/ring_buffer.cc:116: << "block that corresponds to
offset already freed";
nit: I would keep this.

https://codereview.chromium.org/1168853002/diff/140002/gpu/command_buffer/cli...
File gpu/command_buffer/client/ring_buffer.cc (right):

https://codereview.chromium.org/1168853002/diff/140002/gpu/command_buffer/cli...
gpu/command_buffer/client/ring_buffer.cc:121: free_offset_ -= block.size;
nit: I would just do free_offset_ = offset so that you don't have to worry about
wraparound

https://codereview.chromium.org/1168853002/diff/140002/gpu/command_buffer/cli...
gpu/command_buffer/client/ring_buffer.cc:129: free_offset_ -=
blocks_.back().size;
Same here, if you want to pop PADDING ones, free_offset_ = blocks_.back().offset
should work.


Actually, you may want to simply set block.state = PADDING and go directly to
the while loop.

https://codereview.chromium.org/1168853002/diff/140002/gpu/command_buffer/cli...
gpu/command_buffer/client/ring_buffer.cc:132: } else if (&block ==
&blocks_.front()) {
I'm not sure the front part is that useful, because typically the use case is to
discard the last created block, and otherwise that case is handled efficiently
by FreeOldestBlock().

If you want to keep it, you could set block.state = PADDING and then go directly
to the while loop.

[David Yen, 2015-06-10 18:17:10]

https://codereview.chromium.org/1168853002/diff/120001/gpu/command_buffer/cli...
File gpu/command_buffer/client/ring_buffer.cc (right):

https://codereview.chromium.org/1168853002/diff/120001/gpu/command_buffer/cli...
gpu/command_buffer/client/ring_buffer.cc:116: << "block that corresponds to
offset already freed";
On 2015/06/09 22:55:05, piman (Very slow to review) wrote:
> nit: I would keep this.

I've changed it to check if state != PADDING. I think it is okay to discard a
FREE_PENDING_TOKEN.

https://codereview.chromium.org/1168853002/diff/140002/gpu/command_buffer/cli...
File gpu/command_buffer/client/ring_buffer.cc (right):

https://codereview.chromium.org/1168853002/diff/140002/gpu/command_buffer/cli...
gpu/command_buffer/client/ring_buffer.cc:121: free_offset_ -= block.size;
On 2015/06/09 22:55:05, piman (Very slow to review) wrote:
> nit: I would just do free_offset_ = offset so that you don't have to worry
about
> wraparound

Done.

https://codereview.chromium.org/1168853002/diff/140002/gpu/command_buffer/cli...
gpu/command_buffer/client/ring_buffer.cc:129: free_offset_ -=
blocks_.back().size;
On 2015/06/09 22:55:05, piman (Very slow to review) wrote:
> Same here, if you want to pop PADDING ones, free_offset_ =
blocks_.back().offset
> should work.
> 
> 
> Actually, you may want to simply set block.state = PADDING and go directly to
> the while loop.

Done.

https://codereview.chromium.org/1168853002/diff/140002/gpu/command_buffer/cli...
gpu/command_buffer/client/ring_buffer.cc:132: } else if (&block ==
&blocks_.front()) {
On 2015/06/09 22:55:05, piman (Very slow to review) wrote:
> I'm not sure the front part is that useful, because typically the use case is
to
> discard the last created block, and otherwise that case is handled efficiently
> by FreeOldestBlock().
> 
> If you want to keep it, you could set block.state = PADDING and then go
directly
> to the while loop.

I think it is better to handle it here so that GetLargestFreeSizeNoWaiting() can
return a size accommodating this block immediately. In other parts of the code
we expect to wait otherwise.

[David Yen, 2015-06-10 22:31:43]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2015-06-10 22:31:44]

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/1168853002/#ps170001
(title: "Simplified how discard is implemented")

[commit-bot: I haz the power, 2015-06-10 22:34:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1168853002/170001

[David Yen, 2015-06-10 22:39:59]

The CQ bit was unchecked by dyen@chromium.org

[David Yen, 2015-06-10 22:41:22]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2015-06-10 22:41:23]

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/1168853002/#ps190001
(title: "Added dcheck and improved other error messages")

[commit-bot: I haz the power, 2015-06-10 22:41:48]

Commit queue failed due to new patchset.

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1168853002/190001

[commit-bot: I haz the power, 2015-06-10 23:56:28]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2015-06-10 23:57:30]

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