Jump to content
  • Advertisement
Sign in to follow this  
SlashC++Programmer

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

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

If you intended to correct an error in the post then please contact us.

Recommended Posts

Advertisement

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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!