various improvements in performance, safety, features and readability

[Youka]

I tried to keep the original authors style and focused on rather important changes. Everything was tested with the example source but i'd like some impressions too.

[h1arshad]

I agree the for(;;) is more unclear. One idea is to suppress the particular warning for it with #pragma warning (disable: ??). I find it strange that the compiler doesn't issue the same warning for for(;;)

[JoshuaBrookover]

Shouldn't this be a while loop? I am getting consistent crashing on line 66 unless I change this to:

while(!this->stop && this->tasks.empty())

This is my (admittedly bizarre) test case:

int main()
{
    ThreadPool pool;

    for (int i = 0; i < 100000; ++i)
    {
        pool.enqueue([i]
        {
            return i;
        }).get();
    }

    return 0;
}

[h1arshad]

@JoshuaBrookover works for me on VS 2013 with Nov 2013 CTP.

edit: I have a slightly different version to current ThreadPool class. I prefered the previous version of this class

[wilx]

@h1arshad : @JoshuaBrookover is right. Conditional variables can wake up spuriously and the condition needs to be rechecked on each wakeup. I have modified the code similarly in my fork that I use for log4cplus: https://github.com/log4cplus/ThreadPool/blob/master/ThreadPool.h#L113

[JoshuaBrookover]

It seems to be a bit of a race condition. If I run it enough times (or lower 100000 to something like 100) it will eventually work. But I do wonder why it was changed to begin with, since the original code used the lambda version of condition.wait() (which under the hood uses a while loop):

this->condition.wait(lock,
    [this]{ return this->stop || !this->tasks.empty(); });

http://en.cppreference.com/w/cpp/thread/condition_variable/wait

[h1arshad]

@wilx That's true, I wasn't aware what was in the code above. The previous version had such a check.

How come you had two conditional variables in yours (producer and consumer). was it not sufficient just to have one? Or more up front, is there a spurious wake up call that can occur somewhere which i'm not seeing..

[wilx]

@h1arshad: I have two condition variables because there are situation where I want to wake up only producers or only consumers. If I used only one CV, all threads would wake up and the congestion on the mutex would be bigger. All threads would have to wake up to check the condition and some of them would just go to sleep again immediately.

[h1arshad]

I see! Thanks for the explanation

Date: Thu, 23 Jul 2015 11:16:19 -0700
From: [email protected]
To: [email protected]
CC: [email protected]
Subject: Re: [ThreadPool] various improvements in performance, safety, features and readability (#27)

In ThreadPool.hpp:

• {

•    if(!threads_n)
  

•        throw std::invalid_argument("more than zero threads expected");
  

•    this->workers.reserve(threads_n);
  

•    for(; threads_n; --threads_n)
  

•        this->workers.emplace_back(
  

•            [this]
  

•            {
  

•                while(true)
  

•                {
  

•                    std::function<void()> task;
  

•                    {
  

•                        std::unique_lockstd::mutex lock(this->queue_mutex);
  

•                        if(!this->stop && this->tasks.empty())
  

@h1arshad: I have two condition variables because there are situation where I want to wake up only producers or only consumers. If I used only one CV, all threads would wake up and the congestion on the mutex would be bigger. All threads would have to wake up to check the condition and some of them would just go to sleep again immediately.

—
Reply to this email directly or view it on GitHub.

[Youka]

By @wilx

Conditional variables can wake up spuriously and the condition needs to be rechecked on each wakeup.

From cppreference

wait causes the current thread to block until the condition variable is notified or a spurious wakeup occurs, optionally looping until some predicate is satisfied.

Thanks for this hint. I'll delete this commit.

[JoshuaBrookover]

@wilx I had not seen your comment when I made my second comment. That is good to know about the spurious wakeups, I didn't know it would do that. Thanks!

[deepaknc]

@Youka nice change! Instead of several commits, can you squash them all as one change so it's easier to look at. Or create separate pull requests for each of the commits.

[Calthron]

Suggest to declare ThreadPool as final, instead of introducing virtual destructor.

[patrikhuber]

Interestingly, Chris Kohlhoff's thread_pool implementation (part of a ISO C++ standards proposal) uses std::thread::hardware_concurrency in its default constructor. I wonder whether that's an oversight or whether it's planned to change the standard.

https://github.com/chriskohlhoff/executors/blob/master/include/experimental/bits/thread_pool.h#L22
The code hasn't been updated in two years though so I'm not sure about the status of that proposal.

/cc @chriskohlhoff

[wolframroesler]

Joining in a little late but ...

Simplified for(;;) to while(true) which is more common and less optimizer work

Sorry but I completely disagree. for(;;) has been the idiomatic way of writing an endless loop since C was invented in the stone age. And do you have measurements to back up the "less optimizer work" part?

Love your "role of 5" addition, it's really missing in the original. Also agree with your choice of the ctor default parameter.

[patrikhuber]

Sorry but I completely disagree. for(;;) has been the idiomatic way of writing an endless loop since C was invented in the stone age. And do you have measurements to back up the "less optimizer work" part?

I would be surprised if it doesn't generate the exact same assembly code. And indeed I think it does: https://godbolt.org/g/SLuFaP

[wolframroesler]

@patrikhuber you are right, it will result in the same executable code. I understood "less optimizer work" to mean that @Youka thought compilation would be faster. I'm pretty sure it won't be (at least, not in a measurable way).