Update vendored thread_pool implementation #16210
Conversation
wence-
added
improvement
|
I culled all the compile-time optional features from the vendored implementation, one could reinstate them if wanted. Marked as non-breaking, even though the interface to the thread pool changed, since it's a detail API. This change should be benchmarked before merging. |
|
@madsbk does kvikIO need a similar change to its thread pool? |
Just to note, if one does this, we should consider if we also want to keep the other features (e.g. using a priority queue for the tasks rather than a FIFO queue) |
Agree, let's do both. One PR to update to the new version of the thread pool and another PR to use/benchmark the new features. |
|
Should we just fetch this using rapids-cmake instead of vendoring? It seems simpler, and they document how to do it with CPM (which rapids-cmake uses, so we'd do basically the same thing). |
|
I would agree with @vyasr on previous comment, as it will be easier to maintain and also fetch future versions easily using git tags instead of manually updating. |
OK, I did this, probably. |
| for (size_t i = 0; i < num_files; ++i) { | ||
| threads.submit(read_func, i); | ||
| } | ||
| threads.pause(); |
There was a problem hiding this comment.
pause and unpause methods are only enabled when this macro BS_THREAD_POOL_ENABLE_PAUSE is explicitly defined. Reference link here .
Co-authored-by: Srinivas Yadav <[email protected]>
cpp/CMakeLists.txt
Outdated
| @@ -798,13 +800,16 @@ target_compile_definitions(cudf PRIVATE "RMM_LOGGING_LEVEL=LIBCUDF_LOGGING_LEVEL | |||
| # Define spdlog level | |||
| target_compile_definitions(cudf PUBLIC "SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_${LIBCUDF_LOGGING_LEVEL}") | |||
|
|
|||
| # Enable pause functions in thread_pool implementation | |||
| target_compile_definitions(cudf PRIVATE "BS_THREAD_POOL_ENABLE_PAUSE=1") | |||
There was a problem hiding this comment.
We should move this off cudf and onto the BS_thread_pool target in get_thread_pool.cmake as an INTERFACE compile definition.
target_compile_definitions(BS_thread_pool INTERFACE "BS_THREAD_POOL_ENABLE_PAUSE=1")
Co-authored-by: Robert Maynard <[email protected]>
@GregoryKimball before we merge this, can we confirm that benchmarking doesn't present a negative signal? |
|
/merge |
Instead of hard coding the header of [`BS::thread_pool`](https://github.com/bshoshany/thread-pool?tab=readme-ov-file), we now fetch the implementation. Done for cudf in rapidsai/cudf#16210. Authors: - Mads R. B. Kristensen (https://github.com/madsbk) - Kyle Edwards (https://github.com/KyleFromNVIDIA) Approvers: - Bradley Dice (https://github.com/bdice) URL: #408
Description
Since we introduced the vendored thread_pool in #8752, upstream has introduced some new features, and particularly now uses condition variables/notification to handle when there are no tasks in the queue. This avoids the issue described in #16209 where the thread pool by default artificially introduces a delay of 1000microseconds to all tasks whenever the task queue is emptied.
Checklist