Sign in to follow this  
GanonPhantom

My ThreadPool

Recommended Posts

Im currently working on my threadpool for a library im making... I have a fairly nice base design, but it feels quite hackish. What I want to be able to do, is spawn new threads when the Task lists gains to size X (20 being X in this case). The design i have now works. Im able to sit in a continous while loop and push tasks into the pool and they will be correctly handled. But, once again, i feel the design in quite hackish. Any suggestions?

#ifndef H_THREADPOOL_H
#define H_THREADPOOL_H

#include "Thread.h"
#include <list>
#include "Mutex.h"
#include <boost/bind.hpp>
#include <boost/function.hpp>

class ThreadPool {
	public:
		/**
		* Constructor
		*/
		ThreadPool() {
			m_Mutex = new Mutex();
			NewThread();
		};

		//==============================================================================

		/**
		* Destructor
		*/
		~ThreadPool() { 
			
		};

		//==============================================================================

		/**
		* Adds an event to the pool.
		*/
		void QueueTask(boost::function<void()> _Callback) {
			m_Mutex->AquireLock();
			m_Tasks.push_back(_Callback);

			if(m_Tasks.size() > 20 && m_Threads.size() < 300)
				NewThread();
			m_Mutex->ReleaseLock();
		};

		//===============================================================================

	protected:

	private:

		list<boost::function<void()>>						m_Tasks;
		Mutex*												m_Mutex;
		list<IThread*>										m_Threads;

		//===============================================================================

		/**
		* Worker thread.
		*/
		void WorkerThread(IThread*	_This) {
			while(true) {


				m_Mutex->AquireLock();

				if(m_Tasks.size() > 0) {
					m_Tasks.front()();
					m_Tasks.pop_front();
				}

				printf("TaskCounter[%d] \n",m_Tasks.size());
				m_Mutex->ReleaseLock();


				wait(1);
			}
				
		};

		//===============================================================================

		/**
		* Adds a new thread to the Pool.
		*/
		void NewThread() {
			m_Mutex->AquireLock();
			IThread* _Thread = 0;
			_Thread = new Thread<ThreadPool,IThread*>(this, _Thread, &ThreadPool::WorkerThread);
			_Thread->Execute();
			m_Threads.push_back(_Thread);
			m_Mutex->ReleaseLock();
		};

		//===============================================================================

};


#endif

Share this post


Link to post
Share on other sites
Quote:
Original post by arthurprs
use Critical Section instead of mutex, simpler and faster
For a critical section as small as this one, it might actually be beneficial to write your own mutex using InterlockedCompareExchange (or direct assembly on non-windows platforms). These are often called a fast userspace mutex (or futex) as a regular mutex is executed in kernel mode, which can be expensive.


Also, the way your code is constructed, it seems that only one task can be executed at a time because you're executing tasks while still holding the lock:
	m_Mutex->AquireLock();//lock tasks
if(m_Tasks.size() > 0) {
m_Tasks.front()();//execute task
m_Tasks.pop_front();
}
m_Mutex->ReleaseLock();//unlock tasks

Try something like this:
	boost::function<void()> task;
bool popped = false;
m_Mutex->AquireLock();//lock tasks
if(m_Tasks.size() > 0) {
task = m_Tasks.front();//pop task
m_Tasks.pop_front(); // from list
popped = true;
}
m_Mutex->ReleaseLock();//unlock tasks
if( popped )
task();


[EDIT]
Also, you have the ability for the thread pool to grow, but not to shrink.
Perhaps a mechanism for killing off idle threads would be beneficial:
void WorkerThread(IThread*	_This) {
int idleCount = 0;
while(idleCount < 42) {
...
if( popped )
{
task();
idleCount = 0;
}
else
idleCount++;
}
KillThread(_This);//remove _This from m_Threads
}

Share this post


Link to post
Share on other sites
Quote:
Original post by Hodgman
Quote:
Original post by arthurprs
use Critical Section instead of mutex, simpler and faster
For a critical section as small as this one, it might actually be beneficial to write your own mutex using InterlockedCompareExchange (or direct assembly on non-windows platforms). These are often called a fast userspace mutex (or futex) as a regular mutex is executed in kernel mode, which can be expensive.


Also, the way your code is constructed, it seems that only one task can be executed at a time because you're executing tasks while still holding the lock:
*** Source Snippet Removed ***
Try something like this:
*** Source Snippet Removed ***

[EDIT]
Also, you have the ability for the thread pool to grow, but not to shrink.
Perhaps a mechanism for killing off idle threads would be beneficial:
*** Source Snippet Removed ***


Thanks a bunch :), ill take all that into account and impliment it.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this