Return value or Exception or something else ?
-
Mr.Brainley wrote:
I don't want to use a pointer for return.
Why not? I also question why INFINITE is hard coded? Does not seem appropriate for thread pooling. Also does your pool provide a timeout mechanism to account for user code hogging and hanging threads?
Mr.Brainley wrote:
Any other ideas
A smart pointer implementation that would encapsulate returning the thread to the pool etc.
led mike
I think my thread pool works different from what you think. Basically, i have a central semaphore that is increased by one each time a Job is added. On that, a thread wakes up and asks the queue for a job. The t_Job type of my example is a simple class containing a pointer to an interface with a function to execute, and state-information. The INFINITE wait here is just for the mutex on the jobqueue, wich of course needs to be synchronized. The pool does not have a timeout mechanism. I don't think it is neccessary, since i write it for a very special purpose, but anyhow i would'nt know how to implement that. Can you give me a hint there ? The main idea was, that whenever is thread is wakened by the semaphore, that means that there actually IS a job. But in Win32 you have to give a maximum count for a semaphore. Since i don't want to limit the number of jobs in my queue, i tried to find another solution, wich brought me to the problem at hand. I think i will just live with the limit (can set it high). The maximum workloud can be estimated and that will have to suffice.
-
This is my function :
CThreadPool::t_Job& CThreadPool::CJobQueue::getJob()
{
WaitForSingleObject(m_JobListMutex, INFINITE);for ( std::deque::iterator it = m\_JobList.begin(); it != m\_JobList.end(); it++ ) if ( it->m\_JobState != JOB\_WORKING) { it->m\_JobState = JOB\_WORKING; ReleaseMutex(m\_JobListMutex); return (\*it); } ReleaseMutex(m\_JobListMutex); // Need to do something here to indicate no item has been found}
I don't want to use a pointer for return. And i don't think it's apropriate to throw an exception, since that scenario is unavoidable, in fact quite regular. My only Idea left would be to return some special reference to a static element that functions as sort of a NULL. Any other ideas/comments ?
I see now that you are opposing the use of pointers.. why? Pointers are an intrinsic concept in C++ - why work against it? :~
-- Presented in doublevision (where drunk)
-
This is my function :
CThreadPool::t_Job& CThreadPool::CJobQueue::getJob()
{
WaitForSingleObject(m_JobListMutex, INFINITE);for ( std::deque::iterator it = m\_JobList.begin(); it != m\_JobList.end(); it++ ) if ( it->m\_JobState != JOB\_WORKING) { it->m\_JobState = JOB\_WORKING; ReleaseMutex(m\_JobListMutex); return (\*it); } ReleaseMutex(m\_JobListMutex); // Need to do something here to indicate no item has been found}
I don't want to use a pointer for return. And i don't think it's apropriate to throw an exception, since that scenario is unavoidable, in fact quite regular. My only Idea left would be to return some special reference to a static element that functions as sort of a NULL. Any other ideas/comments ?
Because pointers are evil. Check this FAQ for more on that topic. Also, i couldn't get it to work. Can't cast the iterator to a pointer. No idea how it can be done.
-
I think my thread pool works different from what you think. Basically, i have a central semaphore that is increased by one each time a Job is added. On that, a thread wakes up and asks the queue for a job. The t_Job type of my example is a simple class containing a pointer to an interface with a function to execute, and state-information. The INFINITE wait here is just for the mutex on the jobqueue, wich of course needs to be synchronized. The pool does not have a timeout mechanism. I don't think it is neccessary, since i write it for a very special purpose, but anyhow i would'nt know how to implement that. Can you give me a hint there ? The main idea was, that whenever is thread is wakened by the semaphore, that means that there actually IS a job. But in Win32 you have to give a maximum count for a semaphore. Since i don't want to limit the number of jobs in my queue, i tried to find another solution, wich brought me to the problem at hand. I think i will just live with the limit (can set it high). The maximum workloud can be estimated and that will have to suffice.
Have you ever used an IO completion port? It can be used for what you are doing without the semaphore release count limit. Instead of IO you would use PostQueuedCompletionStatus() to post jobs and your pool of threads wait for jobs using GetQueuedCompletionStatus(). When a waiting thread gets a completion packet it already has the job (pointer) so it doen't need to retrieve it from a list. It works pretty efficiently. Just a thought. Mark
-
I see now that you are opposing the use of pointers.. why? Pointers are an intrinsic concept in C++ - why work against it? :~
-- Presented in doublevision (where drunk)
Joergen Sigvardsson wrote:
Pointers are an intrinsic concept in C++
Actually, pointers are an intrisic concept in C. C++ has references, and that has a reason. I try to follow that guideline.
-
Have you ever used an IO completion port? It can be used for what you are doing without the semaphore release count limit. Instead of IO you would use PostQueuedCompletionStatus() to post jobs and your pool of threads wait for jobs using GetQueuedCompletionStatus(). When a waiting thread gets a completion packet it already has the job (pointer) so it doen't need to retrieve it from a list. It works pretty efficiently. Just a thought. Mark
I was not aware of such a thing. Thanks alot !
-
Return pointer instead of reference? That way you can safely use NULL to indicate "no job". Using exceptions for that is in my opinion a complete waste of resources... May I ask why you are using mutexes? You could speed things up by using spinlocks (CRITICAL_SECTION). See docs for EnterCriticalSection(), LeaveCriticalSection(), etc, in the MSDN library. I'm assuming that the mutexes are never, or very seldom, held for a longer period of time. If they are, then mutexes is preferable, because they will put waiting thread to sleep rather than leaving it to "spin".
-- Now with chucklelin
Thanks for the advice on the Mutex. You are right, they are never held for long. I will change that.
-
I was not aware of such a thing. Thanks alot !
Mr.Brainley wrote:
I was not aware of such a thing. Thanks alot !
No problem. I just happen to be working on some code using one right now :) There's only 3 APIs and 1 structure involved so it's pretty simple and powerful. You'll see most articles describe using them for scalable multithreaded IO but they also make a handy queue for thread pools. Cheers, Mark
-
Joergen Sigvardsson wrote:
Pointers are an intrinsic concept in C++
Actually, pointers are an intrisic concept in C. C++ has references, and that has a reason. I try to follow that guideline.
Uhm.. that is a rather dull guideline. Just because some academic has decided that references are better than pointers (which the majority of the C++ community uses), it's counter productive NOT to use them in cases like this, as it would cut your development by at least an hour (seeing that you posted this approximately an hour ago). But hey, it's your time. :)
-- Based on a True Story
-
Uhm.. that is a rather dull guideline. Just because some academic has decided that references are better than pointers (which the majority of the C++ community uses), it's counter productive NOT to use them in cases like this, as it would cut your development by at least an hour (seeing that you posted this approximately an hour ago). But hey, it's your time. :)
-- Based on a True Story
Ok, you're right. And i don't do that just because some academic decided it. I use pointers often enough. But i also take my time to explore other solutions, since i think i still have much to learn. That is why i don't just use pointers because everyone else uses them. There may be a better solution. ( and remember : The fast path leads to the dark side ;)
-
Mr.Brainley wrote:
I was not aware of such a thing. Thanks alot !
No problem. I just happen to be working on some code using one right now :) There's only 3 APIs and 1 structure involved so it's pretty simple and powerful. You'll see most articles describe using them for scalable multithreaded IO but they also make a handy queue for thread pools. Cheers, Mark
actually, I'm implementing this thread pool for a scalable multithreaded IO application. Well, seems i don't need to do that anymore.
-
actually, I'm implementing this thread pool for a scalable multithreaded IO application. Well, seems i don't need to do that anymore.
Then you'll really like IO completion ports then. You can use the same pool of threads for handling queued jobs and queued IO requests if you choose to. Works slick!
-
This is my function :
CThreadPool::t_Job& CThreadPool::CJobQueue::getJob()
{
WaitForSingleObject(m_JobListMutex, INFINITE);for ( std::deque::iterator it = m\_JobList.begin(); it != m\_JobList.end(); it++ ) if ( it->m\_JobState != JOB\_WORKING) { it->m\_JobState = JOB\_WORKING; ReleaseMutex(m\_JobListMutex); return (\*it); } ReleaseMutex(m\_JobListMutex); // Need to do something here to indicate no item has been found}
I don't want to use a pointer for return. And i don't think it's apropriate to throw an exception, since that scenario is unavoidable, in fact quite regular. My only Idea left would be to return some special reference to a static element that functions as sort of a NULL. Any other ideas/comments ?
You should wrap your mutex calls in a class (open it in the constructor and release it in the destructor). This actually allows for the exception option (otherwise, throwing an exception would be bad since the code would not be exception safe). Given what you are trying to do, I would probably pass in a reference to be returned to as an argument and return a bool or some other status indicator as a return value. It would make your code look something like this:
class MutexHolder { public: MutexHolder(HANDLE mutex) { _Mutex = mutex; WaitForSingleObject(_Mutex, INFINITE); // I believe this should actually be OpenMutex // In which case, you would change the constructor arguement to a string } ~MutexHolder() { ReleaseMutex(_Mutex); } private: HANDLE _Mutex; }; bool job_not_working(const your_deque_type& t) { if (t.m_JobState != JOB_WORKING) { return true; } } bool CThreadPool::CJobQueue::getJob(CThreadPool::t_Job& job) { MutexHolder(m_JobListMutex); deque<your_deque_type>::iterator it = find_if(m_JobList.begin(), m_JobList.end(), job_not_working); if (it != m_JobList.end()) { job = *it; job.m_JobState = JOB_WORKING; return true; } return false; }If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
-
You should wrap your mutex calls in a class (open it in the constructor and release it in the destructor). This actually allows for the exception option (otherwise, throwing an exception would be bad since the code would not be exception safe). Given what you are trying to do, I would probably pass in a reference to be returned to as an argument and return a bool or some other status indicator as a return value. It would make your code look something like this:
class MutexHolder { public: MutexHolder(HANDLE mutex) { _Mutex = mutex; WaitForSingleObject(_Mutex, INFINITE); // I believe this should actually be OpenMutex // In which case, you would change the constructor arguement to a string } ~MutexHolder() { ReleaseMutex(_Mutex); } private: HANDLE _Mutex; }; bool job_not_working(const your_deque_type& t) { if (t.m_JobState != JOB_WORKING) { return true; } } bool CThreadPool::CJobQueue::getJob(CThreadPool::t_Job& job) { MutexHolder(m_JobListMutex); deque<your_deque_type>::iterator it = find_if(m_JobList.begin(), m_JobList.end(), job_not_working); if (it != m_JobList.end()) { job = *it; job.m_JobState = JOB_WORKING; return true; } return false; }If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
The problem with that approach, is that you'd be copying the job object. That may not always be possible. If you're going to return a status value, I'd suggest the COM way of returning the job: pointer to pointer, or reference to pointer (if you're hell bent on using references)
-- From the Makers of Futurama
-
The problem with that approach, is that you'd be copying the job object. That may not always be possible. If you're going to return a status value, I'd suggest the COM way of returning the job: pointer to pointer, or reference to pointer (if you're hell bent on using references)
-- From the Makers of Futurama
The object is stored in a deque, therefore it is already being copied. The only way to avoid that is to have the deque store pointers, in which case the approach still works, you just need to pass in a reference to a pointer instead.
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
-
Uhm.. that is a rather dull guideline. Just because some academic has decided that references are better than pointers (which the majority of the C++ community uses), it's counter productive NOT to use them in cases like this, as it would cut your development by at least an hour (seeing that you posted this approximately an hour ago). But hey, it's your time. :)
-- Based on a True Story
Joergen Sigvardsson wrote:
Uhm.. that is a rather dull guideline. Just because some academic has decided that references are better than pointers (which the majority of the C++ community uses), it's counter productive NOT to use them in cases like this, as it would cut your development by at least an hour (seeing that you posted this approximately an hour ago).
Knowing how to use references increases code readability, maintainability, and decreases the liklihood of bugs being introduced via improper usage of pointers. In general, if you can get away with not using pointers, you should do so.
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
-
Joergen Sigvardsson wrote:
Uhm.. that is a rather dull guideline. Just because some academic has decided that references are better than pointers (which the majority of the C++ community uses), it's counter productive NOT to use them in cases like this, as it would cut your development by at least an hour (seeing that you posted this approximately an hour ago).
Knowing how to use references increases code readability, maintainability, and decreases the liklihood of bugs being introduced via improper usage of pointers. In general, if you can get away with not using pointers, you should do so.
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
Zac Howland wrote:
Knowing how to use references increases code readability, maintainability
That I don't buy. The only thing that differs between references and pointers in terms of readability are "->" vs "." and "*" vs "&". If anything, references goes against readability, because it's not obvious by seeing the usage of a variable whether it's by value or by reference. With a pointer, it becomes quite obvious. References also have a cumbersome feature which is not always possible to overcome. You can't assign a reference later on - it must be initialized. That is not always possible. Also, you do not have the option to represent "no object" with references (unless you use unsafe methods which aren't portable).
Zac Howland wrote:
In general, if you can get away with not using pointers, you should do so.
The only time I use references are when "no object" isn't an option. In the case of the OP, a reference isn't worthwhile as it doesn't cope well with the NULL case - an expected case it turned out later in a clarification by the OP's author. Returning a status value indicating success is not a desired either as it requires an extra copy which may not be doable if the state of the job object is to be visible to all.
Zac Howland wrote:
and decreases the liklihood of bugs being introduced via improper usage of pointers.
I can buy that.
-- Broadcast simultaneously one year in the future
-
This is my function :
CThreadPool::t_Job& CThreadPool::CJobQueue::getJob()
{
WaitForSingleObject(m_JobListMutex, INFINITE);for ( std::deque::iterator it = m\_JobList.begin(); it != m\_JobList.end(); it++ ) if ( it->m\_JobState != JOB\_WORKING) { it->m\_JobState = JOB\_WORKING; ReleaseMutex(m\_JobListMutex); return (\*it); } ReleaseMutex(m\_JobListMutex); // Need to do something here to indicate no item has been found}
I don't want to use a pointer for return. And i don't think it's apropriate to throw an exception, since that scenario is unavoidable, in fact quite regular. My only Idea left would be to return some special reference to a static element that functions as sort of a NULL. Any other ideas/comments ?
I use boost::optional library for similar problems. But you return a reference and it introduces some limitations to what boost::optional library could do. Good luck..
Orhun Birsoy
-
Zac Howland wrote:
Knowing how to use references increases code readability, maintainability
That I don't buy. The only thing that differs between references and pointers in terms of readability are "->" vs "." and "*" vs "&". If anything, references goes against readability, because it's not obvious by seeing the usage of a variable whether it's by value or by reference. With a pointer, it becomes quite obvious. References also have a cumbersome feature which is not always possible to overcome. You can't assign a reference later on - it must be initialized. That is not always possible. Also, you do not have the option to represent "no object" with references (unless you use unsafe methods which aren't portable).
Zac Howland wrote:
In general, if you can get away with not using pointers, you should do so.
The only time I use references are when "no object" isn't an option. In the case of the OP, a reference isn't worthwhile as it doesn't cope well with the NULL case - an expected case it turned out later in a clarification by the OP's author. Returning a status value indicating success is not a desired either as it requires an extra copy which may not be doable if the state of the job object is to be visible to all.
Zac Howland wrote:
and decreases the liklihood of bugs being introduced via improper usage of pointers.
I can buy that.
-- Broadcast simultaneously one year in the future
Joergen Sigvardsson wrote:
That I don't buy. The only thing that differs between references and pointers in terms of readability are "->" vs "." and "*" vs "&". If anything, references goes against readability, because it's not obvious by seeing the usage of a variable whether it's by value or by reference. With a pointer, it becomes quite obvious.
Most software engineers I know disagree with you on that, but as this area can quickly become a religious topic, I won't go into the discussion.
Joergen Sigvardsson wrote:
References also have a cumbersome feature which is not always possible to overcome. You can't assign a reference later on - it must be initialized. That is not always possible
It is not always cumbersome, but rather forces a limitation on the code that is likely desirable. For example, calling delete on a pointer you don't "own" will delete the object, but cause problems at runtime (and may be difficult to debug). However, calling delete on a reference will give you a compile time error with line number pointing to your problem (no pun intended).
Joergen Sigvardsson wrote:
Also, you do not have the option to represent "no object" with references (unless you use unsafe methods which aren't portable).
That is when exceptions and status values are more valuable. Returning NULL requires that the client code must check to make sure they received a valid pointer (or suffer a bad runtime bug). Throwing an exception will break the execution of the current method so that no harm can be done to memory. Return a status code and passing in a reference to copy data to ensures that (even if the client fails to check the status), the object will be valid (i.e. no memory corruption).
Joergen Sigvardsson wrote:
The only time I use references are when "no object" isn't an option. In the case of the OP, a reference isn't worthwhile as it doesn't cope well with the NULL case - an expected case it turned out later in a clarification by the OP's author. Returning a status value indicating success is not a desired either as it requires an extra copy which may not be doable if the state of the job object is to be visible to all.
There are ways to handle the NULL case. The question isn't whether it does, but which method works better for a given situation. Pointers will w
-
This is my function :
CThreadPool::t_Job& CThreadPool::CJobQueue::getJob()
{
WaitForSingleObject(m_JobListMutex, INFINITE);for ( std::deque::iterator it = m\_JobList.begin(); it != m\_JobList.end(); it++ ) if ( it->m\_JobState != JOB\_WORKING) { it->m\_JobState = JOB\_WORKING; ReleaseMutex(m\_JobListMutex); return (\*it); } ReleaseMutex(m\_JobListMutex); // Need to do something here to indicate no item has been found}
I don't want to use a pointer for return. And i don't think it's apropriate to throw an exception, since that scenario is unavoidable, in fact quite regular. My only Idea left would be to return some special reference to a static element that functions as sort of a NULL. Any other ideas/comments ?
A bit off topic but you may want to consider using Window's built-in thread pool. Look up the
QueueUserWorkItemAPI.Steve