Small issue with lates release

[Green-Sky]

So I was extending the fresh thread-pool release with a priority_queue. While doing so I ran into multiple issues:

• std::vector<std::atomic<bool>> seems to be invalid code.
  I read that std::atomic is non-copyable and non-movable, while std::vector requires at least either.
• running static analysis (cppcheck) told me flag1 == falg2 comparing the same value.
  Not sure if cppcheck is right here, but I think a compiler might optimize this away...
  Possible solution: declaring bool flag; a volatile.

I really like how simple the Library is 😃

My wip code: https://github.com/Green-Sky/thread-pool/commits/priority_queue
Right now my code consistently fails the "task monitoring" test. I suspect the first issue to be the cause.

[bshoshany]

Thanks for opening the issue @Green-Sky. I'm glad you like the simplicity of the library, this is indeed one of its core design principles :)

Regarding vectors: yes, you cannot copy an std::atomic<bool> and this means the vector cannot do anything that would require copying such as push_back() (which might want to copy elements if it needs to reallocate memory), but if it's a static vector there should in principle be no issue.

That being said, I do feel that using a smart pointer here would be better (even though whenever I use smart pointers people always tell me "why didn't you just use a vector??") and I will change that in a future release. Meanwhile you can replace

std::vector<std::atomic<bool>> flags(n)

with

std::unique_ptr<std::atomic<bool>[]> flags = std::make_unique<std::atomic<bool>[]>(n)

(and similarly for the other vectors) to avoid this issue.

Regarding cppcheck: I've never used it before but I tried it now and the only thing it told me is to "Consider using std::accumulate algorithm instead of a raw loop" on line 375 (which I intentionally didn't do in order to keep the test as transparent as possible). How are you getting this message about the flags? Are you using some specific flags when calling cppcheck?

I don't think it's true that flag1 and flag2 are always the same value. Perhaps it's confused because on line 227 I use *flag1_ = *flag2_ = true which of course means they should always have the same value, but that's exactly what we're testing for, if they don't have the same value then we know there's a problem :)

Regarding task priority: I've actually been thinking of adding that functionality, but it seems to me that std::priority_queue might be too slow. So I was thinking of implementing it a bit differently. But I first want to check performance with std::queue vs. std::priority_queue and see if there's a noticeable difference. I haven't gotten around to it so far, though.

[Green-Sky]

Regarding cppcheck: I've never used it before but I tried it now and the only thing it told me is to "Consider using std::accumulate algorithm instead of a raw loop" on line 375 (which I intentionally didn't do in order to keep the test as transparent as possible). How are you getting this message about the flags? Are you using some specific flags when calling cppcheck?

Yes I did get that one too and intentionally ignored it 😄 .
I ran $ cppcheck --enable=all --std=c++17 BS_thread_pool.hpp BS_thread_pool_test.cpp.

... but it seems to me that std::priority_queue might be too slow.

A quick look turned up this, but its quiet dated (~2006 from the looks of it)
https://gcc.gnu.org/onlinedocs/libstdc++/ext/pb_ds/pq_performance_tests.html

But I first want to check performance with std::queue vs. std::priority_queue and see if there's a noticeable difference. I haven't gotten around to it so far, though.

Just take a look at the changes I made, they are not many.

[bshoshany]

I ran $ cppcheck --enable=all --std=c++17 BS_thread_pool.hpp BS_thread_pool_test.cpp.

I tried that but still don't get that message. Here's the full output:

Checking BS_thread_pool.hpp ...
1/2 files checked 38% done
Checking BS_thread_pool_test.cpp ...
BS_thread_pool_test.cpp:375:13: style: Consider using std::accumulate algorithm instead of a raw loop. [useStlAlgorithm]
        sum += s;
            ^
Checking BS_thread_pool_test.cpp: _MSC_VER...
2/2 files checked 100% done

(I also added --suppress=missingIncludeSystem because for some reason it complains it cannot find the standard library headers...)

Maybe we're not using the same version? I'm using 2.8.

... but it seems to me that std::priority_queue might be too slow.

A quick look turned up this, but its quiet dated (~2006 from the looks of it) https://gcc.gnu.org/onlinedocs/libstdc++/ext/pb_ds/pq_performance_tests.html

But I first want to check performance with std::queue vs. std::priority_queue and see if there's a noticeable difference. I haven't gotten around to it so far, though.

Just take a look at the changes I made, they are not many.

Thanks, I will take a look at some point in the future, but right now I have higher-priority things to work on (pun intended ;)

[Green-Sky]

Maybe we're not using the same version? I'm using 2.8.

Oh gad, looks like mine is ancient: 1.90 (ubuntu 20.04)

my full output:

Checking BS_thread_pool.hpp ...
1/2 files checked 39% done
Checking BS_thread_pool_test.cpp ...
BS_thread_pool_test.cpp:80:9: style: Condition 'output_log' is always true [knownConditionTrueFalse]
    if (output_log)
        ^
BS_thread_pool_test.cpp:772:9: style: Condition 'output_log' is always true [knownConditionTrueFalse]
    if (output_log)
        ^
BS_thread_pool_test.cpp:781:9: style: Condition 'output_log' is always true [knownConditionTrueFalse]
    if (output_log)
        ^
BS_thread_pool_test.cpp:786:9: style: Condition 'enable_tests' is always true [knownConditionTrueFalse]
    if (enable_tests)
        ^
BS_thread_pool_test.cpp:791:13: style: Condition 'enable_tests' is always true [knownConditionTrueFalse]
        if (enable_tests)
            ^
BS_thread_pool_test.cpp:793:13: style: Condition 'enable_benchmarks' is always true [knownConditionTrueFalse]
        if (enable_benchmarks)
            ^
BS_thread_pool_test.cpp:229:21: style: Same expression on both sides of '&&' because 'flag1' and 'flag2' represent the same value. [knownConditionTrueFalse]
        check(flag1 && flag2);
                    ^
BS_thread_pool_test.cpp:225:22: note: 'flag1' is assigned value 'false' here.
        bool flag1 = false;
                     ^
BS_thread_pool_test.cpp:226:22: note: 'flag2' is assigned value 'false' here.
        bool flag2 = false;
                     ^
BS_thread_pool_test.cpp:229:21: note: Same expression on both sides of '&&' because 'flag1' and 'flag2' represent the same value.
        check(flag1 && flag2);
                    ^
BS_thread_pool_test.cpp:255:21: style: Same expression on both sides of '&&' because 'flag1' and 'flag2' represent the same value. [knownConditionTrueFalse]
        check(flag1 && flag2);
                    ^
BS_thread_pool_test.cpp:252:22: note: 'flag1' is assigned value 'false' here.
        bool flag1 = false;
                     ^
BS_thread_pool_test.cpp:253:22: note: 'flag2' is assigned value 'false' here.
        bool flag2 = false;
                     ^
BS_thread_pool_test.cpp:255:21: note: Same expression on both sides of '&&' because 'flag1' and 'flag2' represent the same value.
        check(flag1 && flag2);
                    ^
BS_thread_pool_test.cpp:294:46: style: Same expression on both sides of '&&' because 'flag2' and 'flag1' represent the same value. [knownConditionTrueFalse]
        check(my_future.get() == 42 && flag1 && flag2);
                                             ^
BS_thread_pool_test.cpp:285:22: note: 'flag2' is assigned value 'false' here.
        bool flag2 = false;
                     ^
BS_thread_pool_test.cpp:284:22: note: 'flag1' is assigned value 'false' here.
        bool flag1 = false;
                     ^
BS_thread_pool_test.cpp:294:46: note: Same expression on both sides of '&&' because 'flag2' and 'flag1' represent the same value.
        check(my_future.get() == 42 && flag1 && flag2);
                                             ^
BS_thread_pool_test.cpp:379:13: style: Consider using std::accumulate algorithm instead of a raw loop. [useStlAlgorithm]
        sum += s;
            ^
Checking BS_thread_pool_test.cpp: _MSC_VER...
2/2 files checked 100% done
nofile:0:0: information: Cppcheck cannot find all the include files (use --check-config for details) [missingIncludeSystem]

Thanks, I will take a look at some point in the future, but right now I have higher-priority things to work on (pun intended ;)

Yea sure, also lol.

[bshoshany]

Interesting. I think they probably made it smarter in subsequent versions, and therefore the new version recognizes that this is not really an issue. Thanks! :)