HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Dividing a ThreadPool class into component classes to reduce/isolate complexity

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
threadpoolintoreducedividingclassesisolatecomponentclasscomplexity

Problem

I have a ThreadPool class which has grown to a point where it is becoming difficult to keep all of this unit of work in mind. It is also becoming harder to read. For this reason I would like to break this class into component classes.

First of all here is the class header with comments:

```
//ThreadPool.h

#ifndef THREADPOOL_H
#define THREADPOOL_H

#define doNothing()

#include
#include
#include
#include
#include
#include
#include
#include

typedef std::function voidFunctionType;

class ThreadPool{

public:
ThreadPool(unsigned int desiredThreadCount);
~ThreadPool();

//Functions Affecting ThreadPool
//Function used by threads to submit work to ThreadPool
void workSubmit(voidFunctionType functionPointer);
//Function used by threads to take work from ThreadPool
voidFunctionType takeTask(void);
//Function which returns the number of work in the queue
unsigned int workCount(void);
//Function which
bool isWorkAvailable(void);

//Functions Affecting Thread Availability.
//Increments availableThreads variable which indicates number of threads available
void incThreadAvailability(void);
//Decrements availableThreads variable which indicates number of treads available
void decThreadAvailability(void);
//Returns availableThreads variable which indicates number of threads available
unsigned int getThreadAvailability(void);
//Function returns total number of threads generated
unsigned int getThreadNum(void);

//Functions Affecting Threads
//Checks to see if work is available, if work is not available thread is put to sleep
//If work is available thread processes work.
void threadSleepUntillWorkAvail(void);

private:
// Number of threads generated
unsigned int numberOfThreads;

//Vector of threads
std::vector workers;

// Number of threads available to process work
unsigned int availableThreads;
// Mutex for the availableThreads variable
std::mutex threadAvailabilityLock;
std::unique_lock threadA

Solution

There are way too many member variables:

unsigned int numberOfThreads;
std::vector workers;
unsigned int availableThreads;


Can you not find the number of threads with workers.size(). What's the difference between number of Threads and Available Threads and more importantly why do you need to know!

You have three mutex's and two condition variables! Why? Also why are you putting the unique_lock as a member of the class! That would make them hard to use in a shared context.

Normally I would expect to see:

//In class:
    std::mutex               lock;
    std::condition_variable  sync;

//Adding Stuff Function:
    std::lock_guard   guard(lock);
    queue.push(item);
    sync.notify();

//Taking stuff from Queue
    std::lock_guard   guard(lock);
    sync.wait(guard, [queue](){!queue.empty();});
    if (!queue.empty()) {
        result = queue.pop();
    }


The only method that should be public on that class is adding a new job. All other methods are private members.

This is what mine would look like this:

class WorkQueue
{
    private:
        std::vector            threads;
        std::deque>   queue;
        std::mutex                          lock;
        std::condition_variable             sync;
        bool                                finished;

        std::function getWork();
        void work();
    public:
        WorkQueue(int threadCount);
        ~WorkQueue();
        template
        void addWork(F&& func);
};

Code Snippets

unsigned int numberOfThreads;
std::vector<std::thread> workers;
unsigned int availableThreads;
//In class:
    std::mutex               lock;
    std::condition_variable  sync;

//Adding Stuff Function:
    std::lock_guard   guard(lock);
    queue.push(item);
    sync.notify();

//Taking stuff from Queue
    std::lock_guard   guard(lock);
    sync.wait(guard, [queue](){!queue.empty();});
    if (!queue.empty()) {
        result = queue.pop();
    }
class WorkQueue
{
    private:
        std::vector<std::thread>            threads;
        std::deque<std::function<void()>>   queue;
        std::mutex                          lock;
        std::condition_variable             sync;
        bool                                finished;

        std::function<void()> getWork();
        void work();
    public:
        WorkQueue(int threadCount);
        ~WorkQueue();
        template<typename F>
        void addWork(F&& func);
};

Context

StackExchange Code Review Q#149791, answer score: 2

Revisions (0)

No revisions yet.