[llvm-commits] [llvm] r151834 - in /llvm/trunk: lib/Support/Allocator.cpp unittests/Support/AllocatorTest.cpp

[Argyrios Kyrtzidis]

On Mar 1, 2012, at 2:16 PM, Benjamin Kramer wrote:

> 
> On 01.03.2012, at 22:30, Benjamin Kramer wrote:
> 
>> 
>> On 01.03.2012, at 22:14, Argyrios Kyrtzidis wrote:
>> 
>>> On Mar 1, 2012, at 1:03 PM, Benjamin Kramer wrote:
>>> 
>>>> 
>>>> On 01.03.2012, at 21:36, Argyrios Kyrtzidis wrote:
>>>> 
>>>>> Author: akirtzidis
>>>>> Date: Thu Mar  1 14:36:32 2012
>>>>> New Revision: 151834
>>>>> 
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=151834&view=rev
>>>>> Log:
>>>>> If BumpPtrAllocator is requested to allocate a size that exceeds the slab size,
>>>>> increase the slab size.
>>>>> 
>>>>> Modified:
>>>>> llvm/trunk/lib/Support/Allocator.cpp
>>>>> llvm/trunk/unittests/Support/AllocatorTest.cpp
>>>>> 
>>>>> Modified: llvm/trunk/lib/Support/Allocator.cpp
>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Allocator.cpp?rev=151834&r1=151833&r2=151834&view=diff
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/Support/Allocator.cpp (original)
>>>>> +++ llvm/trunk/lib/Support/Allocator.cpp Thu Mar  1 14:36:32 2012
>>>>> @@ -87,15 +87,21 @@
>>>>> /// Allocate - Allocate space at the specified alignment.
>>>>> ///
>>>>> void *BumpPtrAllocator::Allocate(size_t Size, size_t Alignment) {
>>>>> +  // 0-byte alignment means 1-byte alignment.
>>>>> +  if (Alignment == 0) Alignment = 1;
>>>>> +
>>>>> +  size_t PaddedSize = Size + sizeof(MemSlab) + Alignment - 1;
>>>>> +
>>>>> +  // If requested size exceeds slab size, increase slab size.
>>>>> +  while (PaddedSize > SlabSize)
>>>>> +    SlabSize *= 2;
>>>> 
>>>> Have you measured that this doesn't make us waste a ton of memory?
>>> 
>>> Without my change BumpPtrAllocator will just corrupt memory when "PaddedSize > SlabSize"; do you have an alternative way in mind to handle this ?
>>> 
>>>> Also this makes the "separate slab" code below redundant.
>>> 
>>> Not sure what you mean, could you elaborate ?
>> 
>> When BumpPtrAllocator sees a really big allocation it should allocate a separate slab for just that allocation, without changing the allocation settings. If that corrupts memory, we have a serious bug.
>> 
>>> 
>>>> 
>>>> BumpPtrAllocator already (slowly) grows the allocation size when a lot of stuff is allocated, see StartNewSlab. Back then I tweaked the parameters there to minimize wasted memory in clang's ASTContext. If you're seeing a lot of malloc calls it may make sense to revisit the parameters there or initialize an allocator with a bigger slab size.
>>> 
>>> I'm not interested in reducing malloc calls, the issue is that if the BumpPtrAllocator starts with a small size and then you try to allocate a bigger size than that, you either get assertion or corrupted memory, see the unit test case later in then commit.
>> 
>> Ah, I see. That shouldn't happen if the "separate slab" code was working correctly.
> 
> I committed a minimal fix in r151842. Argyrios, can you check that your issue is still fixed? Thanks!

It is fixed, thanks!

-Argyrios