implement MmapSlabAllocator (issue1905049)

[ste...@uplinklabs.net]

Reviewers: Reid Kleckner,

Message:
Here's the draft version of the mmap-based allocator. Please comment,
etc.

Description:
implement MmapSlabAllocator

This memory allocator uses mmap()/munmap().

Signed-off-by: Steven Noonan <ste...@uplinklabs.net>

Please review this at http://codereview.uplinklabs.net/1905049/show

Affected files:
M include/llvm/Support/Allocator.h
M lib/Support/Allocator.cpp

[ste...@uplinklabs.net]

Here are my personal comments. I'd appreciate feedback.

http://codereview.uplinklabs.net/1905049/diff/1/2
File include/llvm/Support/Allocator.h (right):

http://codereview.uplinklabs.net/1905049/diff/1/2#newcode27
include/llvm/Support/Allocator.h:27: class MmapAllocator {
Should this entire block (and the block further down) be #ifdef'd out on
non-UNIX systems? Windows doesn't even _have_ mmap() to my knowledge
(THANK YOU, MICROSOFT), and thus we don't want it declared but not
defined there, I'd think.

http://codereview.uplinklabs.net/1905049/diff/1/2#newcode168
include/llvm/Support/Allocator.h:168: static MmapSlabAllocator
DefaultSlabAllocator;
Do we want this as the default? We probably want MallocSlabAllocator as
default on Windows, and MmapSlabAllocator as default on everything else.

http://codereview.uplinklabs.net/1905049/diff/1/3
File lib/Support/Allocator.cpp (right):

http://codereview.uplinklabs.net/1905049/diff/1/3#newcode22
lib/Support/Allocator.cpp:22: #ifdef __APPLE__
Is this the LLVM way of doing platform specific things?

http://codereview.uplinklabs.net/1905049/diff/1/3#newcode33
lib/Support/Allocator.cpp:33: void *MmapAllocator::Allocate(size_t Size,
size_t /*Alignment*/) {
Perhaps the 2nd parameter can be dropped entirely. I don't even know why
the MallocAllocator keeps it. Historical reasons?

http://codereview.uplinklabs.net/1905049/diff/1/3#newcode36
lib/Support/Allocator.cpp:36: mmap_fd, mmap_fd_offset);
What do these even _do_? I had to add the mmap_fd and mmap_fd_offset for
it to even function properly, otherwise mmap returned MAP_FAILED. Or
perhaps the change that made it return a valid pointer was some tweaking
of adding MAP_PRIVATE or something. Maybe these are unnecessary?

http://codereview.uplinklabs.net/1905049/diff/1/3#newcode49
lib/Support/Allocator.cpp:49: return static_cast<T*>(Allocate(sizeof(T),
0));
Calling (void *)Allocate(size_t, size_t) because it's cleaner (I think)
than to rewrite a copy of the mmap() call.

http://codereview.uplinklabs.net/1905049/show

[reid.k...@gmail.com]

So the two main things to work on are:

- MmapSlabAllocator shouldn't be the default for the BumpPtrAllocator.
At the very least, we should add a command line switch and check it in
the BumpPtrAllocator constructor to choose the allocator.

- We should keep the mmap stuff behind the llvm::sys::Memory interface
with all the other radioactive ifdefs. :)

http://codereview.uplinklabs.net/1905049/diff/1/2
File include/llvm/Support/Allocator.h (right):

http://codereview.uplinklabs.net/1905049/diff/1/2#newcode27
include/llvm/Support/Allocator.h:27: class MmapAllocator {

On 2010/08/04 08:30:26, Steven Noonan wrote:
> Should this entire block (and the block further down) be #ifdef'd out
on
> non-UNIX systems? Windows doesn't even _have_ mmap() to my knowledge
(THANK YOU,
> MICROSOFT), and thus we don't want it declared but not defined there,
I'd think.

Ideally they'd forward to VirtualAlloc on Windows, but that's definitely
a TODO for now. :)

http://codereview.uplinklabs.net/1905049/diff/1/2#newcode168
include/llvm/Support/Allocator.h:168: static MmapSlabAllocator
DefaultSlabAllocator;

On 2010/08/04 08:30:26, Steven Noonan wrote:
> Do we want this as the default? We probably want MallocSlabAllocator
as default
> on Windows, and MmapSlabAllocator as default on everything else.

Probably not. Now I know where those little stiches on the perf.py
graph come from. :) Those are other, smaller BumpPtrAllocators getting
created and destroyed.

You should be able to pass a custom allocator to the BumpPtrAllocator
constructor in the LiveRanges code.

http://codereview.uplinklabs.net/1905049/diff/1/3
File lib/Support/Allocator.cpp (right):

http://codereview.uplinklabs.net/1905049/diff/1/3#newcode22
lib/Support/Allocator.cpp:22: #ifdef __APPLE__

On 2010/08/04 08:30:26, Steven Noonan wrote:
> Is this the LLVM way of doing platform specific things?

The LLVM way of doing these things is to put stuff in
lib/System/Unix/Memory.inc and lib/System/Win32/Memory.inc .

http://codereview.uplinklabs.net/1905049/diff/1/3#newcode33
lib/Support/Allocator.cpp:33: void *MmapAllocator::Allocate(size_t Size,
size_t /*Alignment*/) {

On 2010/08/04 08:30:26, Steven Noonan wrote:
> Perhaps the 2nd parameter can be dropped entirely. I don't even know
why the
> MallocAllocator keeps it. Historical reasons?

It's for compatibility with the other allocators. The idea is that you
can replace BumpPtrAllocator with MallocAllocator, and things will keep
working.

The reason MallocAllocator ignores it is that malloc guarantees to
return memory aligned to 8 bytes on 32-bit platforms and 16 bytes on
64-bit platforms.

mmap returns page-aligned addresses, which usually means 4096 byte
alignment, so I'd say it's safe to ignore it.

http://codereview.uplinklabs.net/1905049/diff/1/3#newcode36
lib/Support/Allocator.cpp:36: mmap_fd, mmap_fd_offset);

On 2010/08/04 08:30:26, Steven Noonan wrote:
> What do these even _do_? I had to add the mmap_fd and mmap_fd_offset
for it to
> even function properly, otherwise mmap returned MAP_FAILED. Or perhaps
the
> change that made it return a valid pointer was some tweaking of adding
> MAP_PRIVATE or something. Maybe these are unnecessary?

I'm not thoroughly familiar with the mmap interface, just what it does
behind the scenes.

I think mmap_fd_offset is unnecessary, since in
lib/System/Unix/Memory.inc they use a 0 literal on all platforms.

I'd think you should be able to just use -1 for the fd on Mac, since
that's what they do there also.

MAP_PRIVATE is the opposite of MAP_SHARED, which doesn't make sense for
anonymous memory. When you have a read file backing the memory,
MAP_PRIVATE means that any modifications you make to the memory will not
be flushed to disk. This is usually implemented by copy-on-write, so
you try to write to the page, that traps, and the OS transparently maps
a new physical page of memory.

MAP_ANON/MAP_ANONYMOUS probably means what you think it does: don't try
to point this memory at a file.

The various PROT's do the obvious thing: allow me to read/write this.

http://codereview.uplinklabs.net/1905049/diff/1/3#newcode49
lib/Support/Allocator.cpp:49: return static_cast<T*>(Allocate(sizeof(T),
0));

On 2010/08/04 08:30:26, Steven Noonan wrote:
> Calling (void *)Allocate(size_t, size_t) because it's cleaner (I
think) than to
> rewrite a copy of the mmap() call.

+1

http://codereview.uplinklabs.net/1905049/show