4
\$\begingroup\$

I made a single header object pool for use in personal projects. It's supposed to be easy to use, cross-platform and thread safe. It uses a free list for allocations and a hashmap for dereferencing handles. I'm self taught so I'm unsure if I'm performing any cardinal sins.

While coding it I focused on keeping my code short but legible, so I made use of modern (C++14/17) features. I try to follow CppCoreGuidelines everywhere but some things are too low level so they are in C-style C++ (e.g. memset and memcpy).

ObjectPool.hpp:

#pragma once

#ifndef OBJECT_POOL_PAGE_LENGTH
#define OBJECT_POOL_PAGE_LENGTH 32
#endif
#ifndef OBJECT_POOL_PAGE_ALIGNMENT
#define OBJECT_POOL_PAGE_ALIGNMENT 64
#endif

#define OBJECT_SIZE_1 sizeof(std::size_t)

#define OBJECT_SIZE_2 sizeof(std::size_t) * 2
#define OBJECT_SIZE_4 sizeof(std::size_t) * 4
#define OBJECT_SIZE_8 sizeof(std::size_t) * 8
#define OBJECT_SIZE_16 sizeof(std::size_t) * 16
#define OBJECT_SIZE_32 sizeof(std::size_t) * 32
#define OBJECT_SIZE_64 sizeof(std::size_t) * 64

#include <cstring>
#include <iostream>
#include <list>
#include <memory>
#include <mutex>
#include <sstream>
#include <tuple>
#include <typeinfo>
#include <unordered_map>
#include <vector>

/*! Things related to an aligned generic object pool implementation. */
namespace razaron::objectpool
{
    /*! @cond */
    using ArrayA = AlignedArray<char, OBJECT_POOL_PAGE_LENGTH * OBJECT_SIZE_2, OBJECT_POOL_PAGE_ALIGNMENT>;
    using ArrayB = AlignedArray<char, OBJECT_POOL_PAGE_LENGTH * OBJECT_SIZE_4, OBJECT_POOL_PAGE_ALIGNMENT>;
    using ArrayC = AlignedArray<char, OBJECT_POOL_PAGE_LENGTH * OBJECT_SIZE_8, OBJECT_POOL_PAGE_ALIGNMENT>;
    using ArrayD = AlignedArray<char, OBJECT_POOL_PAGE_LENGTH * OBJECT_SIZE_16, OBJECT_POOL_PAGE_ALIGNMENT>;
    using ArrayE = AlignedArray<char, OBJECT_POOL_PAGE_LENGTH * OBJECT_SIZE_32, OBJECT_POOL_PAGE_ALIGNMENT>;
    using ArrayF = AlignedArray<char, OBJECT_POOL_PAGE_LENGTH * OBJECT_SIZE_64, OBJECT_POOL_PAGE_ALIGNMENT>;

    using PoolA = std::tuple<Handle *, std::list<std::unique_ptr<ArrayA>>, std::shared_ptr<std::recursive_mutex>>;
    using PoolB = std::tuple<Handle *, std::list<std::unique_ptr<ArrayB>>, std::shared_ptr<std::recursive_mutex>>;
    using PoolC = std::tuple<Handle *, std::list<std::unique_ptr<ArrayC>>, std::shared_ptr<std::recursive_mutex>>;
    using PoolD = std::tuple<Handle *, std::list<std::unique_ptr<ArrayD>>, std::shared_ptr<std::recursive_mutex>>;
    using PoolE = std::tuple<Handle *, std::list<std::unique_ptr<ArrayE>>, std::shared_ptr<std::recursive_mutex>>;
    using PoolF = std::tuple<Handle *, std::list<std::unique_ptr<ArrayF>>, std::shared_ptr<std::recursive_mutex>>;

    using PoolTuple = std::tuple<PoolA, PoolB, PoolC, PoolD, PoolE, PoolF>;

    template <typename Pool>
    using Page = typename std::tuple_element<1, Pool>::type::value_type::element_type;

    // clang-format off
    template <typename T>
    using PoolCond1 = std::conditional <sizeof(T) <= OBJECT_SIZE_2, PoolA,
        typename std::conditional <sizeof(T) <= OBJECT_SIZE_4, PoolB,
        typename std::conditional <sizeof(T) <= OBJECT_SIZE_8, PoolC,
        typename std::conditional <sizeof(T) <= OBJECT_SIZE_16, PoolD,
        typename std::conditional <sizeof(T) <= OBJECT_SIZE_32, PoolE, PoolF>::type>::type>::type>::type>;
    // clang-format on
    /*! @endcond */

    /*! Hashmap for mapping Handle%s to pointers. */
    using HandleMap = std::unordered_map<Handle, void *, HandleHash, HandleEqual>;

    /*! Stores objects of any type with size upto \c sizeof(std::size_t)*64 Bytes in contiguous aligned memory.
    *   For more information and examples, see page \ref objectpool.
    */
    class ObjectPool
    {
    public:
        ObjectPool() noexcept;  /*!< Default constructor. */
        template <std::size_t... Is>
        void init(PoolTuple &p);
        /*! Copies an object of type T into the ObjectPool.
        *
        *   @tparam     T                  The type of the object to be moved int o the ObjectPool.
        *
        *   @param      object         The object to copy into the ObjectPool.
        *
        *   @exception  std::length_error  T is too large for ObjectPool.
        *
        *   @retval     Handle             On success, a Handle for accessing the object.
        *   @retval     Handle             On failure, an empty Handle.
        */
        template <class T>
        Handle push(const T &object);

        /*! Moves an object of type T into the ObjectPool.
        *
        *   @tparam     T                  The type of the object to be moved int o the ObjectPool.
        *
        *   @param      object         The object to move into the ObjectPool.
        *
        *   @exception  std::length_error  T is too large for ObjectPool.
        *
        *   @retval     Handle             On success, a Handle for accessing the object.
        *   @retval     Handle             On failure, an empty Handle.
        */
        template <class T>
        Handle push(T &&object);

        /*! Constructs an object of type T directly into the ObjectPool.
        *
        *   @tparam     T                  The type of the object to be moved into the ObjectPool.
        *   @tparam     Args               The parameter pack used to construct the T object.<sup>[1]</sup>
        *
        *   @param      args               Constructor arguments to pass to the constructor of T.
        *
        *   @exception  std::length_error  T is too large for ObjectPool.
        *
        *   @retval     Handle             On success, a Handle for accessing the object.
        *   @retval     Handle             On failure, an empty Handle.
        *
        *   <small><sup>[1]</sup> Don't enter this. It <a title="cppreference" href="http://en.cppreference.com/w/cpp/language/template_argument_deduction">deduced</a> by the compiler.</small>
        */
        template <class T, class... Args>
        Handle emplace(Args... args);

        /*! Gets a pointer to an object in the ObjectPool.
        *
        *   @tparam T                          The type of the object to get from the ObjectPool.
        *
        *   @param  handle                     The Handle used to search for the object in the ObjectPool.
        *
        *   @exception  std::invalid_argument  T and handle are mismatched.
        *   @exception  std::length_error      T is too large for ObjectPool.
        *
        *   @retval T*                         On success, a pointer to the desired object.
        *   @retval nullptr                    On failure, a nullptr.
        */
        template <class T>
        T *get(const Handle &handle);

        //TODO template<class T> std::vector<T*> get(std::vector<Handle> handles);

        /*! Removes an object from the ObjectPool and free's the space for reuse.
        *   It calls the destructor for non-trivially destructible objects.
        *
        *   @tparam T           The type of the object to remove from the ObjectPool.
        *
        *   @param  handle  The Handle of the object to remove from the ObjectPool.
        */
        template <class T>
        void erase(const Handle &handle);

        /*! Moves an object to an earlier free position.
        *
        *   @tparam T   The type of the object to reorder.
        *
        *   @param  handle    The Handle of the object to reorder
        *
        *   @retval     Handle             On success, a Handle for the objects new position.
        *   @retval     Handle             On failure, an empty Handle.
        */
        template <class T>
        Handle reorder(const Handle &handle);

        /*! Removes unused pages, releasing their memory. */
        void shrink();

        /*! Returns the current total capacity in bytes. */
        std::size_t capacity(); // add overload with size parameter. Checks how many size bytes long object can fit.


    private:
        template <class T, class Pool, class... Args>
        Handle allocateConstruct(Args... args);

        template <class T, class Pool>
        Handle allocateMove(T &&object);

        template <class Pool>
        void addPage();

        template <class Pool>
        Page<Pool>* getPage(HandleIndex index);

        template <class T>
        T *getObject(const Handle &handle);

        template <class T, class Pool>
        T *getPointer(const Handle &handle);

        template <class Pool, typename T>
        typename std::enable_if<std::is_pointer<T>::value, HandleIndex>::type getIndex(T ptr);

        template <class T, class Pool>
        void erase(const Handle &handle);

        template <class Pool>
        void shrink();

        PoolTuple _pools;
        HandleMap _hashMap;
        std::mutex _hashMapMutex;
    };

    /* *************************************************
                    PUBLIC FUNCTIONS
    ****************************************************/

    template <std::size_t... Is>
    void ObjectPool::init(PoolTuple &p)
    {
        ((std::get<2>(std::get<Is>(p)) = std::make_shared<std::recursive_mutex>()), ...);
    }

    inline ObjectPool::ObjectPool() noexcept
        : _hashMap{}, _hashMapMutex{}
    {
        init<0, 1, 2, 3, 4, 5>(_pools);
    }

    template <class T>
    inline Handle ObjectPool::push(const T &object)
    {
        // Find the pool that fits T
        using Pool = typename PoolCond1<T>::type;

        T val = object;

        if (sizeof(T) <= OBJECT_SIZE_64)
        {
            return allocateMove<T, Pool>(std::move(val));
        }
        else
        {
            std::stringstream message;
            message << typeid(T).name() << " is too large for ObjectPool. sizeof(" << typeid(T).name() << "): "
                << ".";

            throw std::length_error(message.str());
        }
    }

    template <class T>
    inline Handle ObjectPool::push(T &&object)
    {
        // Find the pool that fits T
        using Pool = typename PoolCond1<T>::type;

        if (sizeof(T) <= OBJECT_SIZE_64)
        {
            return allocateMove<T, Pool>(std::forward<T>(object));
        }
        else
        {
            std::stringstream message;
            message << typeid(T).name() << " is too large for ObjectPool. sizeof(" << typeid(T).name() << "): "
                << ".";

            throw std::length_error(message.str());
        }
    }

    template <class T, class... Args>
    inline Handle ObjectPool::emplace(Args... args)
    {
        // Find the pool that fits T
        using Pool = typename PoolCond1<T>::type;

        if (sizeof(T) <= OBJECT_SIZE_64)
        {
            return allocateConstruct<T, Pool>(args...);
        }
        else
        {
            std::stringstream message;
            message << typeid(T).name() << " is too large for ObjectPool. sizeof(" << typeid(T).name() << "): " << sizeof(T) << ".";

            throw std::length_error(message.str());
        }
    }

    template <class T>
    inline T *ObjectPool::get(const Handle &handle)
    {
        if (handle.size != sizeof(T))
        {
            std::stringstream message;
            message << "Type mismatch. HandleSize: " << handle.size << " != sizeof(T): " << sizeof(T) << ". typeid(T): " << typeid(T).name();

            throw std::invalid_argument(message.str());
        }
        else if (sizeof(T) <= OBJECT_SIZE_64)
        {
            return getObject<T>(handle);
        }
        else
        {
            std::stringstream message;
            message << "HandleSize (" << handle.size << ") too large for ObjectPool.";

            throw std::length_error(message.str());
        }
    }

    template <class T>
    inline void ObjectPool::erase(const Handle &handle)
    {
        // Find the pool that fits T
        using Pool = typename PoolCond1<T>::type;

        if (handle.size != sizeof(T))
        {
            std::stringstream message;
            message << "Type mismatch. HandleSize: " << handle.size << " != sizeof(T): " << sizeof(T) << ". typeid(T): " << typeid(T).name();

            throw std::invalid_argument(message.str());
        }
        else if (sizeof(T) <= OBJECT_SIZE_64)
        {
            return erase<T, Pool>(handle);
        }
        else
        {
            std::stringstream message;
            message << "HandleSize (" << handle.size << ") too large for ObjectPool.";

            throw std::length_error(message.str());
        }
    }

    template <class T>
    inline Handle ObjectPool::reorder(const Handle &handle)
    {
        using Pool = typename PoolCond1<T>::type;

        if (handle.size != sizeof(T))
        {
            std::stringstream message;
            message << "Type mismatch. HandleSize: " << handle.size << " != sizeof(T): " << sizeof(T) << ". typeid(T): " << typeid(T).name();

            throw std::invalid_argument(message.str());
        }

        auto pool = &std::get<Pool>(_pools);

        std::lock_guard<std::recursive_mutex> lk{ *std::get<2>(*pool) };

        // If the first free pointer is located after handle, return null
        if (handle.index < getIndex<Pool>(std::get<0>(*pool)))
            return {};

        T temp;

        // If no object currently exists for handle, returm null
        if (getObject<T>(handle))
            temp = *getObject<T>(handle);
        else
            return {};

        erase<T, Pool>(handle);

        return allocateMove<T, Pool>(std::move(temp));
    }

    inline std::size_t ObjectPool::capacity()
    {
        auto &pA = std::get<PoolA>(_pools);
        auto &pB = std::get<PoolB>(_pools);
        auto &pC = std::get<PoolC>(_pools);
        auto &pD = std::get<PoolD>(_pools);
        auto &pE = std::get<PoolE>(_pools);
        auto &pF = std::get<PoolF>(_pools);

        return std::get<1>(pA).size() * sizeof(ArrayA) + std::get<1>(pB).size() * sizeof(ArrayB) + std::get<1>(pC).size() * sizeof(ArrayC) + std::get<1>(pD).size() * sizeof(ArrayD) + std::get<1>(pE).size() * sizeof(ArrayE) + std::get<1>(pF).size() * sizeof(ArrayF);
    }

    inline void ObjectPool::shrink()
    {
        shrink<PoolA>();
        shrink<PoolB>();
        shrink<PoolC>();
        shrink<PoolD>();
        shrink<PoolE>();
        shrink<PoolF>();
    }

    /* *************************************************
                    PRIVATE FUNCTIONS
    ****************************************************/

    template <class T, class Pool, class... Args>
    inline Handle ObjectPool::allocateConstruct(Args... args)
    {
        return allocateMove<T, Pool>(T{ args... });
    }

    template <class T, class Pool>
    inline Handle ObjectPool::allocateMove(T &&object)
    {
        auto pool = &std::get<Pool>(_pools);

        std::lock_guard<std::recursive_mutex> lk{ *std::get<2>(*pool) };

        // If the next free position pointer points to non-existant page, add a new page
        size_t totalPositions = std::get<1>(*pool).size() * OBJECT_POOL_PAGE_LENGTH;
        if (totalPositions == 0 || totalPositions <= std::get<0>(*pool)->index)
        {
            addPage<Pool>();
        }

        // Get pointers to the current and next free elements
        Handle *curFree = std::get<0>(*pool);
        Handle *nextFree = getPointer<Handle, Pool>(*curFree);

        // Copy object data to the location current free pointer
        std::memcpy(curFree, &object, sizeof(T));

        // Set the pools first free pointer to the next free pointer
        std::get<0>(*pool) = nextFree;

        // Configure a Handle for the newly placed object
        Handle h{ HandleSize{ sizeof(T) }, HandleIndex{ getIndex<Pool>(curFree) }, false };

        // Adds the new object to the ObjectPools hashmap
        {
            std::lock_guard<std::mutex> lk{ _hashMapMutex };

            _hashMap[h] = static_cast<void *>(curFree);
        }

        return h;
    }

    template <class Pool>
    inline void ObjectPool::addPage()
    {
        auto pool = &std::get<Pool>(_pools);

        // Create and push a new page onto the pool
        auto page = new Page<Pool>;
        std::get<1>(*pool).emplace_back(page);

        // Initialize the pages positions with free handles pointing to the next free Handle
        auto pageData = std::get<1>(*pool).back()->data();
        for (auto i = 0; i < OBJECT_POOL_PAGE_LENGTH; i++)
        {
            HandleIndex nextFree = static_cast<HandleIndex>(i + 1 + ((std::get<1>(*pool).size() - 1) * OBJECT_POOL_PAGE_LENGTH));

            Handle h = { static_cast<HandleSize>(page->size() / OBJECT_POOL_PAGE_LENGTH), nextFree, true };
            std::memcpy(&pageData[i * page->size() / OBJECT_POOL_PAGE_LENGTH], &h, sizeof(h));
        }

        // If it's the first page, set the first free position to the beginning of the page
        if (std::get<0>(*pool) == nullptr)
            std::get<0>(*pool) = reinterpret_cast<Handle *>(page->data());
    }

    template <class Pool>
    inline typename std::tuple_element<1, Pool>::type::value_type::element_type* ObjectPool::getPage(HandleIndex index)
    {
        auto pool = &std::get<Pool>(_pools);

        // Quotient is the page number and remainder is the position in that page
        std::div_t d = std::div(index, OBJECT_POOL_PAGE_LENGTH);

        // Finds a pointer to the correct page
        Page<Pool> *page = nullptr;
        for (auto &p : std::get<1>(*pool))
        {
            if (!d.quot)
            {
                page = p.get();
                break;
            }
            d.quot--;
        }

        return page;
    }

    template <class T>
    inline T *ObjectPool::getObject(const Handle &handle)
    {
        std::lock_guard<std::mutex> lk{ _hashMapMutex };

        auto it = _hashMap.find(handle);

        if (it != _hashMap.end())
        {
            return static_cast<T *>(it->second);
        }
        else
            return nullptr;
    }

    template <class T, class Pool>
    inline T *ObjectPool::getPointer(const Handle &handle)
    {
        auto pool = &std::get<Pool>(_pools);

        std::lock_guard<std::recursive_mutex> lk{ *std::get<2>(*pool) };

        // Find the page containg handle
        auto page = getPage<Pool>(handle.index);

        // Quotient is the page number and remainder is the position in that page
        std::div_t d = std::div(handle.index, OBJECT_POOL_PAGE_LENGTH);

        // Find and cast the element refering to objects first byte
        auto objectPtr = reinterpret_cast<T *>(&page->data()[d.rem * std::get<0>(*pool)->size]);

        return objectPtr;
    }

    template <class Pool, typename T>
    inline typename std::enable_if<std::is_pointer<T>::value, HandleIndex>::type ObjectPool::getIndex(T ptr)
    {
        auto pool = &std::get<Pool>(_pools);

        // Find the page that contains ptr
        std::size_t ptrAdr = reinterpret_cast<std::size_t>(ptr);
        std::size_t pageAdr = 0;
        std::size_t diff = 0;
        int pageNumber = 0;

        for (auto &p : std::get<1>(*pool))
        {
            pageAdr = reinterpret_cast<std::size_t>(p->data());
            diff = ptrAdr - pageAdr;

            ++pageNumber;

            if (diff >= 0 && diff < sizeof(Page<Pool>))
                break;
        }

        // Throw if no page found
        if (!(diff >= 0 && diff < sizeof(Page<Pool>)))
        {
            throw std::out_of_range("Pointer is not in any page.");
        }

        // Calculate index relative to it's page
        std::size_t position = ptrAdr - pageAdr;
        position = position / std::get<0>(*pool)->size;

        // Add add sum of preceding positions to get absolute index
        position = position + (pageNumber - 1) * OBJECT_POOL_PAGE_LENGTH;

        // If position is in valid range, return. Else, throw.
        if (position <= std::numeric_limits<HandleIndex>::max())
        {
            return static_cast<HandleIndex>(position);
        }
        else
        {
            std::stringstream message;
            message << "Calculated position too large for HandleIndex max value. std::numeric_limits<HandleIndex>::max()" << std::numeric_limits<HandleIndex>::max();

            throw std::overflow_error(message.str());
        }
    }

    template <class T, class Pool>
    inline void ObjectPool::erase(const Handle &handle)
    {
        auto pool = &std::get<Pool>(_pools);

        std::lock_guard<std::recursive_mutex> lk{ *std::get<2>(*pool) };

        // Get index of first free position
        auto posCurFree = getIndex<Pool>(std::get<0>(*pool));

        // Fail if first free position and object being removed are the same
        if (handle.index == posCurFree) return;

        Handle *ptrToRemove = getObject<Handle>(handle);

        // Call object destructor if it is manually set
        if (std::is_destructible<T>::value && !std::is_trivially_destructible<T>::value)
            reinterpret_cast<T *>(ptrToRemove)->~T();

        // Resets the data back to zero
        std::memset(ptrToRemove, 0, std::get<0>(*pool)->size);

        // If the object being removed is located BEFORE the first free position
        if (handle.index < posCurFree)
        {

            // Setup the object being removed to become the next firstFree pointer
            ptrToRemove->isFree = true;
            ptrToRemove->size = std::get<0>(*pool)->size;
            ptrToRemove->index = posCurFree;

            std::get<0>(*pool) = ptrToRemove;
        }
        // If the object being removed is located AFTER the first free position
        else
        {
            Handle *ptrPrevFree = nullptr;
            Handle *ptrNextFree = std::get<0>(*pool);

            std::size_t posNextFree = getIndex<Pool>(ptrNextFree);

            // Loop through free positions until handle is inbetween prevFree and nextFree
            while (posNextFree < handle.index)
            {
                ptrPrevFree = ptrNextFree;

                ptrNextFree = getPointer<Handle, Pool>(*ptrNextFree);
                posNextFree = getIndex<Pool>(ptrNextFree);
            }

            // Currently, ptrToRemove is zeroed, so I have to get it's index from handle
            ptrPrevFree->index = handle.index;

            // Setup the ptr being removed to be inbetween ptrPrevFree and ptrNextFree
            ptrToRemove->isFree = true;
            ptrToRemove->size = std::get<0>(*pool)->size;
            ptrToRemove->index = static_cast<HandleIndex>(posNextFree);
        }

        // Removes object from the hashmap.
        {
            std::lock_guard<std::mutex> lk{ _hashMapMutex };

            if (!_hashMap.erase(handle))
            {
                std::stringstream message;
                message << "Handle{ size: " << handle.size << ", index: " << handle.index << " }"
                    << " not found in ObjectPool::_hashMap.";

                throw std::out_of_range(message.str());
            }
        }

        return;
    }

    template <class Pool>
    inline void ObjectPool::shrink()
    {
        auto pool = &std::get<Pool>(_pools);

        std::lock_guard<std::recursive_mutex> lk{ *std::get<2>(*pool) };

        auto pages = &std::get<1>(*pool);

        if (!std::get<0>(*pool))
            return;

        std::vector<Handle *> freePtrs{ std::get<0>(*pool) };

        std::size_t lastPos = pages->size() * OBJECT_POOL_PAGE_LENGTH;

        // loop through all free handles
        while (freePtrs.back()->index != lastPos)
        {
            freePtrs.push_back(getPointer<Handle, Pool>(*freePtrs.back()));
        }

        if (freePtrs.size() < OBJECT_POOL_PAGE_LENGTH)
            return;

        lastPos++;
        size_t pos = freePtrs.size();
        size_t toDelete = 0;
        while (pos > 0)
        {
            pos -= OBJECT_POOL_PAGE_LENGTH;

            if (freePtrs[pos]->index == (lastPos -= OBJECT_POOL_PAGE_LENGTH))
                toDelete++;
            else
                break;
        }

        auto begin = pages->begin();
        auto end = pages->end()--;

        std::advance(begin, pages->size() - toDelete);

        pages->erase(begin, end);
    }
}

Handle is a free list handle struct. It's always smaller than sizeof(2*std::size_t) (outside of exotic systems). This is not for review.

// Handling for pointers etc.
using HandleSize = std::size_t; /*!< Represents the size of Handle%d objects. */
using HandleIndex = unsigned short; /*!< Represents the indexed location of Handle%d objects. */
/*! Handles are used to abstract data access away from pointers. */
struct Handle
{
    HandleSize size{}; /*!< The size of the Handle%d object. */
    HandleIndex index{};/*!< The indexed location of the Handle%d object. */
    bool isFree{true};/*!< Whether the index denotes a free or occupied location. */

    /*! Basic equality comparator. */
    bool operator==(const Handle &rhs) noexcept
    {
        return (size == rhs.size && index == rhs.index && isFree == rhs.isFree);
    }
};

struct HandleHash
{
    std::size_t operator()(const Handle &h) const noexcept
    {
        auto hash1 = std::hash<HandleSize>()(h.size);
        auto hash2 = std::hash<HandleIndex>()(h.index);
        return hash1 ^= hash2 + 0x9e3779b9 + (hash1 << 6) + (hash1 >> 2);
    }
};

struct HandleEqual
{
    bool operator()(const Handle &lhs, const Handle &rhs) const noexcept
    {
        return lhs.size == rhs.size && lhs.index == rhs.index;
    }
};

AlignedArray is nothing more than a wrapper around a c-style array allocated in aligned heap memory. This isn't for review.

inline void* aligned_malloc(size_t size, size_t align) noexcept
{
    void *result;
    #ifdef _WIN32
    result = _aligned_malloc(size, align);
    #else
    if(posix_memalign(&result, align, size)) result = 0;
    #endif
    return result;
}

inline void aligned_free(void *ptr) noexcept
{
    #ifdef _WIN32
    _aligned_free(ptr);
    #else
    free(ptr);
    #endif

}

template <class T, std::size_t S, std::size_t A>
struct alignas(A) AlignedArray
{
public:
    T* data() noexcept { return _array.data(); }
    std::size_t size() noexcept { return S; }
    std::size_t alignment() noexcept { return A; }

    T& operator [](std::size_t i) { return _array[i]; }
    void* operator new(std::size_t sz) { return aligned_malloc(sz, A); }
    void operator delete(void* ptr) { return aligned_free(ptr); }

private:
    std::array<T, S> _array{};
};

Example usage:

struct Foo
{
    float x, y;
    Foo(float x, float y) : x{ x }, y{ y } {}
    ~Foo() { std::clog << "Destroyed!" << std::endl; }
};

using Bar = std::array<char, 3>;

ObjectPool pool;

Handle fooHdl = pool.emplace<Foo>(3.14159f, 2.71828f); // In place construction
Handle barHdl = pool.push<Bar>(Bar{"GB"});             // Passing a temporary/rvalue

auto fooPtr = pool.get<Foo>(fooHdl);                   // auto resolves to Foo*
float sum = fooPtr->x + fooPtr->y;

auto barPtr = pool.get<Bar>(barHdl);                   // auto resolves to Bar*
std::cout << barPtr->data() << std::endl;

pool.erase<Foo>(fooHdl);                               // Calls Foo::~Foo then deallocates from pool
pool.erase<Bar>(barHdl);                               // Just deallocates from pool

Throw the Handle and AlignedArray code into the ObjectPool.hpp file and the example into a main.cpp file and it should compile. Remember to use the compiler options -std=c++17 -lpthread or equivalent. The program should output:

destroy
GB
destroy

destroy showing up twice is normal. Even std::vector<Foo>::push_back(Foo&&) would output that.

I would like feedback on a few things:

  • Style: I have a code style that I try to be consistent with. Am I doing anything fundamentally bad?
  • Preprocessor: I'm banal about type safety so avoid using it in all my other code. Is it better to switch the macros over over to constexpres for added type safety?
  • Code: I'd like an overall review but also have specific questions.
    • Am I making proper use of modern C++?
    • Is the code easy to understand?
    • Am I handling thread safety properly?
  • Would you use this over alternatives? I made it just for personal use but designed it to be stranger friendly. Did I accomplish that?

Things I'm aware of but don't mind critique:

  • I can replace all the ArrayX and PoolX typedefs with a single template each. The code is already quite heavily templated so I find the current way easier to read. Also it's friendlier on intellisense/linters.
  • ObjectPool::capacity() and ObjectPool::shrink() can use fold expressions. I only noticed that as I was writing this question...
  • I'm using pointer arithmetic. CppCoreGuidelines says no. I will refactor with gsl::span.
  • I use exceptions (divisive subject). In all my personal code I use exceptions only at the lowest level. The intent of my exceptions is that, if caught, you should exit cleanly and not try to carry on after the stack unwind. I basically use them as a runtime alternative to things I can't (or don't want to) check at compile time. To that end I usually provide helper functions so try/catch blocks are only for debugging/error checking.
  • Spacing is brokeded due to cross-platform/editor development and forgetting to set the tab to space feature in editors after reinstalling Windows/Linux a few times. Plan to fix.
  • ObjectPool::init() is just a helper so should be private but I want it near the constructor for prettiness. Will be replacing with a templated lambda inside the constructor come C++20.
  • It's not nice to have to supply a template argument to the public functions but, just like for std::tuple, it's needed for type safety.

PS. This is my first time using codereview.stackexchange. So I apologise in advance if the question is ill-formed.

\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Overall

The thing that I wonder most about the overall design is why you have opted not to use standard library interfaces wherever possible. It seems to me that an object pool is probably essentially a std::pmr::memory_resource (there is std::pmr::synchronized_pool_resource, for example), which you could then use easily in containers with a std::pmr::polymorphic_allocator... so I could have a whole vector of pooled objects automatically:

auto pool = razaron::objectpool::ObjectPool{};

// ... and assuming ObjectPool is derived from std::pmr::memory_resource
auto pooled_foos = std::pmr::vector<Foo>{std::pmr::polymorphic_allocator<Foo>{&pool}};

// Can now freely create Foo objects in the vector that are in the
// pool, and they could even be automatically erased.

Without the std::pmr::memory_resource interface, I have to manually create all the Foo objects I want in the vector, then have the vector either be a vector<Handle> (which would be clunky to use) or vector<std::reference_wrapper<Foo>> and manually get() each object to store it in the vector. And in either case, the onus is on me to do the cleanup.

(You could even take a cue from std::pmr::synchronized_pool_resource and fall back to an upstream allocator, rather than just failing for large allocations.)

And there's more than that, of course. For example, it seems to me that Handle is almost a smart pointer, so it could simply be std::unique_ptr or std::shared_ptr with a custom deleter. And AlignedArray<T, S, A> is just std::aligned_storage_t<S, A>.

But as neither Handle nor AlignedArray are part of the review, I'll focus just on the object pool itself.

Questions and other issues

... some things are too low level so they are in C-style C++ (e.g. memset and memcpy).

That is misguided thinking. C++ is every bit as low level as C, and in fact sometimes more so, while still providing all the type safety and high-level features you could want.

In particular, any half-decent standard library will define their algorithms to fall back to fast primitives whenever possible. In plain English: std::copy will automatically become std::memcpy/std::memmove if possible, and std::fill will automatically become std::memset. If you don't believe me, just grep your standard library headers to see for yourself.

I'm not saying "replace all your std::memset with std::fill!!!", because that's not really necessary (and it would require bringing in the whole algorithms library). std::memset and std::memcpy are perfectly legitimate tools for low-level stuff like this. I'm just giving you a heads up to not underestimate your friendly neighbourhood standard library implementer. Just write good, clear C++ code, and trust in your tooling (but always double check!).

Style: I have a code style that I try to be consistent with. Am I doing anything fundamentally bad?

Meh, looks perfectly legible to me.

Preprocessor: I'm banal about type safety so avoid using it in all my other code. Is it better to switch the macros over over to constexpres for added type safety?

Yes. Absolutely. In fact, I'd say your use of the preprocessor is a code smell. I wouldn't let it pass review.

Am I making proper use of modern C++?

In my opinion, you're making good use of the language facilities... not so much the library though, as I mentioned above. It's not just not using standard library stuff in the code itself, it's that the overall interface doesn't jibe with standard library practices.

Is the code easy to understand?

Yes, I'd say so.

Am I handling thread safety properly?

There are some dodgy things in there. The top level suggestion I would have is to have a more rigorously consistent practice when it comes to locking and unlocking. For example, you should only, only, ONLY lock in certain functions - say, the top-level user interface functions, for example - and then call reusable implementation functions that assume the locking is already done. You should not need a std::recursive_mutex... that's a red flag to me that code is going to be cavalier about locking.

I'll mention other, more specific concerns deeper in the review.

Would you use this over alternatives? I made it just for personal use but designed it to be stranger friendly. Did I accomplish that?

The code is clean, and it's not a bad concept, and I do see a use for object pools. The showstopper for me is the lack of integration with the rest of the language.

I mean, the point of a pool is to be able to store a bunch of objects together - for better locality - and to wipe them all out together (perhaps by "winking" them out, to borrow John Lakos's term). But if I want to get a bunch of objects in your pool... it's really clunky. I need to write a loop (or use an algo) where I construct each object individually, and then either store the handles or the addresses somewhere (they're not available to me from the pool itself), and then when I'm done, I actually have manually erase them all (in the general case - if I know they're trivially destructable, I don't need to... but then I wouldn't need to do that for any pool, including the standard library ones).

By contrast, if the library used standard library interfaces like std::pmr::memory_resource and smart pointers (particulary standard smart pointers with custom deleters)... pretty much everything I described above becomes automatic. I'd just create my container - any standard container, or any container with a standard allocator interface - and populate it... and that's it. Everything else is managed and cleaned up automatically.

Anywho, on with the review....

Preamble

#pragma once

This is non-portable, and for a very good reason (that's too technical to get into here). Standard include guards work just fine.

You already know about the problems with the #defines.

Typedefs

using ArrayA = AlignedArray<char, OBJECT_POOL_PAGE_LENGTH * OBJECT_SIZE_2, OBJECT_POOL_PAGE_ALIGNMENT>;

There is a very subtle bug in your code caused by an equally subtle flaw in your logic.

You assume that the alignment of T... is less than or equal to the size of T (rounded up to a power of 2). That's not always true.

If I have a type Foo that has very specific alignment requirements, I would run into trouble trying to put it in your object pool. Let's say Foo is defined as:

// aligned on 64 byte cache line size (for example)
struct [[some_special_alignment_intrinsic(64)]] Foo
{
    // but only 4 bytes in size
    std::array<std::byte, 4> data;
};

If I try to store that in your object pool, it will be stored at 4 byte alignment, rather than 64. That could be catastrophic.

To fix this, you'd probably have to go through all the code and change just about every sizeof(T) to std::max(sizeof(T), alignof(T)).

using PoolA = std::tuple<Handle *, std::list<std::unique_ptr<ArrayA>>, std::shared_ptr<std::recursive_mutex>>;

These pool types bug me for a couple reasons.

The first thing that bugs me is that you're using a tuple. Tuples are lazy hacks for grouping data in a throwaway context - like returning multiple values from a function (and only then when the order and meaning of the values is obvious). They are not for building key types in your library.

These pools should be defined as classes, perhaps templated on the alignment. The key signal to you that that's what you should have done is your init() function. It's only purpose is to construct the pools... but if they were their own type, they would construct themselves, and you wouldn't need the init() function.

The second thing that bugs me is that I can't for the life of me think of a situation where you'd need a std::list<std::unique_ptr<T>>. Any time I see a container other vector used - not counting maps and sets - I get suspicious. Vector should be your default container, unless you have particularly special needs, and you've measured to be sure a vector won't cut it. In practice, just about the only thing you gain from using std::list over std::vector is that iterators are not invalidated on insertion or removal - in other words, that the address of something in a list stays constant even as you add/remove items, whereas in a vector, adding/removing stuff can shift the other elements around. But that benefit becomes meaningless when the elements being stored are pointers themselves. std::list<std::unique_ptr<T>> offers no real benefits over std::vector<std::unique_ptr<T>>... and the vector can be much faster and simpler to use (because it's random-access).

I looked through the code, and didn't see any reason why a vector wouldn't work here, though, admittedly, there's a lot of code, so I might have missed something.

The third thing that bugs me std::shared_ptr. std::shared_ptr is a powerful tool, and sometimes you do need it... but it's also too often a tool used by people who don't really know what they're doing. "Shared" doesn't mean shared access, it means shared ownership. Using a shared pointer is essentially admitting: "I don't really know who is responsible for this object". But it seems to me you do know who should be responsible for the mutex: the pool itself.

I looked to see if there was anywhere where the ownership of the mutex was shared, and saw nothing - but again, there's a lot of code, so maybe I missed something. From what I could tell, the only thing you ever do with the mutex is lock/unlock it; it's never passed around.

And that brings me to the fourth and final thing: the fact that you're using a recursive mutex rather than a regular mutex. As with shared pointers, there's almost never any reason to use a recursive mutex. Doing so suggests you're being careless about your locking strategies, and that's a fast ride to deadlocksville. You were probably thinking that recursive mutexes are a safe way to avoid deadlocks, but the opposite is true. The best - and arguably the only - way to avoid deadlocks is to only lock mutexes together (using std::lock or std::scoped_lock), not one at a time. If you try to lock one, then the other... you're cruisin for a bruisin'. And when you're dealing with a recursive mutex, it may be already locked, so you're always in the (possible) position of locking mutexes one after the other.

There doesn't seem to be any particular reason why you need recursive mutexes here anyway.

Also, I presume the only reason you're using a smart pointer here at all is because the mutex isn't movable or copyable, and that won't work with your tuple. That problem would go away if you used a custom type.

So I would recommend:

  1. Using an actual type for your pools, not a tuple.
  2. Using a std::vector<std::unique_ptr<ArrayX>> rather than a list.
  3. Using a std::mutex, rather than a recursive mutex.
  4. Using the mutex directly as a member - not by pointer.

Using an actual type for the pool will allow you to offload a lot of the work of individual pool management to the pool class, making the wrapper class simpler.

using PoolTuple = std::tuple<PoolA, PoolB, PoolC, PoolD, PoolE, PoolF>;

This seems like something that would be better defined in the ObjectPool class, along with Page, PoolCond1, and HandleMap. None of them are in the public interface (well, ignoring init()).

ObjectPool

template <std::size_t... Is>
void ObjectPool::init(PoolTuple &p)
{
    ((std::get<2>(std::get<Is>(p)) = std::make_shared<std::recursive_mutex>()), ...);
}

inline ObjectPool::ObjectPool() noexcept
    : _hashMap{}, _hashMapMutex{}
{
    init<0, 1, 2, 3, 4, 5>(_pools);
}

All this goes away if you use a proper type for your pools, rather than a tuple. It would reduce to:

inline ObjectPool::ObjectPool() = default;

The unordered map prevents it from being either constexpr or noexcept. (The custom pool type may also, depending on how you write it, but it seems to me the pool constructors could be both constexpr and noexcept, except that the vector prevents it being constexpr.)

In any case, I'd say the way you're doing init() is clunky and brittle. This looks like a job for std::apply() and a lambda.

template <class T>
inline Handle ObjectPool::push(const T &object)
{
    // ...
}

template <class T>
inline Handle ObjectPool::push(T &&object)
{
    // Find the pool that fits T
    using Pool = typename PoolCond1<T>::type;

    if (sizeof(T) <= OBJECT_SIZE_64)
    {
        return allocateMove<T, Pool>(std::forward<T>(object));
    }
    else
    {
        std::stringstream message;
        message << typeid(T).name() << " is too large for ObjectPool. sizeof(" << typeid(T).name() << "): "
            << ".";

        throw std::length_error(message.str());
    }
}

Why have the T const& overload? T&& is a forwarding reference. It will work for everything: lvalues, rvalues, everything.

You may want to use decay_t<T> internally here, rather than T directly.

You do the check at runtime for whether a type cannot be handled by the pool... but why? This is something that can be done entirely at compile time, in multiple ways. First, you should probably refigure PoolCond1 to trigger a compile-time error if the size is too big. That's no biggie; probably nothing more than this:

template <typename T>
struct PoolCond1
{
    constexpr auto alignment = std::max(sizeof(T), alignof(T));
    static_assert(alignment > (your max align here));
    using type = std::conditional<alignment <= OBJECT_SIZE_2, PoolA,
        // and so on with the conditional as is
};

With that you don't need the if:

template <class T>
Handle ObjectPool::push(T&& object)
{
    // Will trigger a compile-time error if T is too big.
    using Pool = typename PoolCond1<T>::type;

    return allocateMove<T, Pool>(std::forward<T>(object));
}

But if you're paranoid you can add an if constexpr that checks the size and throws if it's too big. Or a static assert.

If you do opt to do that, I would strongly recommend making custom exception types, rather than simply throwing length_error. This is clunky:

if constexpr (std::max(sizeof(T), alignof(T)) > OBJECT_SIZE_64)
{
    std::stringstream message;
    message << typeid(T).name() << " is too large for ObjectPool. sizeof(" << typeid(T).name() << "): "
        << ".";

    throw std::length_error(message.str());
}

If you created a custom exception type like this:

template <typename T>
class alignment_overflow : public std::length_error
{
public:
    alignment_overflow() :
        std::length_error{make_message_()}
    {}

private:
    static auto make_message_()
    {
        auto message = std::ostringstream{};
        message << typeid(T).name() << " is too large for ObjectPool. sizeof(" << typeid(T).name() << "): .";

        return message.str();
    }
};

That could boil down to:

if constexpr (std::max(sizeof(T), alignof(T)) > OBJECT_SIZE_64)
    throw alignment_overflow<T>{};

Even better, you could make a base class for all pool exceptions:

class pool_error
{
public:
    constexpr explicit pool_error(std::size_t alignment) :
        alignment_{alignment}

    virtual ~pool_error() = default;

    virtual auto alignment() const noexcept -> std::size_t { return alignment_; }
    // maybe other functions with useful info, like the typeid or type name

private:
    std::size_t alignment_;
};

And use that to define your exception types:

template <typename T>
class alignment_overflow :
    public virtual pool_error,
    public virtual std::length_error
{
public:
    alignment_overflow() :
        pool_error{std::max(sizeof(T), alignof(T))},
        std::length_error{make_message_()}
    {}

    // ...
};

And now you can do something like this:

try
{
    // pool operations
}
// all these catches match, so first come first serve
catch (alignment_overflow const& x) {}
catch (pool_error const& x) {}
catch (std::length_error const& x) {}
catch (std::exception const& x)
{
    // if you want the extra info in a general catch...
    if (auto p = dynamic_cast<pool_error const*>(&x); p)
    {
        std::cerr << p->alignment();
    }
}

However you choose to define your custom exceptions, I suggest using them, rather than all the hassle of constructing those what() messages in the middle of code, which clutters things up.

template <class T, class... Args>
inline Handle ObjectPool::emplace(Args... args)
{
    // Find the pool that fits T
    using Pool = typename PoolCond1<T>::type;

    if (sizeof(T) <= OBJECT_SIZE_64)
    {
        return allocateConstruct<T, Pool>(args...);
    }
    else
    {
        std::stringstream message;
        message << typeid(T).name() << " is too large for ObjectPool. sizeof(" << typeid(T).name() << "): " << sizeof(T) << ".";

        throw std::length_error(message.str());
    }
}

You're doing perfect forwarding wrong.

First, the arguments have to be forwarding references. So Args&&....

Second, you have to actually forward them. So std::forward<Args>(args)..., not just args....

Once again, these errors can be caught at compile time, so this becomes:

template <class T, class... Args>
Handle ObjectPool::emplace(Args&&... args)
{
    using Pool = typename PoolCond1<T>::type; // does compile-time check

    return allocateConstruct<T, Pool>(std::forward<Args>(args)...);
}

And the same concepts apply to this:

template <class T>
inline T *ObjectPool::get(const Handle &handle)
{
    if (handle.size != sizeof(T))
    {
        std::stringstream message;
        message << "Type mismatch. HandleSize: " << handle.size << " != sizeof(T): " << sizeof(T) << ". typeid(T): " << typeid(T).name();

        throw std::invalid_argument(message.str());
    }
    else if (sizeof(T) <= OBJECT_SIZE_64)
    {
        return getObject<T>(handle);
    }
    else
    {
        std::stringstream message;
        message << "HandleSize (" << handle.size << ") too large for ObjectPool.";

        throw std::length_error(message.str());
    }
}

which becomes:

template <class T>
T* ObjectPool::get(Handle const& handle) const
{
    constexpr auto alignment = std::max(sizeof(T), alignof(T));

    static_assert(alignment <= OBJECT_SIZE_64);

    if (handle.size != alignment)
        throw type_mismatch<T>{handle};

    return getObject<T>(handle);
}

and so on for the other functions. As you can see, these functions get so small that it actually becomes feasible to simply define them right in the class. In any case, you don't need the inline specifier because they're templates.

Also get() seems like something that could/should be const.

Skip on down to reorder()... I'm not sure why you use a pointer here:

auto pool = &std::get<Pool>(_pools);

It only adds the hassle of having to use *, and you never reseat it, so why not use a reference?

Now here's where you start getting into trouble with the mutexes and locking. You lock the pool mutex in reorder()... then you call getIndex(), getObject(), and allocateMove(), the latter two of which also lock the pool mutex... and then allocateMove() also locks the hash table mutex while holding the pool mutex.

The danger is in allocateMove(). A deadlock happens, basically, when one function locks mutexes A then B while another locks mutexes B then A. So any time you want to lock two mutexes, you should be paranoid. You may think that all it takes is programming discipline to make sure you always lock A then B in order... but that's naïve because the compiler is free to reorder instructions.

Any time you lock more than one mutex, you should always use std::lock() or std::scoped_lock and lock them all together. As of C++17, you can probably use std::scoped_lock pretty much universally - forget all about std::lock_guard.

The first issue you need to avoid is recursive locking. That's just a matter of programming discipline. For example, you could work with the rule to only lock in public methods, and never call one public method from another. Or you could name all functions that require locks <something something>_locked_() (like getIndex_locked_()), and insist that once a function acquires a lock it can only call *_locked_() functions, and *_locked_() functions can only call other *_locked_() functions. Since you're dealing with multiple locks (unwise!), you may have to use more complex function names to keep track of what's locked and what's not.

A lot of this complexity could be alleviated if you used proper types for your pools, rather than tuples.

So let's get practical. Here's the sitch with regards to reorder(): You know reorder() needs to mess with both the pool and the handle hashtable. You have two options:

  1. take all the locks at once
  2. lock things only when needed, making sure to never try to lock something while something else is already locked

Both are valid strategies.

Option 1 is simpler.

template <class T>
inline Handle ObjectPool::reorder(const Handle &handle)
{
    using Pool = typename PoolCond1<T>::type;

    if (handle.size != sizeof(T))
        throw type_mismatch<T>{handle};

    auto& pool = std::get<Pool>(_pools);

    // All locking is done here.
    auto lock = std::scoped_lock{std::get<2>(pool), _hashMapMutex};

    // Do everything with the pool and hashtable here. None of the
    // private functions called are allowed to do any locking.
}

Option 2 is more efficient... but that only helps if it doesn't introduce errors. Is it safe to first allocate the object then release the pool lock, the lock and update the hash map separately? Probably not, because you're checking for a space and then filling it - if you give up the lock, someone else may fill it instead.

But it's not that big a deal here, because you bail early if you're not going to be doing any reordering. It's a bummer to lock the hashtable unnecessarily in that case, but it least it passes quickly.

Before I show what that might look like, there's one more thing about reorder() that I think needs improvement. After you get the lock, you check the index, and return immediately if there's no need to reorder. That's good. But then you immediately create a T object. Why? You don't need it yet. In fact, you may not need it at all! You won't need it until getObject(). On top of that, you call getObject() twice, unnecessarily.

So the whole bottom half of reorder() needs to be restructured to not create any objects unless necessary.

Put altogether:

template <class T>
Handle ObjectPool::reorder(Handle const& handle)
{
    using Pool = typename PoolCond1<T>::type;

    if (handle.size != sizeof(T))
        throw type_mismatch<T>{handle};

    auto& pool = std::get<Pool>(_pools);

    auto lock = std::scoped_lock{std::get<2>(pool), _hashMapMutex};

    // Notice I've flipped the condition
    if (getIndex<Pool>(std::get<0>(pool)) > handle.index)
    {
        if (auto p = getObject<T>(handle); p)
        {
            auto temp = std::move(*p);
            erase<T, Pool>(handle);
            return allocateMove<T, Pool>(std::move(temp));
        }
    }

    return {};
}

Next is capacity().... This could probably be greatly simplified and generalized with std::apply() and/or fold expressions.

Same for shrink().

On to the private methods....

template <class T, class Pool, class... Args>
inline Handle ObjectPool::allocateConstruct(Args... args)

template <class T, class Pool>
inline Handle ObjectPool::allocateMove(T &&object)

For allocateConstruct() see above for the comments on emplace() about perfect forwarding.

But the bigger problem here is that you've defined allocateConstruct() in terms of allocateMove(). That just seems bass-ackwards. This way you're not really doing a real emplace, and you can't support non-movable/non-copyable types. The right thing to do would be to do the emplace properly... and then allocateMove() becomes superfluous.

The other problem is that you do the move with std::memcpy... which is a terrible idea. It will only work for trivially copyable types, and you don't limit the pool to those types. If you tried to put a std::string in the pool, what would happen is that the memcpy would copy the guts of object into the new location... then object would be destroyed when it goes out of scope, which would free the string's memory... so what's left in the pool is now a zombie string pointing to deallocated memory.

What you want to be doing here is placement new.

With all that, allocateConstruct() would now look something like this:

template <class T, class Pool, class... Args>
Handle ObjectPool::allocateConstruct(Args&&... args)
{
    auto& pool = std::get<Pool>(_pools);

    // No locking! That should be done at a higher level.

    // This code would be a lot more clear if one could write
    //     pool.list
    // rather than
    //     std::get<1>(pool)
    // and so on.
    auto totalPositions = std::get<1>(pool).size() * OBJECT_POOL_PAGE_LENGTH;
    if (totalPositions == 0 || totalPositions <= std::get<0>(pool)->index)
    {
        addPage<Pool>();
    }

    auto curFree = std::get<0>(pool);
    auto nextFree = getPointer<Handle, Pool>(*curFree);

    // NO MORE MEMCPY!
    new (curFree) T{std::forward<Args>(args)...};

    std::get<0>(pool) = nextFree;

    Handle h{ HandleSize{ sizeof(T) }, HandleIndex{ getIndex<Pool>(curFree) }, false };

    _hashMap[h] = static_cast<void *>(curFree);

    return h;
}

And allocateMove() can simply forward to allocateConstruct() or be removed completely.

I think that's about enough for the review, because from this point on it's pretty much repeating the same points. The one additional thing to note is that in erase():

if (std::is_destructible<T>::value && !std::is_trivially_destructible<T>::value)
    reinterpret_cast<T *>(ptrToRemove)->~T();

std::memset(ptrToRemove, 0, std::get<0>(*pool)->size);

can pretty much reduce to:

std::destroy_at(reinterpret_cast<T*>(ptrToRemove));

This will boil down to a no-op for trivially destructible types, but if you're really paranoid you can make it:

// No need to check std::is_destructible<T>, at least not *here*.
// T *HAS* to be destructible... because you're destroying it!
if constexpr (!std::is_trivially_destructible_v<T>)
    std::destroy_at(reinterpret_cast<T*>(ptrToRemove));

The memset seems unnecessary.


pragma once

#pragma once is supported on most C++17 compilers...

Include guards are supported on all C++17 compilers, even on the strictest conformance settings.

... I don't intend to have multiple copies of the same file.

That's like saying "I don't need to worry about smart pointers, because I don't intend to leak memory".

Whatever you may intend, it is not hard to get into the situation where you have multiple copies of a header. You make a shared library of useful utility functions, then you make an app that makes use of that library... you want to use your object pool in the app, but uh oh, the library was using it too, and now you have two copies of the header - one in the library code base, one in the app code base.

Even if you are the most perfect programmer who ever lived, and would never create a situation where the same header is in two different places, it could still very easily happen. Some other programmer might have found the header useful and included in their code. And even if you're the only programmer who will ever use the file, and you're perfect, you could still find yourself with multiple copies of the file, because of something your build system does, or maybe your revision control system, package manager, or snap or whatever else.

So whatever your intentions, it's a possibility. And if it happens, #pragma once is not guaranteed to work. I see no benefit in using a non-standard extension that doesn't work instead of a standard construct that does. Just use include guards; it's the standard, portable solution.

Alignment problems

Your test case appeared to work because you didn't actually test the problem. Bar is supposed to be 64-bit aligned. You never actually tested whether it was. And it isn't. See here: http://cpp.sh/8w5ti (Note, the function push() there should really be called emplace().)

Normally a type's size is >= its alignment. But it is possible to create types whose alignment is >= their size. If you try to put those objects in one of your pools, it will "work", but you're in undefined behaviour territory. Who knows what will happen next: an access violation or corrupted memory are both likely possibilities.

Using sizeof(T) will work for most objects. But to be really safe, you should probably use max(sizeof(T), alignof(T)). I have edited the notes above to take into account every edge case I can think of.

Forward ref problems

I am just guessing, but what I suspect you're doing is calling Foo f; pool.push<Foo>(f); or something like that. When you make the call like that, you are preventing template argument deduction and losing the benefit of reference collapsing. Just do Foo f; pool.push(f);. That will work in every case.

See for yourself: http://cpp.sh/8pdxy

As you can see, it works for every type of value I throw at it - lvalues, lvalue refs, rvalue refs, even const rvalue refs (which are weird). And I deliberately made it a member template, just like your push() functions. (I probably should have named it push() instead of bar(). Oh well.)

Being able to call pool.push(t); rather than pool.push<T>(t); is the whole point of having a push() function. If you're only ever going to call push<T>(t);, then you might as well call emplace<T>(t); and delete the push() functions completely.

(You still need to call emplace() like pool.emplace<Foo>(arg1, arg2, etc);, because emplace() can't deduce T from the arguments like push() can.)

\$\endgroup\$
5
  • \$\begingroup\$ #pragma once is supported on most C++17 compilers and I don't intend to have multiple copies of the same file. Which is why I kept it. On alignof vs sizeof: I'm confused about what the problem is. The intent of AlignedArray is for itself to be aligned (i.e. to 64 byte boundaries) but for it's contents to be managed externally (i.e. by the ObjectPool). I tried your example and it didn't cause any problems. \$\endgroup\$ Jun 9, 2018 at 14:09
  • \$\begingroup\$ Regarding "Why have the T const& overload? T&& is a forwarding reference. It will work for everything: lvalues, rvalues, everything.": That doesn't appear to be the case, at least not on MSVC++. It fails to compile saying it can't convert T to T&&. \$\endgroup\$ Jun 9, 2018 at 15:23
  • 1
    \$\begingroup\$ I've added answers to your issues at the end of the answer. \$\endgroup\$
    – indi
    Jun 12, 2018 at 1:28
  • \$\begingroup\$ Alignment problems: From what I understand sizeof(T) is always a multiple of alignof(T) and placement new assumes you're passing it an aligned address. See [stackoverflow.com/a/4638295/1249634](here) and [stackoverflow.com/a/11782277/1249634](here). I also tested it in GCC, Clang and MSVC. Adding next += sizeof(T) - reinterpret_cast<std::size_t>(next) % sizeof(T); to the top of push fixed it. Forward ref problems: I get it now. When passing a const ref T resolves to const T& and since I'm using memcpy that's perfectly legal for const values. \$\endgroup\$ Jun 12, 2018 at 13:26
  • \$\begingroup\$ It is possible to create types where alignof(T) > sizeof(T): stackoverflow.com/q/46457449 \$\endgroup\$
    – indi
    Jun 12, 2018 at 21:53
0
\$\begingroup\$

One cardinal sin is

#define OBJECT_SIZE_2 sizeof(std::size_t) * 2

Don't use #define for constants or "functions" (⧺ES.31).

I would suggest going over the document I linked above, to learn much more.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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