• Advertisement
Sign in to follow this  

My ThreadPool

This topic is 3472 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

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
Advertisement
That mutex class is a wrapper i made for Critical Section. So when i call AquireLock, its actually calling EnterCriticalSection(), and ReleaseLoc() calls LeaveCriticalSection().

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
Sign in to follow this  

  • Advertisement