Public Group

# Does Object Pooling with a Vector in C++ have problems with memory?

This topic is 734 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

Hi guys,

I made some Object Pooling with a Vector in C++. The problem is though does the push_back() and erase() cause memory problems?

Here is my code below

http://pastebin.com/KQL6NRn2

##### Share on other sites

As written this pool appears to buy you exactly nothing.

Look at this line:

Bullet newBullet = pool.GetBullet(); // get a bullet from the pool

Regardless of what GetBullet() returns (which is, I suspect, a "Bullet"), "newBullet" is a copy of that. It's not a reference to an entry in the pool. It's a completely new bullet.

This line:

bullets.push_back(newBullet); // add the bullet to the vector

Then makes another copy of "newBullet," which is ultimately stored in the vector. You don't call reserve() on the bullets vector, so you'll be causing the underlying storage of the vector to reallocate, causing, potentially, more copies.

You should probably post your pool class, as it's likely very broken. Certainly the use of the API is wrong and results in no benefit from using the pool at all. I'm very skeptical of what the pool's ReturnBullet functionality does.

##### Share on other sites

As written this pool appears to buy you exactly nothing.

Look at this line:

Bullet newBullet = pool.GetBullet(); // get a bullet from the pool

Regardless of what GetBullet() returns (which is, I suspect, a "Bullet"), "newBullet" is a copy of that. It's not a reference to an entry in the pool. It's a completely new bullet.

This line:

bullets.push_back(newBullet); // add the bullet to the vector

Then makes another copy of "newBullet," which is ultimately stored in the vector. You don't call reserve() on the bullets vector, so you'll be causing the underlying storage of the vector to reallocate, causing, potentially, more copies.

You should probably post your pool class, as it's likely very broken. Certainly the use of the API is wrong and results in no benefit from using the pool at all. I'm very skeptical of what the pool's ReturnBullet functionality does.

Here is my BulletPool class. Thank you very much for helping me out. :D

http://pastebin.com/ri79ydYm

Edited by SlashC++Programmer

##### Share on other sites

The problem is though does the push_back() and erase() cause memory problems?

Yes, it would, with the way you have things here. If you're passing around pointers to things in a contiguous container like std::vector, performing an operation that causes the container's memory to be reallocated will make those pointers point at garbage, potentially crashing your program. Therefore, you either need to enforce that the pool never changes size (as you're doing here), use a raw array, or use handles instead of pointers.

BulletPool(Bullet, int num)


BulletPool's constructor takes a Bullet, and then does nothing with it? What was the intent here?

Bullet GetBullet() // get bullet from the pool


GetBullet() returns a Bullet by value - it copies the bullet. If you want GetBullet to return a pointer to the bullet (which I'm guessing is your intent here), then you need to actually do that:

Bullet* GetBullet()
{
//...
}

This pool class looks very Java-esque to me - it seems to be assuming that class types are all reference types. You code posted in the OP seems to have the same assumption. Remember that in C++ you must explicitly make something a pointer or a reference if you want reference semantics.

else
{
std::cout << "Error: You exhausted the pool!" << '\n';
}

return mBullets[mCounter];


This seems smelly. In the event of the pool being exhausted, you're just giving the caller the last bullet that was allocated. That could cause all kinds of problems - what happens when all the bullets are freed? I would either return nullptr or throw an exception (or use double-indirection as Khatharr did). Edited by Oberon_Command

##### Share on other sites

These result in the same initialization, but the last one is the most efficient.

BulletPool(Bullet, int num)
{
mCounter = num;

int i = num;
while(--i > -1)
{
mBullets.push_back(Bullet());
}
}
///////////////////////
BulletPool(Bullet, int num)
{
mCounter = num;

for(int i = 0; i < num; i++) {
mBullets.emplace_back();
}
}
///////////////////////
BulletPool(Bullet, int num) :
mBullets(num),
mCounter(num)
{
//empty
}


.

I think that this is not the pattern that you want.

Bullet GetBullet() //this is returning by value - it will make a copy, not a reference
{
if(mCounter > 0)
{
return mBullets[--mCounter];
}
else
{
std::cout << "Error: You exhausted the pool!" << '\n'; //this is an error, but
}

return mBullets[mCounter]; //you go from there to here, returning an object that's already in use
}



.

Maybe something more like this:

bool GetBullet(Bullet** ppBullet) {
if(mCounter > 0) {
*ppBullet = &mBullets[--mCounter];
return true;
}

return false;
}



.

However, the real issue here is the overall approach to the problem: Why are you trying to make a pool like this? You could easily just use a vector and reserve, or even just use a std::array, but this checkout/return idea is not going to work for you. What problem are you trying to solve? What's the goal of this class?

##### Share on other sites

You could easily just use a vector and reserve

Not if OP wants to prevent users from calling push_back or otherwise modifying the size of the container while also being able to set the size of the pool at runtime instead of compile time.

##### Share on other sites
I suggest using a std::deque for backing object pools like this, as push_back/push_front (or any tail/head insertion) on a deque is guaranteed not to invalidate iterators/pointers/references to the objects stored within.

There are better data structures for pools but none in the standard library. Just use the deque for now and replace the guts of the pool down the road if you prove via hard data that you really need to. Edited by SeanMiddleditch

##### Share on other sites
You are still storing bullets by value instead of by reference (pointer), so your code will still copy the pool bullet into the vector of bullets even if the pool bullet is passed back by reference.
Your implementation of GetBullet() still has the same problem of what will happen when all the bullets have been allocated.
What is ReturnBullet supposed to accomplish? The way you have it, it takes a reference to a bullet and then copies the bullet the reference points to into the bullet list. I suspect this won't do what you seem to think it will do. Edited by Oberon_Command

##### Share on other sites

You could easily just use a vector and reserve

Not if OP wants to prevent users from calling push_back or otherwise modifying the size of the container while also being able to set the size of the pool at runtime instead of compile time.

Is that OP's goal? I still don't know what the use case is. A heap array sort of thing? Do Bullet objects really need to be 'checked out' from the container this way?

Edited by Khatharr

1. 1
Rutin
24
2. 2
3. 3
JoeJ
18
4. 4
5. 5

• 38
• 23
• 13
• 13
• 17
• ### Forum Statistics

• Total Topics
631706
• Total Posts
3001835
×