Make FencedAlloctor fail on size = 0.

Make FencedAlloctor fail on size = 0

BUG=none
R=piman@chromium.org


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

[greggman, 2012-12-03 04:21:55]

Hey Al, PTAL

[greggman, 2012-12-03 04:38:48]

Hey Piman, PTAL

I see a problem. It seems like this should always work

  size = fenced_allocator->GetLargestFreeSize();
  offset = fenced_allocator->Alloc(size);

but if size is 0 it will fail because it always requires at least 1 byte so that
pointers are never the same. I don't see an easy way around that. Is it worth
fixing? 

One way would be to make all zero allocations happen outside the buffer's range,
basically special casing zero. Kinda a scary but if the allocation is zero size
then the app shouldn't be writing any data anyway which is no different than if
it writes more bytes than it allocates. That would require tracking zero
allocations separately since you'd need to know which offsets you can use. Seems
like overkill?

[greggman, 2012-12-03 05:12:21]

On 2012/12/03 04:38:48, greggman wrote:
> Hey Piman, PTAL
> 
> I see a problem. It seems like this should always work
> 
>   size = fenced_allocator->GetLargestFreeSize();
>   offset = fenced_allocator->Alloc(size);
> 
> but if size is 0 it will fail because it always requires at least 1 byte so
that
> pointers are never the same. I don't see an easy way around that. Is it worth
> fixing? 
> 
> One way would be to make all zero allocations happen outside the buffer's
range,
> basically special casing zero. Kinda a scary but if the allocation is zero
size
> then the app shouldn't be writing any data anyway which is no different than
if
> it writes more bytes than it allocates. That would require tracking zero
> allocations separately since you'd need to know which offsets you can use.
Seems
> like overkill?

I uploaded a version with separate zero allocation tracking. Check delta 1 for
the original. I can track the top zero alloc and clear zero_allocs_ when it goes
to zero. Or I can leave it as is. Or I can check in delta 1 (no zero allocation
tracking). I just was thinking that handling the zero issue gets passed all the
way up so it's best to handled it here.

[apatrick, 2012-12-03 20:45:43]

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

https://codereview.chromium.org/11419280/diff/9001/gpu/command_buffer/client/...
gpu/command_buffer/client/fenced_allocator.cc:47: for (size_t ii = 0; ii <
zero_allocs_.size(); ++ii) {
How common is allocating with zero size? You can't just keep incrementing
zero_offset_ on every zero allocation, CHECK it doesn't overflow and make
FreeZeroAlloc a no-op.

Alternatively, if zero allocs are common, how about have a std::set of free zero
offsets so it doesn't have to do a linear search each time?

https://codereview.chromium.org/11419280/diff/9001/gpu/command_buffer/client/...
gpu/command_buffer/client/fenced_allocator.cc:78: zero_allocs_[index] = false;
Would DCHECK(zero_allocs_[index]) make sense here?

[greggman, 2012-12-04 09:24:09]

On 2012/12/03 20:45:43, apatrick wrote:
>
https://codereview.chromium.org/11419280/diff/9001/gpu/command_buffer/client/...
> File gpu/command_buffer/client/fenced_allocator.cc (right):
> 
>
https://codereview.chromium.org/11419280/diff/9001/gpu/command_buffer/client/...
> gpu/command_buffer/client/fenced_allocator.cc:47: for (size_t ii = 0; ii <
> zero_allocs_.size(); ++ii) {
> How common is allocating with zero size? You can't just keep incrementing
> zero_offset_ on every zero allocation, CHECK it doesn't overflow and make
> FreeZeroAlloc a no-op.
> 
> Alternatively, if zero allocs are common, how about have a std::set of free
zero
> offsets so it doesn't have to do a linear search each time?
> 
>
https://codereview.chromium.org/11419280/diff/9001/gpu/command_buffer/client/...
> gpu/command_buffer/client/fenced_allocator.cc:78: zero_allocs_[index] = false;
> Would DCHECK(zero_allocs_[index]) make sense here?

I'm going to abort this CL. I can't see a way to make it work easily because
MemoryChunk has an InChuck(ptr) function and ptrs outside the list fail. Maybe
FencedAllocator should just not allow zero allocations period as opposed to now
where it allocates them sometimes?

[greggman, 2012-12-05 07:19:43]

Hi Piman, PTAL

[piman, 2012-12-05 08:50:52]

lgtm

[commit-bot: I haz the power, 2012-12-05 13:03:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/11419280/11001

[commit-bot: I haz the power, 2012-12-05 16:12:40]

Change committed as 171235