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

[Benjamin Kramer]

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.

- Ben

> 
> -Argyrios
> 
>> 
>> - Ben
>> 
>>> +
>>> if (!CurSlab) // Start a new slab if we haven't allocated one already.
>>>   StartNewSlab();
>>> 
>>> // Keep track of how many bytes we've allocated.
>>> BytesAllocated += Size;
>>> 
>>> -  // 0-byte alignment means 1-byte alignment.
>>> -  if (Alignment == 0) Alignment = 1;
>>> -
>>> // Allocate the aligned space, going forwards from CurPtr.
>>> char *Ptr = AlignPtr(CurPtr, Alignment);
>>> 
>>> @@ -106,7 +112,6 @@
>>> }
>>> 
>>> // If Size is really big, allocate a separate slab for it.
>>> -  size_t PaddedSize = Size + sizeof(MemSlab) + Alignment - 1;
>>> if (PaddedSize > SizeThreshold) {
>>>   MemSlab *NewSlab = Allocator.Allocate(PaddedSize);
>>> 
>>> 
>>> Modified: llvm/trunk/unittests/Support/AllocatorTest.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/AllocatorTest.cpp?rev=151834&r1=151833&r2=151834&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/unittests/Support/AllocatorTest.cpp (original)
>>> +++ llvm/trunk/unittests/Support/AllocatorTest.cpp Thu Mar  1 14:36:32 2012
>>> @@ -93,6 +93,14 @@
>>> EXPECT_EQ(2U, Alloc.GetNumSlabs());
>>> }
>>> 
>>> +// Test allocating with a size larger than the initial slab size.
>>> +TEST(AllocatorTest, TestSmallSlabSize) {
>>> +  BumpPtrAllocator Alloc(128);
>>> +
>>> +  Alloc.Allocate(200, 0);
>>> +  EXPECT_EQ(1U, Alloc.GetNumSlabs());
>>> +}
>>> +
>>> // Mock slab allocator that returns slabs aligned on 4096 bytes.  There is no
>>> // easy portable way to do this, so this is kind of a hack.
>>> class MockSlabAllocator : public SlabAllocator {
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>