search

progschj / ThreadPool

A simple C++11 Thread Pool implementation
zlib License
7.85k stars 2.24k forks source link

various improvements in performance, safety, features and readability #27

Open Youka opened 9 years ago

Youka commented 9 years ago

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 commented 9 years ago

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(;;)

Youka commented 9 years ago

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 commented 9 years ago

@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 commented 8 years ago

@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 commented 8 years ago

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

patrikhuber commented 8 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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).