pool: correctly forward constructor parameters

pool.hpp

@@ -20,6 +20,7 @@
 #include "nocopy.hpp"
 #include "debug.hpp"
 
+#include <atomic>
 #include <cstdlib>
 #include <cstdint>
 
@@ -29,35 +30,48 @@ namespace util {
     /// non-POD types can be stored, but there are no guarantees for calling
     /// item destructors at pool destruction time.
     template <typename T>
-    class pool : public nocopy {
+    class pool {
     protected:
+        union node;
+
         union alignas (T) node {
-            node *_node;
-            char _data[sizeof (T)];
+            std::atomic<node*> next;
+            char data[sizeof (T)];
         };
 
-        node *m_head; // root address of allocation
-        node *m_next; // next available entry in the linked list
+        static_assert (std::atomic<node*>::is_always_lock_free);
 
+        // root address of allocation. used in deletion at destruction time.
+        node* m_head;
+
+        // the next available entry in the linked list
+        std::atomic<node *> m_next;
+
+        // the total number of items that could be stored
         const size_t m_capacity;
-        size_t m_size;
+
+        // the number of items currently stored.
+        std::atomic<size_t> m_size;
 
     public:
+        pool (const pool&) = delete;
+        pool& operator= (const pool&) = delete;
+
+        pool (pool&&);
+        pool& operator= (pool&&);
+
         explicit
         pool (unsigned int _capacity):
             m_capacity (_capacity),
             m_size (0u)
         {
-            static_assert (sizeof (T) >= sizeof (uintptr_t),
-                           "pool<T>'s chained block system requires that T be at least pointer sized");
-
             // allocate the memory and note the base address for deletion in destructor
-            m_next = m_head = new node[m_capacity]; // static_cast<node *> (operator new (sizeof (T) * m_capacity));
+            m_next = m_head = new node[m_capacity];
 
-            // initialise the linked list of nodes
-            for (unsigned int i = 0; i < m_capacity - 1; ++i)
-                m_next[i]._node = m_next + i + 1;
-            m_next[m_capacity - 1]._node = nullptr;
+            // build out a complete singly linked list from all the nodes.
+            for (size_t i = 0; i < m_capacity - 1; ++i)
+                m_next[i].next = m_next + i + 1;
+            m_next[m_capacity - 1].next = nullptr;
         }
 
         ~pool ()
@@ -68,69 +82,89 @@ namespace util {
         }
 
         // Data management
-        template <typename ...Args>
-        T*   acquire (Args&... args)
+        [[nodiscard]] T*
+        allocate (void)
         {
             // double check we have enough capacity left
             if (!m_next)
                 throw std::bad_alloc ();
             CHECK_LT (m_size, m_capacity);
 
-            // save what will become the next node shortly. it could be overwritten
-            // in the constructor we're about to call.
-            node *newnext = m_next->_node;
-            T *data = reinterpret_cast<T*> (m_next);
+            // unlink the current cursor
+            do {
+                node* curr = m_next;
+                node* soon = curr->next;
 
-            // try to construct the returnable object.
+                if (m_next.compare_exchange_weak (curr, soon)) {
+                    ++m_size;
+                    return reinterpret_cast<T*> (curr);
+                }
+            } while (1);
+        }
+
+        void
+        deallocate (T *base)
+        {
+            auto soon = reinterpret_cast<node*> (base);
+
+            do {
+                node *curr = m_next;
+                soon->next = curr;
+                if (m_next.compare_exchange_weak (curr, soon)) {
+                    --m_size;
+                    return;
+                }
+            } while (1);
+        }
+
+
+        template <typename ...Args>
+        T*
+        construct (Args &&...args)
+        {
+            auto ptr = allocate ();
             try {
-                new (data) T (args...);
+                return new (ptr) T (std::forward<Args> (args)...);
             } catch (...) {
-                // the constructor may have overwritten the node linkages before
-                // throwing. fix this up before forwarding the exception.
-                m_next->_node = newnext;
+                deallocate (ptr);
                 throw;
             }
-
-            // the object is valid. save the new linked list head and bump the
-            // stats for availability.
-            m_next  = newnext;
-            m_size++;
-
-            return data;
         }
 
-        void release (T *data)
+
+        void
+        destroy (T *ptr)
         {
-            CHECK_NEZ (m_size);
-
-            data->~T();
-            node *newnode = reinterpret_cast<node *> (data);
-
-            newnode->_node = m_next;
-            m_next = newnode;
-            m_size--;
+            ptr->~T ();
+            deallocate (ptr);
         }
 
+
         size_t capacity (void) const
         {
             return m_capacity;
         }
+
+
         size_t size (void) const
         {
             return m_size;
         }
+
+
         bool empty (void) const
         {
             return m_size == m_capacity;
         }
 
+
         // Indexing
         size_t index (const T*) const;
 
         T& operator[] (size_t idx) &;
+
         const T& operator[] (size_t idx) const&;
     };
 }
 
-
 #endif

test/pool.cpp

@@ -14,7 +14,7 @@ check_single (util::TAP::logger &tap)
     // Ensure a single element doesn't break the circular linked list
     util::pool<uint64_t> single(1);
     tap.expect_nothrow ([&] {
-        single.release (single.acquire ());
+        single.deallocate (single.allocate ());
     }, "single element acquire-release");
 }
 
@@ -28,19 +28,19 @@ check_unique_ptr (util::TAP::logger &tap)
 
     // Take all pointers out, checking they are unique, then replace for destruction.
     while (!uintpool.empty ())
-        uintset.insert (uintpool.acquire ());
+        uintset.insert (uintpool.allocate ());
 
     tap.expect_eq (uintset.size (), uintpool.capacity (), "extracted maximum elements");
 
     for (auto i: uintset)
-        uintpool.release (i);
+        uintpool.deallocate (i);
 
     tap.expect_eq (uintpool.size (), 0u, "re-inserted maximum elements");
     uintset.clear ();
 
     // Do the above one more time to ensure that releasing works right
     while (!uintpool.empty ())
-        uintset.insert (uintpool.acquire ());
+        uintset.insert (uintpool.allocate ());
     tap.expect_eq (uintset.size (), uintpool.capacity (), "re-extracted maximum elements");
 }
 
@@ -56,7 +56,7 @@ check_keep_value (util::TAP::logger &tap)
 
     // Give every item a unique value
     for (unsigned int i = 0; i < uintpool.capacity (); ++i) {
-        uint64_t *item = uintpool.acquire ();
+        uint64_t *item = uintpool.allocate ();
         *item = i;
 
         uintvector.push_back(item);
@@ -83,6 +83,42 @@ check_keep_value (util::TAP::logger &tap)
 }
 
 
+//-----------------------------------------------------------------------------
+void
+check_keep_variadic_value (util::TAP::logger &tap)
+{
+    struct foo_t {
+        foo_t (uint64_t _a, uint64_t _b, uint64_t _c, uint64_t _d):
+            a (_a),
+            b (_b),
+            c (_c),
+            d (_d)
+        { ; }
+
+        uint64_t a, b, c, d;
+    };
+
+    util::pool<foo_t> alloc (512);
+
+    bool success = true;
+
+    for (uint64_t a = 0; a < 8; ++a)
+        for (uint64_t b = 0; b < 8; ++b)
+            for (uint64_t c = 0; c < 8; ++c) {
+                uint64_t d = (a << 24) | (b << 16) | (c << 8);
+                auto ptr = alloc.construct (a, b, c, d);
+
+                if (ptr->a != a || ptr->b != b || ptr->c != c || ptr->d != d) {
+                    success = false;
+                    goto done;
+                }
+            }
+
+done:
+    tap.expect (success, "variadiac construction retains values");
+}
+
+
 //-----------------------------------------------------------------------------
 int
 main (int, char **)
@@ -92,6 +128,7 @@ main (int, char **)
     check_single (tap);
     check_unique_ptr (tap);
     check_keep_value (tap);
+    check_keep_variadic_value (tap);
 
     return tap.status ();
 }