[llvm-commits] [unladen-swallow] Re: Patch to allow the JITMemoryManager to allocate more blocks of memory

[Reid Kleckner]

On Mon, Jul 13, 2009 at 10:37 PM, Evan Cheng<evan.cheng at apple.com> wrote:
>
> Hi Reid,
>
> Thanks. A couple of comments.
>
> Can you change CheckInvariants() to return true / false and the error
> message by reference instead?

Sure.  That's nice because it makes the ostream deconstructor will
automatically flush things, whereas it didn't before.

> +  /// BumpAllocator - This is a simple memory allocator that simply bumps a
> +  /// pointer forwards across a slab of memory until it runs out, at which
> +  /// point it allocates a new slab.  If the size of an allocation is above
> a
> +  /// certain threshold, the allocator allocates a separate slab for it, to
> +  /// avoid wasted space.  We use this class to allocate function stubs and
> +  /// global data structures.
> +  class BumpAllocator {
>
> Is there a reason not to use the BumpPtrAllocator in Support/Allocator.h?

That allocator uses malloc.  One of the things I was trying to do is
get all the code and globals laid out contiguously in memory by using
the NearBlock parameter of sys::Memory::AllocateRWX.  That isn't
particularly reliable, and it's not even supported on Windows.  It
would still be nice to get all slab allocation centralized through the
memory manager instead of through malloc, though.

I could parameterize the allocator in Support to use either malloc or
the JITMemoryManager, or I could just use mine or the one in Support
wholesale.  Which would you prefer?

> +    for (MemoryRangeHeader *Hdr = (MemoryRangeHeader*)Start, *LastHdr =
> NULL;
> +         Start <= (char*)Hdr && (char*)Hdr < End;
> +         Hdr = &Hdr->getBlockAfter()) {
> +      if (Hdr->ThisAllocated == 0) {
>
> How about "if (!Hdr->ThisAllocated) continue;" to reduce indentation.
> Thanks,

That can't be done, since the 'if' doesn't cover the entire body of
the for loop.  I did notice that the 'else if' clause will never
execute because ThisAllocated is a one bit field, so I dropped that.

Thanks for the review,
Reid
-------------- next part --------------
A non-text attachment was scrubbed...
Name: JITMemoryManager.diff
Type: text/x-diff
Size: 44886 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090714/8badae3d/attachment.diff>