5
\$\begingroup\$

How could this template class ThreadPool be improved?

I use Boost queue to keep std::function's numbers from std::array(simple hashing) from which I can later retrieve the std::function's objects to execute them with operator(). It's not possible to store them directly in boost::lockfree::queue (Stored Object Type must have non-trivial destructor).

That's why I emulate primitive lockfree container with std::array<std::pair<int , ...>. The GuarderArray template class lacks garbage collector.

#include <iostream>
#include <thread>
#include <mutex>
#include <memory>
#include <atomic>
#include <functional>
#include <boost/lockfree/queue.hpp>
#include <array>


using std::cout ; using std::cin; using std::endl;
using std::mutex; using std::atomic;
using std::array; using std::function;

template <int NN> using guardedArray =  std::array<std::pair<std::atomic<bool>,std::function<void()>>, NN> ;

template <typename T> using lockfreequeue = boost::lockfree::queue<T,boost::lockfree::capacity<1000>>;

class Functor{
    int myNN;
public:
    Functor(int nn):myNN(nn){};
    Functor(std::function<void()> ff) : myff(ff){};
    std::function<void()> myff;
    Functor(const Functor&) = default;
    Functor operator=(const Functor&) ;
    ~Functor() = default;
    void operator()(){
    myff();

    }
};



template <int NN > class GuardedArray{
    guardedArray<NN> myGuardedArray; 
public:
    int insertObject(std::function<void()> ff){
    int kk ;
    bool empty;

    do{
    kk = rand () % NN;
    empty = myGuardedArray[kk].first;
    cout << "spining ... " << endl;


    }while(!(std::atomic_compare_exchange_weak(&myGuardedArray[kk].first,&empty,true)));
    myGuardedArray[kk].second = ff;
    return kk;
    };
    std::function<void()> getIndex(int ii){return myGuardedArray[ii].second  ; };
    std::thread garbageCollectorThread;



};

template <int NN > class ThreadPool{

private:
    std::thread tt[NN];
    GuardedArray<1000> myGA;
    bool endFlag;
public:
    ThreadPool():endFlag(false){

    for(int ii = 0 ; ii < NN ; ++ii){       
        std::thread localThread(&ThreadPool<NN>:: threadLoop, this);
        tt[ii].swap(localThread);

    }

    };
    void bookTask(std::function<void()> ff){
    int ii = myGA.insertObject(ff);
    mySync.push(ii);
    };

    bool demandedAccess[NN];
    int waitingProcess;
    ~ThreadPool() {endFlag = true; for (int ii = 0 ; ii < 10 ; ii++  ){  tt[ii].join();}   };
    int myWrapper();
    void threadLoop();
    lockfreequeue<int> mySync;

};

template <int NN> void ThreadPool<NN>::threadLoop(){
    std::thread::id kk  = std::this_thread::get_id();
    std::function<void()> fun ; 
    int ff;
    while(!endFlag){

    if(!mySync.empty()){        
        bool result = mySync.pop(ff);
        if(result){
        fun = myGA.getIndex(ff);        
        fun();
        }
    }
    }
}


int main(int argc, char** argv){

    ThreadPool<10> tp;
    Functor ff(( [](){ cout <<"a kuku  "<<endl;  }));
    tp.bookTask(ff);
    std::this_thread::sleep_for(std::chrono::seconds(20));
    return 0 ; 
}
\$\endgroup\$
5
  • \$\begingroup\$ Can you please at least add good indentation. Currently its nearly impossible to read. \$\endgroup\$ Apr 9, 2014 at 2:11
  • \$\begingroup\$ Seems everyone has his own >>good<< indentation language... \$\endgroup\$
    – user40334
    Apr 9, 2014 at 9:18
  • 1
    \$\begingroup\$ @user40334 On Code Review, you should not modify your code once posted. Otherwise, it invalidates the answers. \$\endgroup\$
    – Morwenn
    Apr 9, 2014 at 9:19
  • \$\begingroup\$ @user40334: There are a couple of acceptable indention patterns. Yours matches none of them. \$\endgroup\$ Apr 9, 2014 at 15:46
  • 1
    \$\begingroup\$ There were still some changes that invalidated my answer. You may still add the additional functionality that was not addressed, otherwise please keep everything else intact. \$\endgroup\$
    – Jamal
    Apr 9, 2014 at 19:49

1 Answer 1

3
\$\begingroup\$

Just some stylistic things that stick out to me:

  • I think it's a bit cumbersome to have all of this:

    using std::cout ; using std::cin; using std::endl;
    using std::mutex; using std::atomic;
    using std::array; using std::function;
    

    You're also still prefixing std:: in places, which defeats the purpose of having this. Since you could always end up using something not already in this list, you might as well keep prefixing std:: as you're doing already, and remove all of that above.

  • Some of your indentation and semicolon placing are inconsistent. In some places you indent or not, and in some places you have a space before the ending semicolon. Keep the indentation consistent (you should utilize it) and choose what to do with all of your semicolons.

  • It would be more readable and maintainable to place the template statements on separate lines from the class or function, especially if either statement is lengthy:

    template <typename T>
    class Class
    {
    };
    

    template <typename T>
    void function()
    {
    }
    
\$\endgroup\$
1
  • \$\begingroup\$ yeah code paste was little messy , I better auto-format this in editor. My initial bet was to use the std::move casting to prevent spourious cp ctors. \$\endgroup\$
    – user40334
    Apr 8, 2014 at 22:55

Your Answer

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

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