9
\$\begingroup\$

Relatively new to C++. Please help me understand the potential issues with the following blocking object pool.

#pragma once
#include <vector>
#include <queue>
#include <memory>
#include <thread>
#include <mutex>
#include <condition_variable>
#include <future>
#include <functional>
#include <stdexcept>

template <typename T>
struct DefaultDeleter {
    DefaultDeleter() {};
    void operator()(T* p) const {
        delete p;
    }
};


/// \brief A blocking object pool containing objects of type T.
/// Objects burrowed from the pool are automatically returned to the pool when they
/// are out of scope.
/// \n
/// When the pool is empty calls to borrow objects blocks until a resource is returned to the pool.
template <typename T, typename Deleter = DefaultDeleter<T>>
class ObjectPool {
public:

    /// \brief Construct an object pool of size n.
    ///
    /// \param n The size of the pool.
    /// \param factory A factory method that returns a pointer to a resource.
    ObjectPool(size_t n, std::function<T*()> factory) {
        for(size_t i = 0; i<n; ++i) {
            resources.emplace_back(factory());
        }
    }

    /// \brief destruct the object pool and delete all resources using the provided deleter.
    ~ObjectPool() {
        Deleter d;
        for (auto it = resources.begin(); it != resources.end(); ++it) {
            d(*it);
        }
    };

    /// \typedef Alias for a function which returns a resource to the pool.
    using ObjectReturner = std::function<void(T*)>

    /// \brief burrow an object from the pool.
    /// When the borrowed pointer is out of scope the resource is returned to the pool.
    std::unique_ptr<T, ObjectReturner> borrow() {
        std::unique_lock<std::mutex> lock(mu);
        if (resources.empty()) {
            this->condition.wait(lock, [this]{return !this->resources.empty();});
        }
        auto resource = std::unique_ptr<T, ObjectReturner>(this->resources.front(), [this](T* t){
            std::unique_lock<std::mutex> lock(mu);
            resources.emplace_back(t);
            condition.notify_one();
        });
        this->resources.pop_front();
        return std::move(resource);
    }


private:
    std::deque<T*> resources;

    // synchronization
    std::mutex mu;
    std::condition_variable condition;
};

EDIT: Maybe ObjectPool is a bad term. The intention in here is that this is a ConnectionPool. A common use case looks like this

    struct CloseDb
    {
        void operator()(sqlite3* db) const
        {
            sqlite3_close(db);
        }
    };

    struct Resource {
        std::shared_ptr<ObjectPool<sqlite3, CloseDb>> sqlitePool;
         Resource() {
                std::function<sqlite3*()> factory = [=] ()
                {
                   sqlite3* db;

                    auto rc = sqlite3_open(sqlite3File.c_str(), &db);
                    if (rc)
                     {
                        sqlite3_close(db);
                        std::string err = sqlite3_errmsg(db);
                        sqlite3_close(db);
                        throw std::runtime_error(err);
                     }
                    return db;
              };

            sqlitePool = std::make_shared<ObjectPool<sqlite3, CloseDb>>(std::thread::hardware_concurrency(), factory);
    }
\$\endgroup\$
5
\$\begingroup\$

This is not what I would normally call an Object Pool (in the traditional sense of the term). That term I would use to refer to a pool of memory that returns "Uninitialized" memory (via a pointer) usually via an allocator so that a new operation could build the object (and the memory is returned after it is deleted).

I do like the term borrow() though as that is an accurate description of what is happening. You are temporarily giving ownership of objects that can be returned (in potentially a new state).

Your ownership semantics are wrong though. You return a unique_ptr<> indicating that ownership has been transferred to the borrower. But that is not actually true. The "pool" actually retains ownership as it calls the destructor on the object when it is destroyed (ONLY the owner of an object should try and delete it (reclaim the resource)).

Also if the "pool" is destroyed (thus destroying all the owned pointers); then the "borrowers" are left holding unqiue_ptr<> to objects that no longer exist (bad point 1). But when they do release them the "deleter" tries to put them back into a "pool" that no longer exists (bad point 2).

As it stands this code is just ill-advised.
You need to think some more about your ownership semantics.

\$\endgroup\$
  • 1
    \$\begingroup\$ you said what I tried to say but without all the fluff :) \$\endgroup\$ – Emily L. Mar 14 '17 at 5:19
  • \$\begingroup\$ Thanks for the reply. Maybe a more accurate name would be a ConnectionPool. The term object pool comes from Apache Commons Pool \$\endgroup\$ – 271828183 Mar 14 '17 at 13:19
  • \$\begingroup\$ What is a better alternative to the unique_ptr when I giving away ownership of the connection to some thread? \$\endgroup\$ – 271828183 Mar 14 '17 at 13:21
  • \$\begingroup\$ @271828183: Without redesigning the system I can't tell (I don't have time to build your system 👴 ). Maybe you should store them as an array of shared pointer. \$\endgroup\$ – Martin York Mar 14 '17 at 13:42
  • \$\begingroup\$ Thanks. I wasn't asking you to build a system, but since you provided feedback on the unqiue_ptr I was wondering if you had experience with something more appropriate for the use case :) \$\endgroup\$ – 271828183 Mar 14 '17 at 15:08
4
\$\begingroup\$

Purpose

You never stated why you need an object pool, but I'm going to assume that it is for performance reasons, as it most often is. If you don't have a solid reason for using an object pool, you shouldn't. It just complicates things for no gain.

With that in mind the performance gain from an object pool is for two reasons:

  1. You have already pre-allocated all the objects in one big block, allocating objects from the pool thus bypasses new/delete (or malloc/free) and the risk of page allocations and all that fun stuff as we have already forced the page allocations on initialisation of the object pool. Hence allocation and de-allocation is typically faster but more importantly more consistent in time.
  2. All the objects are contiguous in memory. This reduces memory fragmentation and improves cache coherency somewhat if you access the objects frequently.

Your design with a trivial factory that calls new for each invocation will achieve the first benefit above, but not the second. To obtain the second benefit you would need to create non-trivial factory and deleter which to me seems like unnecessary work.

For this reason I believe that your overall design is poor as you can get both benefits for no additional hassle for the user with a better design.

Contiguous memory object pool

To obtain all the performance gains we need to use a contiguous memory region for our objects. I'm going to show an untested example (without the blocking part in your implementation) of how you could do it:

template<typename T>
class ObjectPool{
private:
    // This class makes sure we respect the alignment of T and that
    // the ctor and dtor of vector don't call ctor and dtor of T
    // as we have to decide when and how to do this ourselves.
    class alignas(T) Object{
        std::array<byte, sizeof(T)> data;
    };

    class Deleter{
    private:
        std::stack<size_t>& freeStack;
        size_t index;
    public:
        Deleter(std::stack<size_t>& s, size_t i)
        : freeStack(s), index(i)
        {}

        void operator () (T* p){
            p->~(); // Call dtor
            freeStack.push(index); // Do appropriate synchronisation here
        }
    }

public:
    ObjectPool(size_t n)
    : pool(n)
    {
        while(n > 0){
            freeIndicies.push(n-1);
            n--;
        }
    }

    // Copy Ctor and assignment are not sensible for a pool.
    ObjectPool(const ObjectPool&) = delete;
    ObjectPool& operator = (const ObjectPool&) = delete;

    ~ObjectPool(){
        if(freeIndecies.size() != pool.size()){
            // Pool destroyed before all objects died!
        }
        // Otherwise no cleanup as each unique_ptr cleans up after themselves
    }

    template<typename... Args>
    std::unique_ptr<Object, Deleter> alloc(Args&&... args){
        if(freeIndicies.empty()){
            // Handle no free objects
        }
        int index = freeIndicies.top();
        freeIndicies.pop();
        // Use placement new and forward all arguments to the constructor of T
        auto object = new (&pool[index]) T(std::forward<Args>(args)...);


        return std::unique_ptr<Object, Deleter>(object, Deleter(freeIndicies, index);
    }
private:
    std::stack<size_t> freeIndicies;
    std::vector<T> pool;
};

The above simply creates a vector of raw arrays of appropriate size and alignment. This means that when the vector is created, no objects of type T have been constructed (i.e. you don't need a default constructor). It also means that upon destruction of the vector the destructors of T will not be called, this avoids double destruction as we make sure to manually call the destructor in the deleter of the unique_ptr.

In the deleter we also return the object to the pool without actually freeing the memory but we do let the object destruct properly (and release any memory it may hold internally). This is an improvement over the original code which would not properly destruct or construct the objects between uses (without releasing the memory).

We also use variadic templates to allow all constructors of T to be used for constructing the object directly which is much more powerful than the factory method you used.

As Loki points out, this code still suffers from a problem where if the pool is destroyed before all the handed out objects die, then there will be undefined behaviour (and most likely a crash). This is a problem with all object pools of the contiguous memory kind (as above). Either crashing or leaking memory if all objects are not returned before the pool is destroyed. And the use of unique_ptr is still semantically incorrect (as you don't have unique ownership of the data, you have a unique lease, which is almost, but not quite the same thing).

I hope this helps!

\$\endgroup\$
  • \$\begingroup\$ Thanks for the thorough reply. Please see my edit for some clarification. Apologies for not including that earlier. \$\endgroup\$ – 271828183 Mar 14 '17 at 13:25

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.