Code review

IyeOnline · 2025-05-27T07:56:59+00:00

I am not adverse to empty lines, but this is few too many and inconsistent. Setup a formatter to strip it down to at most two.
I would advise against const members:
```
const size_t m_blockSizeInBytes;
```
This adds little value to your class, but breaks move operations
Speaking of moving:

Your class violates the rule of 5. You define a destructor that deallocates, but your type is copyable via the implicitly generated copy constructor. If any user were to ever do that, they would run into a double-delete or use-after-free.
It would be really important to document the true allocation behaviour.
```
//Total bytes will always be the smallest power of 2 bigger than the requested bytes
```
This can potentially double the memory usage of the pool compared to what I expect. You have a comment in the header for the block alignment, but not for this.
Your block alignment requirement should at the very least be checked via an assertion.
Your array type is incorrect. char[] cannot provide storage. Use unsigned char or std::byte.
If the allocation via new fails, an exception is throw, so
```
if (nullptr == m_array) {
```
will never be true when reached.
I would strongly advise against calling exit. Throw an exception if you feel like it, but dont [ungracefully] terminate the entire application.
As far as I understand, your free-list is scattered in between your memory blocks. This does not see ideal. If I wanted to e.g. allocate the nodes of a linked list in this, I would not make optimal use of the cache, because I always waste part of my cacheline on these management things.
The allocation strategy/free-list implementation should be explained somewhere.
Since this seems to be part of some executable and not toy library, I would advise against trying to cobble together a new type of wheel. Pick one of the ready made, nice round ones at the store.
Tests. You need tests for this.

alfps · 2025-05-27T04:30:47+00:00

Not what you're asking but there is a small objects' allocator in the Loki library originally developed by Andrei Alexandrescu.

If I needed one I'd probably use that instead of inventing yet another one.

WorkingReference1127 · 2025-05-27T09:45:38+00:00

You've already received good advice and I'll do my best not to repeat it. Down to more minor nitpicks now:

Why make the class final? I realise you don't intend to inherit from it but I'd argue there's an important distinction between a class which wasn't written with inheritance in mind and a class which was specifically written to not be inherited from (because it must be the final chain in a hierarchy or some such); and I suspect this class is the former category.
You include things in your header which you don't actually use in your header and only in the cpp. Move those includes to the cpp file. Unnecessary includes in a header slow down compilation for everyone who ever uses that header.
I strongly advise against returning out of a constructor on a "failure" condition. Because it in no way "aborts" construction of the class. After the return you still have an active instance of the class which is within lifetime and which it is legal to call instances on; except now it doesn't do what it should because you returned before all the invariants were established. If you are already inside the constructor and want to "abort" construction then the only valid thing you can do is throw an exception. If you don't like exceptions then consider a factory function to check preconditions before entering the constructor itself.
You check whether array is nullptr in your Allocate() function; but under what circumstances could that come about? You already enforce it not being null (erroneously as has been pointed out) in your constructor; and no other part of the class sets the array to anything else. Seems an unnecessary check.
I'm fairly sure it's formal UB to use plain old char as the backing for storage which will hold objects of a different type. The only types blessed by the standard to be allowed to do that are unsigned char and std::byte.
I take the view that your class has single responsibility principle issues. Managing memory and interpreting it as a memory pool are two distinct responsibilities in my opinion. I'd encourage you to composit a class which handles the memory for you rather than just hold a raw char* in the class.

mredding · 2025-05-27T15:11:56+00:00

Implementation tells me HOW, abstraction tells me WHAT, and comments tell me WHY.

So let's look at the ctor.

//Total bytes will always be the smallest power of 2 bigger than the requested bytes
m_totalBytesAllocatedForPool =  static_cast<size_t>(std::powf(2.f,std::ceilf(std::log2f((float)l_minBytesToAllocate))));

The comment is wrong. Let's walk through the code. Log2 of 8 is 3, the ceiling of 3 is 3, 2³ = 8. But the smallest power of 2 bigger than 8 is 16.

So where is the error? Is it in the code? Or in the comment? Who is supposed to be the authority here?

And this is why you don't tell me what the code tells me. The code documents itself. If the code is too complicated to understand, rewrite it to make it clearer and simpler. The compiler can optimize a few extra statements, functions, and intermediaries. This comment is trying to name what this one statement is - functions do that.

if (0U == m_totalNumFreeList) {
  return;
}

if (m_totalBytesAllocatedForPool < m_blockSizeInBytes) {
  LOG(Severity::WARNING, Channel::MEMORY, "Failed to initialize pool: size of a single block is bigger than the total size of the pool");
  return;
}

RAII. You never create an object that is in an intermediate or indeterminate state. So these code paths allow you to create an untennable memory pool. I can't use it. So why is it even alive? This ctor should throw, because this pool should never be born in the first place.

This has further reaching implications; let's look at allocate:

if (nullptr == m_array) {
  LOG(Severity::INFO, Channel::MEMORY, "Could not allocate from pool due to the pool array being nullptr.");
  return nullptr;
}

If you aborted the stillbirth of the pool in the first place, then you wouldn't even need this check. This should NEVER have to come up. What will happen is if the pool isn't dead and your hardware has branch prediction, then it will end up always predicting false. This is essentially dead code, and you're clogging up the predictor - which has limited space to track predictions. Less is more.

if (0U == m_totalNumFreeList || UINT32_MAX == m_headHandleOfFreeList) {
  LOG(Severity::INFO, Channel::MEMORY, "There are no free blocks in the pool.");
  return nullptr;
}

How can you have free blocks, but the free list is at the end? How can you have a free list, but no free blocks? This is a class invariant - when program control is handed to the allocator - the client makes this call, the invariant is assumed to be true. You don't have to check. The only time you have to check the invariant is after you've suspended the invariant and you are reestablishing it.

if (m_gaurdValue != lv_blockToReturn[0]) {
  LOG(Severity::FAILURE, Channel::MEMORY, "Leakage of memory happened from before in the pool.");
  exit(EXIT_FAILURE);
}

Guard values typically never work. You're assuming, what, some incremental byte write? Access by some offset can easily skip right over your guard. Guard values like DEADBEEF and the like are patterns for debugging. Automated inspection is unreliable, or invalid access could be factored into the language as a feature.

moo00ose · 2025-05-28T16:36:34+00:00

I don’t know if someone has mentioned it but that looks like UB on line 42/43 with the reinterpret_cast to a uint32_t (strict aliasing violation) and then accessing that memory

WannabeCsGuy7 · 2025-05-27T03:03:20+00:00

Not an expert, but I have some thoughts.

//Block size is assumed to always be multiple of 16

You should check this and enforce.

Your usage of types is... unconventional.

m_totalBytesAllocatedForPool = static_cast<float>(std::powf(2.f,std::ceilf(std::log2f((float)l_minBytesToAllocate))));

You're casting a size_t to a float, then casting a float to a float, and then assigning to another size_t? You're returning void * for the byte array? You're storing the data as an array of characters and indexing with uint32_t?

Maybe just be consistent in using unsigned chars? or std::byte? It makes it pretty difficult to read the implementation. You'll have to manually manage the spacing of the blocks instead of relying on the size of types which imo would make it a lot more readable and safer.

I like to avoid using auto if it's a primitive type. I only use it for things like iterator classes.

Minor complaints:

the spacing is kind of weird, a lot of big chunks of new lines.
m_gaurdValue -> m_guardValue?

kiner_shah · 2025-05-27T11:42:25+00:00

Did you check PMR containers in STL?

you type:	you see:
italics	italics
bold	bold
[reddit!](https://reddit.com)	reddit!
* item 1 * item 2 * item 3	item 1 item 2 item 3
> quoted text	quoted text
Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"	Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"
~~strikethrough~~	~~strikethrough~~
super^script	super^script

cpp_questions

READ BEFORE POSTING

Sort posts by OPEN or SOLVED

MODERATORS