Fixed-Size Memory Pool

I've been reading about Memory Pools since I came across them in the book Game Programming Complete 4th Edition, and I decided to try and spin my own since I didn't quite understand how the one they presented in the book worked at the time.

One thing I'm uncertain of is defining the deleter in the shared_ptr. I don't like using delegates when they aren't needed, but if I try to pass in the address to the function pointer as the deleter then the program will not compile.

Memory chunks are stored in a singly-linked list, with the first N bytes of each entry in the list being a pointer to the next entry.

While it's nifty using pointer majicks to store the pointer in the head of each chunk, it is scary to look at when you don't understand pointer-to-pointers (I know I didn't when I first tried to roll it out!).

Any suggestions on changes? Improvements? Bugs I didn't see?

```
#include
namespace acorn
{

template
//An immutable memory pool does not grow or shrink in size.
class ImmutableMemoryPool
{
typedef unsigned char byte;

//A pointer to the block of data we will be allocating.
byte* m_pBlock;

//A pointer to the current chunk
byte* m_pCurrentChunk;

//A pointer to the last chunk
byte* m_pLastChunk;

unsigned int m_NumberOfChunks;
unsigned int m_SizeOfChunk;
unsigned int m_SizeOfChunkData;
unsigned int m_SizeOfChunkHeader;

unsigned int m_SizeOfBlock;

public:
ImmutableMemoryPool(unsigned int numberOfChunks) : m_NumberOfChunks(numberOfChunks), m_SizeOfChunkData(sizeof(Object))
{

//Set the size of the header of each chunk
m_SizeOfChunkHeader = sizeof(byte*);

//Calculate the size of a whole chunk
m_SizeOfChunk = m_SizeOfChunkHeader + m_SizeOfChunkData;

//Calculate the size of the block of data
m_SizeOfBlock = m_SizeOfChunk * m_NumberOfChunks;

//

A few things complementing the other answers:

-
Expanding on my previous comment, the name ImmutableMemoryPool gives the idea that the objects allocated by the pool are immutable, which is a pattern used sometimes to allow sharing of immutable instances, but that is not the case here. You meant to say that your pool has a fixed size, so why not call it FixedSizePool?

-
Instead of defining a custom byte type, use the standard std::uint8_t from `. Everyone is already familiar with it.

-
m_SizeOfChunkHeader and m_SizeOfChunkData are constants, so you shouldn't have them wasting memory as class members. They are also currently mutable, which is dangerous. Either just use the sizeof() expression directly, which is fine, or make those two static consts or better static constexpr if you're aiming at C++11 and above.

-
Type casting should be avoided as much as possible. C++ relies on a strong type system. But when dealing with raw memory some type casting is unavoidable. When you have to typecast, use the C++ cast operators. static_cast and reinterpret_cast are your friends. They produce better compiler diagnostics and are also very verbose and colorful on purpose, so they stand out as a clear sign that the programmer is taking matters into his/her own hands, for the better or for worse.

-
The if nesting in Dealloc() is unnecessary. The first condition can just be flipped and early return. The second can also be simplified from an if-then-else to an if-then-return.

-
std::cout is not meant for error logging. You can however use std::cerr if you want printed error output. cerr, or STDERR, is the standard C++ stream for error output. But you might also consider replacing the error logs by exceptions, to give more flexibility to users of your code.

-
Why is InitializeHeaders() protected? You're not planning to inherit from this class, judging by the lack of a virtual destructor, so anything internal should be private. By the way, this is a bit of a personal style, but I find it more interesting to place the private section of a class right at the end. My rationale is that implementation details should not be the first thing you see when you look at a class declaration.

-
Your decision of returning a shared_ptr of the allocated block is questionable. What if all I need is a unique_ptr? My opinion is that this level of memory management is much too low to be dealing with smart pointers. All the user of a memory allocator/pool cares about is a pointer to the allocated block and the block size in bytes, so perhaps your allocator should deal in terms of a Block type? But I'm stealing this idea :P, I strongly suggest watching this excellent presentation: "std::allocator Is to Allocation what std::vector Is to Vexation".

-
Memory alignment is most certainly going to be an issue with your allocator. The first object pulled from the memory pool will be aligned, because operator new returns properly aligned memory, but if the size of the allocated chunk is not evenly divisible by the minimum alignment, the second allocation won't be. This might result in misaligned memory access errors. You will very likely experience that if you try using your allocator with an __m128 vector type that requires 16 bytes of alignment. Taking alignment into account is very easy. You can take advantage of std::alignment_of and std::aligned_storage`. You can also allow the specification of a minimum alignment as an integer template parameter.

-
Tidy up those spurious blank lines here-and-there. One blank line is more than enough to separate two distinct areas of code.

StackExchange Code Review Q#111668, answer score: 5