1
\$\begingroup\$

I'm making a Minecraft clone and have made an Object Pool. Looking for some advice on how I have implemented it.

Uses m_availableObjects as a O(1) way of searching for the next available object.

Thank you in advance.

constexpr int INVALID_OBJECT_ID = -1;

//Internal Use - Object Pool
template <class Object>
struct ObjectInPool : private NonCopyable
{
    ObjectInPool(int ID)
        : object(),
        inUse(false),
        ID(ID)
    {}
    ObjectInPool(ObjectInPool&& orig) noexcept
        : object(std::move(orig.object)),
        inUse(orig.inUse),
        ID(orig.ID)
    {
        orig.ID = INVALID_OBJECT_ID;
        orig.inUse = false;
    }
    ObjectInPool& operator=(ObjectInPool&& orig) noexcept
    {
        object = std::move(orig.object);
        inUse = orig.inUse;
        ID = orig.ID;

        orig.ID = INVALID_OBJECT_ID;
        orig.inUse = false;
        return *this;
    }
    ~ObjectInPool() {}

    Object object;
    bool inUse;
    int ID;
};

//External Use - Object Pool
template <class Object>
class ObjectPool;
template <class Object>
struct ObjectFromPool : private NonCopyable
{
    ObjectFromPool(ObjectInPool<Object>* objectInPool, std::function<void(int)> func)
        : objectInPool(objectInPool),
        m_func(func)
    {
        if (objectInPool)
        {
            objectInPool->inUse = true;
        }
    }
    ~ObjectFromPool()
    {
        if (objectInPool)
        {
            objectInPool->object.reset();
            objectInPool->inUse = false;

            m_func(objectInPool->ID);
        }
    }
    ObjectFromPool(ObjectFromPool&& orig) noexcept
        : objectInPool(orig.objectInPool),
        m_func(orig.m_func)
    {
        orig.objectInPool = nullptr;
    }
    ObjectFromPool& operator=(ObjectFromPool&& orig) noexcept
    {
        objectInPool = orig.objectInPool;
        m_func = orig.m_func;

        orig.objectInPool = nullptr;

        return *this;
    }

    Object* getObject() const 
    {
        return (objectInPool ? &objectInPool->object : nullptr);
    }

private:
    ObjectInPool<Object>* objectInPool;
    std::function<void(int)> m_func;
};

using std::placeholders::_1;

//Object Pool
template <class Object>
class ObjectPool : private NonCopyable, private NonMovable
{
public:
    ObjectPool(int visibilityDistance, int chunkWidth, int chunkDepth)
        : m_objectPool()
    {
        //Added a little bit more than neccessary due to how the inifinite map generates
        int x = visibilityDistance / chunkWidth;
        x += x += 1;
        int z = visibilityDistance / chunkDepth;
        z += z += 1;

        m_objectPool.reserve(x * z);
        for (int i = 0; i < x * z; ++i)
        {
            m_objectPool.emplace_back(i);
            m_availableObjects.push(i);
        }
    }

    ObjectFromPool<Object> getNextAvailableObject()
    {   
        if (!m_availableObjects.empty())
        {
            int ID = m_availableObjects.top();
            m_availableObjects.pop();
            assert(ID < m_objectPool.size());
            return ObjectFromPool<Object>(&m_objectPool[ID], std::bind(&ObjectPool<Object>::releaseID, this, _1));
        }
        else
        {
            return ObjectFromPool<Object>(nullptr, std::bind(&ObjectPool<Object>::releaseID, this, _1));
        }
    }

private:
    std::stack<int> m_availableObjects;
    std::vector<ObjectInPool<Object>> m_objectPool;

    void releaseID(int ID)
    {
        assert(ID != INVALID_OBJECT_ID);
        m_availableObjects.push(ID);
    }
};
\$\endgroup\$
3
\$\begingroup\$

This looks way too overengineered. Is there really a need for all the complexity? Use the KISS principle!

I'm assuming you have this pool because you want to avoid the cost of constructing and destructing Objects, and possibly to allocate a lot of instances of Object up front to avoid latency later on. If that's the case, I would instead write the pool like so:

template<typename Object>
class ObjectPool: private NonCopyable
{
    std::deque<std::unique_ptr<Object>> m_objects;

public:
    ObjectPool(size_t count = 0) {
        for(size_t i = 0; i < count; ++i) {
            auto object = std::make_unique<Object>();
            enqueue(object);
        }
    }

    std::unique_ptr<Object> dequeue() {
        if (m_objects.empty()) {
            return std::make_unique<Object>();
        } else {
            auto object = std::move(m_objects.back());
            m_objects.pop_back();
            return object;
        }
    }

    void enqueue(std::unique_ptr<Object> &object) {
        m_objects.emplace_back(std::move(object));
    }
};

First, the constructor of this object pool just takes the initial size of the pool as a single value, so it doesn't have to know the specifics of chunk sizes.

Second, the dequeue() function creates a new object for you automatically if the pool is empty. Maybe that is not desired, in which you could return an empty std::unique_pointer instead.

Last but not least, instead of leaving it up to the destructor of an ObjectFromPool to return the object back to the pool it came from, there is now a counterpart to dequeue(): enqueue(). It simply returns an object back to its pool. This avoids having to wrap objects in custom classes.

The only issue with this solution might be that you forget to return an object to its pool, but on the other hand it will still safely destruct itself once its lifetime ends.

\$\endgroup\$
1
  • \$\begingroup\$ This is great, thank you! I think I went with my option to try and keep my code cache friendly but I don't think I actually succeeded with that and definitely didn't follow the KISS principle like you say. In theory could I use RAII to wrap the dequeue & enqueue functions with my own object types too? \$\endgroup\$
    – Ryan Swann
    May 27 '20 at 22:51
3
\$\begingroup\$

I agree with G. Sliepen that this seems way too over-engineered. I just wanted to add a few more things.

Is there a reason you're inheriting NonCopyable and NonMovable? This doesn't seem like a good use of inheritance.

Object(const Object&) = delete;
Object(Object&&) = delete;

This is much clearer.

Secondly, x += x += 1; just looks terrible. Focus on readability. The compiler will optimize it anyway, and it does. See here (https://godbolt.org/z/iQOwPg).

Finally, your pool is default-constructing objects at initialization. This is may be fine for small objects, but can lead to performance loss for large objects, especially if you don't use a lot of the objects.

More typically, pool allocators allocate a single block of memory during the start, and internally represent them as linked list of objects. Then, when a user requests an object, it picks the first node from the linked list, constructs the object, and returns it to the user. This method has the added benefit that the user can pass in arguments for the constructor.

Obviously, there are more features you can add, like dynamically resizing the pool if the memory is low.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I would rewrite x = foo; x += x += 1 to: x = 2 * (foo + 1). \$\endgroup\$
    – G. Sliepen
    May 28 '20 at 17:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.