5
\$\begingroup\$

I am studying computer science and just for the sake of practise I thought of implementing an object pool.

This is my first implementation, it is simple and works correctly, but I would love any thoughts on it. Currently, the memory is allocated on the stack, but I am thinking of using policy based design to allow heap allocation as well.

Here is the code of the pool:

#include <iostream>
#include <memory>
#include <functional>

/**
 * Generic class for object pools.
 */
template <class T, int SZ, class Initialiser, class Releaser> class StackObjectPool {
public:
    using pointer = std::unique_ptr<T,std::function<void(T*)> >;
    using PoolType = StackObjectPool<T,SZ,Initialiser, Releaser>;

    StackObjectPool() {}
    StackObjectPool(const PoolType& orig) = delete;
    ~StackObjectPool() {}

    /**
     * Acquires an object not being currently used
     * @return pointer to the acquired object
     * @throw std::out_of_range if all the objects inside the pool are being used
     */
    pointer acquire() {
        unsigned int index = 0; // look for the first free object
        while(m_occupied_registry[index]) ++index;
        if(index >= SZ) throw std::out_of_range("Pool exceeded its size");
        m_occupied_registry[index] = true; // mark it as currently in use
        m_initialiser(&m_objects[index]); // initialise it

        //return an unique_ptr that calls release when reset
        return pointer(&m_objects[index], [this](T* element)->void{release(element);});
    }

    friend std::ostream& operator<<(std::ostream& ostream, const PoolType& pool) {
        for(unsigned int index = 0; index < SZ; ++index) {
            std::cout<<(pool.m_occupied_registry[index]?"[occupied] ":"[free]\n");
            if(pool.m_occupied_registry[index]) {
                std::cout<<pool.m_objects[index]<<"\n";
            }
        }
        return ostream;
    }
private:

    void release(T* element) {
        unsigned int index = 0;
        while(&m_objects[index] != element) ++index;
        m_occupied_registry[index] = false; // mark the released element as free
        m_releaser(element); // call release functor
    }

    Initialiser m_initialiser;
    Releaser m_releaser;
    bool m_occupied_registry[SZ] {0};
    T m_objects[SZ];
};

And here is a Test class and a simple main which I have used to test the pool:

class Test {
public:
    Test():m_a(0),m_b('0'){
        std::cout<<"Construct default Test"<<*this<<"\n";
    }
    Test(int a, char b): m_a(a), m_b(b) { 
        std::cout<<"Construct Test"<<*this<<"\n";
    }
    Test(const Test& orig): m_a(orig.m_a),m_b(orig.m_b) {
        std::cout<<"Construct cpy Test"<<*this<<"\n";
    }
    ~Test() {
        std::cout<<"Delete Test"<<*this<<"\n";
    }
    friend std::ostream& operator<<(std::ostream& ostream, const Test& test) {
        ostream<<"["<<test.m_a<<",'"<<test.m_b<<"']";
        return ostream;
    }
private:
    int m_a;
    char m_b;
};

struct Initialiser {
    void operator() (Test* element) {
        std::cout<<"Initialising Test"<<*element<<"\n";
    }
};

struct Releaser {
    void operator() (Test* element) {
        std::cout<<"Releasing Test"<<*element<<"\n";
    }
};

int main(int argc, char** argv) {
    using TestPool = StackObjectPool<Test,5, Initialiser, Releaser>;
    TestPool pool;
    TestPool::pointer pt1, pt2,pt3,pt4,pt5;

    std::cout<<pool;

    pt1= pool.acquire();
    std::cout<<"Aquired pt:"<<*pt1<<"\n";
    //pt1.reset();

    std::cout<<pool;

    pt2 = pool.acquire();
    std::cout<<"Aquired pt:"<<*pt2<<"\n";
    pt2.reset();

    std::cout<<pool;

    pt3 = pool.acquire();
    std::cout<<"Aquired pt:"<<*pt3<<"\n";
    //pt3.reset();

    std::cout<<pool;
    return 0;
}
\$\endgroup\$
2
\$\begingroup\$

the implementation itself already feels comfortable. It might not say much, though, I find it important that code uses a single style. Congrats!

That said, every code has a few remarks:

  • Why would you name your template argument SZ? Wouldn't MaxSize be a better name?
  • Would it not be more interesting to make the type of SZ unsigned as a negative size will be incorrect
  • Should this class be final?
  • You already use =delete, why don't you use =default for the Ctor and Dtor?
  • I would use a subclass to represent the deletor of the unique_ptr as std::function has a performance overhead
  • In the streaming operator, why do you use std::cout instead of ostream
  • Try to explain what your code should do, not the c++ concepts you are using

It's also nice to have a test on this code, though, it feels a bit lacking:

  • Why don't you try to acquire more than 5 elements?
  • The Initializer and Releaser don't represent real-case usages

So in conclusion: This code ain't bad

\$\endgroup\$
  • \$\begingroup\$ Thanks for the tips! I agree with you on everything, so I will update the improved code in the answer. Thanks for helping. \$\endgroup\$ – amc176 Jan 13 '17 at 0:00
4
\$\begingroup\$

You might change:

void release(T* element, unsigned int index) {
    if (&m_objects[index] != element) {
            std::cerr << "no match index and element\n";
            return; // or exit(1);
    }
    m_occupied_registry[index] = false; // mark the released element as free
    m_releaser(element); // call release functor
}

and at the acquire change the return line:

    return pointer(&m_objects[index], [this,index](T* element)->void{release(element,index);});

The above will improve the efficiency of the release operation, as you don't need to search for the element.

\$\endgroup\$
  • \$\begingroup\$ Thanks! Reading the code again I just realised that there is no need to pass the index as an argument, as it can be easily calculated with some pointer arithmetic: index = element - m_objects; \$\endgroup\$ – amc176 Sep 18 '17 at 8:00

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.