Introduce ThreadPool and use it for quicksort #201

r-devulap · 2025-05-08T22:10:15Z

This pull request adds Threadpool class and uses it for parallel quicksort using std::threads as an alternative to OpenMP. It updates the sorting logic to incorporate std::threads-based parallelism. The performance matches that of openMP.

Most of the code in the PR was written by copilot and needs a careful review.

Copilot

Pull Request Overview

This PR introduces a ThreadPool class to enable parallel quicksort using std::threads as an alternative to OpenMP. Key changes include:

Updating tests to trigger the new std::threads-based parallel sort.
Adding the ThreadPool implementation in src/xss-thread-pool.hpp.
Integrating std::thread-based parallel quicksort logic and updating the CI workflow to enable the new threading option.

Reviewed Changes

Copilot reviewed 5 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/test-qsort.cpp	Updated conditional macro to trigger extended tests with std::threads.
src/xss-thread-pool.hpp	Introduced a new ThreadPool implementation for task-based parallelism.
src/xss-common-qsort.h	Integrated std::thread-based quicksort with conditional compilation.
src/avx512-16bit-qsort.hpp	Updated parallel sort logic to use ThreadPool instead of OpenMP.
.github/workflows/c-cpp.yml	Modified Meson setup command to include std::threads build flag.

Files not reviewed (7)

Makefile: Language not supported
lib/meson.build: Language not supported
meson.build: Language not supported
meson_options.txt: Language not supported
scripts/bench-compare.sh: Language not supported
scripts/branch-compare.sh: Language not supported
tests/meson.build: Language not supported

.github/workflows/c-cpp.yml

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

sterrettm2

I think this looks pretty good overall. Besides the comments below, the only thing that might be nice is making a CI run for thread sanitizer; running that locally didn't show any issues, so it's probably fine, but it would be nice now that we are handling threading manually.

.github/workflows/c-cpp.yml

src/xss-thread-pool.hpp

(1) Add meson flag to build with std threads to ASAN tests (2) Remove unnecessary comments

thiagomacieira

Reviewed only the one commit

thiagomacieira · 2025-05-12T18:33:45Z

src/avx512-16bit-qsort.hpp

        constexpr int thread_limit = 8;
-        int thread_count = std::min(thread_limit, omp_get_max_threads());
+        int thread_count = std::min(thread_limit,
+                                    (int)std::thread::hardware_concurrency());


Why is 8 threads in a system with a lot of cores still better? Maybe you need to partition the data better to avoid locality problems.

thiagomacieira · 2025-05-12T18:34:48Z

src/xss-thread-pool.hpp

+    std::condition_variable condition; // Condition variable for task queue
+    std::condition_variable done_condition; // Condition variable for waiting
+    int active_tasks {0};
+    bool stop;


Needs to be std::atomic<bool>

thiagomacieira · 2025-05-12T18:37:01Z

src/xss-thread-pool.hpp

+        // Add a new task to the queue and notify one of the worker threads
+        std::unique_lock<std::mutex> lock(queue_mutex);
+        tasks.emplace(std::forward<F>(func));
+        condition.notify_one();


Race condition: the started threads may not have reached the first lock yet:

// Lock the queue mutex and wait for a task to be available std::unique_lock<std::mutex> lock(queue_mutex);

if that's the case, then this notify_one will notify no one. That means they will all lock that mutex, then proceed to wait (forever) on the condition.

You either want a semaphore or you need to check above before waiting if there already is work to be done.

Not sure if I understand this one fully. The ThreadPool constructor is called before the call to qsort_threads which is well before submit_task which in turn calls the enqueue method. Is it possible to get enqueue before the ThreadPool constructor is called?

You're missing the fact that it's two threads. You've described what's happening on the original thread, not on the thread you've just started for the pool. It's entirely possible (and even likely) that the original thread continues running after a couple hundred cycles spent in kernel mode, while the newly started thread takes several thousand cycles to start, in another core (which may be busy). That means the original thread may advance all the way to this function and lock the queue_mutex before the started thread has had a chance to lock it and call wait(). That means we notify no one here.

thiagomacieira · 2025-05-12T18:39:15Z

src/xss-thread-pool.hpp

+    void enqueue(F &&func)
+    {
+        // Add a new task to the queue and notify one of the worker threads
+        std::unique_lock<std::mutex> lock(queue_mutex);


Can you use C++17 for this code? If so, you can use CTAD:

std::unique_lock lock(queue_mutex);

Yes. We do use C++-17.

Then you should also use std::jthread instead of plain std::thread

hmm isn't that C++-20? https://en.cppreference.com/w/cpp/thread/jthread/jthread

Oh, right, so it is. Never mind this point then. You can still use CTAD to avoid writing <std::mutex> in C++17

thiagomacieira · 2025-05-12T18:39:34Z

src/xss-thread-pool.hpp

+        std::unique_lock<std::mutex> lock(queue_mutex);
+        stop = true;
+        lock.unlock();
+        condition.notify_all();


Same race condition problem

src/xss-thread-pool.hpp

thiagomacieira · 2025-05-12T18:40:52Z

src/xss-thread-pool.hpp

+    pool.task_start();
+    pool.enqueue([f = std::forward<F>(f), &pool]() {
+        f();
+        pool.task_end();


Race condition: if the first task you submit finishes before you enqueue the second, the thread pool will have notified on done_condition

Yup, thanks for catching that. Would a potential solution be to keep the threads running until explicitly asked to shut down? That can be done by adding a member function terminate_pools() which can be called once the qsort_threads() completes executing.

Yes. But what if you need to do more work? You should keep the threads running for a little while to avoid the cost of startup again.

r-devulap added 5 commits May 8, 2025 14:32

Replace openmp with std::threads: almost entirely written by copilot AI

a3d22a5

Add new meson options to build with std threads

748fa74

update macro in test-qsort

177f70c

Update benchmarking scripts to use new meson flag

c1b66a6

CI: update CI to build and test new code

fd2fe8d

r-devulap requested a review from Copilot May 8, 2025 22:10

Copilot AI reviewed May 8, 2025

View reviewed changes

.github/workflows/c-cpp.yml Outdated Show resolved Hide resolved

Fix typo in the meson build option.

a6ac4df

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

r-devulap mentioned this pull request May 8, 2025

Use std::threads instead of openmp for parallelizing qsort #194

Closed

r-devulap added 2 commits May 9, 2025 09:59

fix lint error

07b5a6e

Move threadpool under xss:tp:: namespace

6c46d71

sterrettm2 reviewed May 9, 2025

View reviewed changes

.github/workflows/c-cpp.yml Show resolved Hide resolved

src/xss-thread-pool.hpp Outdated Show resolved Hide resolved

src/xss-thread-pool.hpp Show resolved Hide resolved

src/xss-thread-pool.hpp Outdated Show resolved Hide resolved

Minor comments based on review

568cd7c

(1) Add meson flag to build with std threads to ASAN tests (2) Remove unnecessary comments

thiagomacieira suggested changes May 12, 2025

View reviewed changes

Introduce ThreadPool and use it for quicksort #201

Are you sure you want to change the base?

Introduce ThreadPool and use it for quicksort #201

Uh oh!

Conversation

r-devulap commented May 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

sterrettm2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thiagomacieira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r-devulap May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

r-devulap May 12, 2025 •

edited

Loading