Skip to main content
gamedev.net
🔒 Locked

My ThreadPool

Started by GanonPhantom 3 replies 1.2k views
Original Post
GanonPhantom
GanonPhantom
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
arthurprs
arthurprs
use Critical Section instead of mutex, simpler and faster
GanonPhantom
GanonPhantom
That mutex class is a wrapper i made for Critical Section. So when i call AquireLock, its actually calling EnterCriticalSection(), and ReleaseLoc() calls LeaveCriticalSection().
Hodgman
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:
	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}
GanonPhantom
GanonPhantom
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.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.