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 nonot call itFixedSizePool
?Instead of defining a custom
byte
type, use the standardstd::uint8_t
from<cstdint>
. Everyone is already familiar with it.m_SizeOfChunkHeader
andm_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 thesizeof()
expression directly, which is fine, or make those twostatic const
s or betterstatic 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
andreinterpret_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 inDealloc()
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 usestd::cerr
if you want printed error output.cerr
, orSTDERR
, 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 avirtual
destructor, so anything internal should beprivate
. By the way, this is a bit of a personal style, but I find it more relevantinteresting to place theprivate
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 aunique_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 aBlock
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 16bytes16 bytes of alignment. Taking alignment into account is very easy. You can take advantage ofstd::alignment_of
andstd::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.
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 no call itFixedSizePool
?Instead of defining a custom
byte
type, use the standardstd::uint8_t
from<cstdint>
. Everyone is already familiar with it.m_SizeOfChunkHeader
andm_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 thesizeof()
expression directly, which is fine, or make those twostatic const
s or betterstatic 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
andreinterpret_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 inDealloc()
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 usestd::cerr
if you want printed error output.cerr
, orSTDERR
, 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 avirtual
destructor, so anything internal should beprivate
. By the way, this is a bit of a personal style, but I find it more relevant to place theprivate
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 aunique_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 aBlock
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 16bytes alignment. Taking alignment into account is very easy. You can take advantage ofstd::alignment_of
andstd::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.
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 itFixedSizePool
?Instead of defining a custom
byte
type, use the standardstd::uint8_t
from<cstdint>
. Everyone is already familiar with it.m_SizeOfChunkHeader
andm_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 thesizeof()
expression directly, which is fine, or make those twostatic const
s or betterstatic 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
andreinterpret_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 inDealloc()
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 usestd::cerr
if you want printed error output.cerr
, orSTDERR
, 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 avirtual
destructor, so anything internal should beprivate
. By the way, this is a bit of a personal style, but I find it more interesting to place theprivate
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 aunique_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 aBlock
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 ofstd::alignment_of
andstd::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.
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 no call itFixedSizePool
?Instead of defining a custom
byte
type, use the standardstd::uint8_t
from<cstdint>
. Everyone is already familiar with it.m_SizeOfChunkHeader
andm_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 thesizeof()
expression directly, which is fine, or make those twostatic const
s or betterstatic 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
andreinterpret_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 inDealloc()
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 usestd::cerr
if you want printed error output.cerr
, orSTDERR
, 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 avirtual
destructor, so anything internal should beprivate
. By the way, this is a bit of a personal style, but I find it more relevant to place theprivate
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 aunique_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 aBlock
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 16bytes alignment. Taking alignment into account is very easy. You can take advantage ofstd::alignment_of
andstd::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.