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

[Benjamin Kramer]

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!

- Ben

> 
> - 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
>>> 
>> 
>