Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use TBB's concurrent queue implementation #158

Merged
merged 4 commits into from
Jul 17, 2023
Merged

Use TBB's concurrent queue implementation #158

merged 4 commits into from
Jul 17, 2023

Conversation

alexdewar
Copy link
Contributor

@alexdewar alexdewar commented Jul 14, 2023

TBB is a C++ library for multithreading. Currently we do use it in the repo, but only indirectly: gcc uses it as its backend for the STL parallel algorithms, so you need it on Linux if you actually want the algorithms to run in parallel. The thing is that you have to remember to install it manually. It would be easier to just have it installed with vcpkg along with the other dependencies. This does mean that it would therefore become a dependency for Windows too, but it's not a big one (< 2MiB on my machine). The other advantage of making TBB a requirement is that we can make use of some of its features. Notably, it contains threadsafe versions of various containers, which, aside from being safer than doing the locking yourself, are also often faster, so we could use these in many places in the code base (#159). Currently the CPU usage is poor, which might be due to lock contention (#80) and in that case, using TBB's containers would likely help.

This PR makes TBB a hard requirement and integrates it with vcpkg. I also replaced the custom ThreadsafeQueue implementation with tbb::concurrent_queue and removed the former from the code base (fixes #151).

The motivation for this PR was that I was getting failures for the ThreadsafeQueue tests on macOS runners. After looking at the tests, it seems that the problem was probably just that the tests relied on things happening within a certain timeframe (eww), but rather than trying to fix the tests, I thought it was better just to rip this class out altogether as it was on my hit list anyway.

@alexdewar alexdewar marked this pull request as draft July 14, 2023 09:56
@alexdewar alexdewar marked this pull request as ready for review July 14, 2023 13:07
Copy link
Contributor

@jamesturner246 jamesturner246 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. Lots of stuff deleted. So this is almost a drop-in replacement for the existing event queue?

@alexdewar
Copy link
Contributor Author

@jamesturner246 Yep. It will behave in exactly the same way.

Copy link
Contributor

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Removing stuff that is not needed is always a good idea. It all seems to boil down to TBB. What is it about and why is no longer something that might not be found?

Comment on lines 1 to 6
find_package(fmt CONFIG REQUIRED)
find_package(cxxopts CONFIG REQUIRED)
find_package(nlohmann_json CONFIG REQUIRED)
find_package(TBB CONFIG REQUIRED)
find_path(RAPIDCSV_INCLUDE_DIRS "rapidcsv.h")
find_package(Threads REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Knowing nothing about CMake, I'm curious: where does CMake looks for all of these packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Traditionally, CMake looks for these on your system and e.g. the TBB library will come with a CMake package config file. And traditionally this worked rather poorly on Windows 😛.

Health-GPS uses vcpkg on top to manage dependencies so they are automatically built from source before everything else is (provided you have vcpkg installed!).

@dalonsoa dalonsoa mentioned this pull request Jul 14, 2023
@alexdewar
Copy link
Contributor Author

TBB is Intel's Threading Building Blocks library: https://github.com/oneapi-src/oneTBB

It basically just provides a bunch of useful bits for multithreaded applications, including tools for parallelising work and threadsafe containers (which I'm using here). Newer versions of the C++ standard include parallelised versions of common algorithms (e.g. sort) and gcc's implementation just uses TBB under the hood, so we need it for that anyway, so it kind of makes sense to just have it as a "proper" dependency and then we can use some of the nice features on all platforms.

@alexdewar alexdewar merged commit ce1be41 into main Jul 17, 2023
5 checks passed
@alexdewar alexdewar deleted the tbb_queue branch July 17, 2023 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace ThreadsafeQueue class with TBB equivalent
3 participants