patterncppMinor
Dividing a ThreadPool class into component classes to reduce/isolate complexity
Viewed 0 times
threadpoolintoreducedividingclassesisolatecomponentclasscomplexity
Problem
I have a
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
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:
Can you not find the number of threads with
You have three mutex's and two condition variables! Why? Also why are you putting the
Normally I would expect to see:
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:
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.