Help improving this desing using C++

Started by
5 comments, last by HexDump 9 years, 7 months ago

Hi,

I spent today playing with object pools in C++ and got to a solution that works but have little quirks. I would like to hear opinions from other pals about this design (not if the pooling is efficient or not, but the system structure) based in some requisites I imposed at first. This are:

1) I want a centralized place for managing my object pools

2) I want my object pool to be a template.

3) As I want to have a pool manager and my pool class is templatized I need a class to inherit from. This class must be the an interface and must have 3 methods. init, newObject and deleteObject.

What I don't like from the solution I'm providing is the dynamic_casts I must do to create and object and to return an object to the pool. This is mainly the main problem I see. Another thing I don't like but I think that there's no solution (beside providing a custom allocator in the init method) is that Object pools and PoolMgr rely on some static methods that the IPoolables must implement like getClassId() and create(). What I don't like is that there's no information in the interfaces to force the user to implement in the specific classes.

Think that all this code has been written in a little rush while I was designing so it could contain bugs. I'm just interested in the idea, ussability and how classes play.

Well here is my solution. If anyone could give any idea, different aproach, or confirm you like it (I doubt it smile.png) I would be really happy.

#IPoolable interface


//IPoolable class. All objects that want to be pooled must inherit from this
class IPoolable
{
public:
	virtual void reset() = 0;
	virtual void create() = 0;
};

# ObjectPool interface


class IObjectPool
{
public:
    virtual void init(unsigned int initialCapacity, unsigned int chunkSize, unsigned int maxCapacity)=0;
    virtual void deleteObj(IPoolable* obj)=0;
    virtual IPoolable* newObj()=0;
};


#ObjectPool


//Object pool that contains specific IPoolables
template <typename T>
class ObjectPool : public IObjectPool
{
public:
	ObjectPool()
	{
		CCLOG("CocosObjectPool created...");
	}
	~ObjectPool()
	{
		CCLOG("CocosObjectPool destroyed...");
	}
	
	virtual void init(unsigned int initialCapacity, unsigned int chunkSize, unsigned int maxCapacity);
	virtual void deleteObj(IPoolable* obj);
	virtual IPoolable* newObj();
	
		
protected:
	
private:
	void _GrowStack(int growSize);
private:
	int _initialCapacity=0;
	int _maxCapacity=0;
	int _chunkSize = 0;
	std::queue<T*> _objects;
};
	
template <typename T>
void ObjectPool<T>::init(unsigned int initialCapacity, unsigned int chunkSize, unsigned int maxCapacity)
{
	_initialCapacity = initialCapacity;
	_chunkSize = chunkSize;
	_maxCapacity = maxCapacity;
	
	if (_initialCapacity != 0)
	{
		_GrowStack(initialCapacity);
	}
}
	
template <typename T>
void ObjectPool<T>::_GrowStack(int growSize)
{
	for (int i = 0; i < growSize; ++i)
	{
		_objects.push(T::create());
	}
}
	
template <typename T>
void ObjectPool<T>::deleteObj(IPoolable* obj)
{
	T* t = dynamic_cast<T*>(obj);
	_objects.push(t);
}
	
template <typename T>
IPoolable* ObjectPool<T>::newObj()
{
	if (_objects.size() == 0)
		_GrowStack(_chunkSize);
	
	T* obj = _objects.front();
	_objects.pop();
	return obj;
}

#PoolMgr


//PoolMgr
class PoolMgr
{
public:
	PoolMgr(){};
	virtual ~PoolMgr(){};
	
	void init(){};
		
	template <typename T> void registerType(unsigned int initialCapacity, unsigned int chunkSize, unsigned int maxCapacity);
	template <typename T> T* createObj();
	template <typename T> void deleteObj(IPoolable* obj);
		
	
protected:
private:
	std::map<size_t, unique_ptr<IObjectPool>> _pools;
};

template <typename T> void PoolMgr::registerType(unsigned int initialCapacity, unsigned int chunkSize, unsigned int maxCapacity)
{
	unique_ptr<ObjectPool<T>> op(new ObjectPool<T>());
	op->init(initialCapacity,chunkSize,maxCapacity);
	_pools.insert(make_pair(T::getClassId(), std::move(op)));
}

template <typename T> T* PoolMgr::createObj()
{
	auto it = _pools.find(T::getClassId());
	return dynamic_cast<T*> ( it->second->newObj() );
}
	
template <typename T> void PoolMgr::deleteObj(IPoolable* obj)
{
	auto it = _pools.find(T::getClassId());
	return it->second->deleteObj(obj);
}


This is how it is used from client code:


using namespace FosforeEngine;

//Using Pool manager
FosforeEngine::PoolMgr* poolMgr = new FosforeEngine::PoolMgr();
poolMgr->registerType<ZombieBunny>(5,5,10);
ZombieBunny* bunny = poolMgr->createObj<ZombieBunny>();

//Without registering the pool to a pool manager
ObjectPool<ZombieBunny>* objectPool = new ObjectPool<ZombieBunny>();
objectPool->init(5, 5, 10);
ZombieBunny* zombie=dynamic_cast<ZombieBunny*>(objectPool->newObj());
objectPool->deleteObj(zombie);

Well here you are, sorry for the long post.

Cheers.

Advertisement

Your 3 requirements are counter-intuitive to the end goal, I believe. Why make an object pool in the first place?

Typically it's done to mitigate the various performance issues which can happen with multiple small allocations. Here, you'll find if you do a lot of allocations, your solution may become the bottleneck. RTTI, virtual interfaces, and a map lookup for every single allocation are going to kill you performance-wise except in trivial situations.

So, the big question is - why are you designing this?

Hi,

I will try to answer your question: I'm designing this that way because I'm trying to mimic a pool manager I had in C# and Java. These languages base their allocations in the heap and there's no access to low level memory management methods/functions.

I know the weakness of my aproach related to memory management. Anyway, for the kind of games I'm trying to do I don't mind creating x objects at the beginning an reuse them for the different game objects I may have. As you suggested, This solution is not cache friendly either, etc... But what I really was expecting was someone to bring some ideas to improve the design of the system (as it is). For example improving the interface, removing dynamic_casts, etc... because I need to refresh my c++ and I'm not 100% confident about my code.

Summarizing: I know this is not best way to do pooling. I just was trying to build a system with this dependencies among classes and interface (As an example).If I were going to create a pooling system now, I would create it around a std::vector that provides contiguos memory and would create a Factory for that knew the differents game objects that I need to pool to avoid map lookup, etc... But this was not my question. You were right about your critics though and I accept them smile.png.

Edit: So, to clear things up, is there a better way to aproach this system in a better way (not making pooling more eficient) but making the interface simpler, avoiding dynamic_casts, etc...?

Cheers.

  • You can use static_cast instead of dynamic_cast. There appears to be only one level of inheritance, you're not checking the dynamic_cast to make sure it's not null anyway, and you can reasonably guarantee the cast is to the correct type.
  • Store a pointer to your IObjectPool as a static variable in your IPoolable class instead of "class id", since class id is stored in a static (i.e. global) variable anyway. Then when you call poolMgr->createObj<ZombieBunny>() it directly calls ZombieBunny::pool->newObj. You eliminate the need to have a map or other table look-up, while sticking with your desired design.

With that, your interface stays the same and you improve a couple performance issues, but I still wonder what problem you are solving in the first place.

Thanks for the tips achild.

Think about this post as an exercise I was doing to refresh my C++. Just wanted to know if there could be some design flaws or better ways to achieve same result with a different design using C++

Sorry for the confusion.


Just wanted to know if there could be some design flaws or better ways to achieve same result with a different design.

Sure.

Simple inheritance from an interface class looks sufficient in this case. Looks like you are trying to override construction, "reset", and destruction. So create a base class with a virtual destructor and a virtual reset function and you are covered that way.

Your pool can be replaced with a standard container directly. Really that is all it is right now, a wrapper around a queue, which is a wrapper around a deque. Works well if endpoint manipulation is your usage pattern, but different usage patterns may suggest a different container. Either way a pool is little more than a container for objects implementing a common interface.

No need for templates or dynamic casting on any of that since your code really should know what kind of objects it contains. That is fine if you want to keep it in, but it isn't strictly necessary based on your description. You are leaning on potentially expensive operations that should be completely unnecessary. "Is this object exactly the same kind I put in? Yup, it hasn't changed type."

The manager goes away. Any class named "Manager" is instantly suspect. Give it a concrete name and remember the single responsibility principle. In this case the manager is just a dictionary-style lookup. If you want to do something more or something different than the standard map-style containers, make an adapter class and name it for the actual functionality it provides, perhaps "PoolContainer" or ... wait for it ... "PoolTable" cool.png

Hi!,


Simple inheritance from an interface class looks sufficient in this case. Looks like you are trying to override construction, "reset", and destruction. So create a base class with a virtual destructor and a virtual reset function and you are covered that way.

Yes, you're right I missed to defien the virtual destructor in the interface. About the single inheritance, Which class are you refering to here? I think this is exactly what I have done. haven't I? GameObjects like ZombieBunny inherit directly from IPoolable interface. ObjectPool from IObjectPool interface. Am I missing something?


Your pool can be replaced with a standard container directly. Really that is all it is right now, a wrapper around a queue, which is a wrapper around a deque. Works well if endpoint manipulation is your usage pattern, but different usage patterns may suggest a different container.

Well, you are right that the pool can be mainly seen as a wrapped container but it does a bit more work, for example it makes the container grow if more objects are needed.It limits maximum number of instances (this has not been implemeted but the intention can be found in the init() signature smile.png). This is why I created the Object Pool class.


The manager goes away. Any class named "Manager" is instantly suspect. Give it a concrete name and remember the single responsibility principle. In this case the manager is just a dictionary-style lookup. If you want to do something more or something different than the standard map-style containers, make an adapter class and name it for the actual functionality it provides, perhaps "PoolContainer" or ... wait for it ... "PoolTable" cool.png

This is a good point. Could you elaborate a bit more how would you use the Adapter pattern here? I have used this pattern to adapt an interface to another, can't see the relationship in this example. On the other hand I'm interested in that sentence "Any class named Manager is instantly suspect". Do you mean that it doesn't give any good information about what it does? For me PoolManager is different from PoolContainer. PoolManager is something that get ownership over created pools and is the responsible of deletion, etc... PoolContainer is just something that contains pools without any more logic about them. This is why it is called PoolManager smile.png.

Sorry to be so picky but I like to discuss this kind of things and understand other's point of view.

Cheers.

This topic is closed to new replies.

Advertisement