Skip to main content
Minor fixes.
Source Link
glampert
  • 17.2k
  • 4
  • 30
  • 89
  • 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 it FixedSizePool?

  • Instead of defining a custom byte type, use the standard std::uint8_t from <cstdint>. 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 relevantinteresting 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 16bytes16 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.

  • 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 it FixedSizePool?

  • Instead of defining a custom byte type, use the standard std::uint8_t from <cstdint>. 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 relevant 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 16bytes 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.

  • 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 <cstdint>. 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.

Source Link
glampert
  • 17.2k
  • 4
  • 30
  • 89

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 it FixedSizePool?

  • Instead of defining a custom byte type, use the standard std::uint8_t from <cstdint>. 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 relevant 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 16bytes 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.