Use std::threads instead of openmp for parallelizing qsort#194
Closed
r-devulap wants to merge 1 commit intonumpy:mainfrom
Closed
Use std::threads instead of openmp for parallelizing qsort#194r-devulap wants to merge 1 commit intonumpy:mainfrom
r-devulap wants to merge 1 commit intonumpy:mainfrom
Conversation
r-devulap
commented
Apr 4, 2025
| arrsize_t task_threshold; | ||
|
|
||
| threadmanager() { | ||
| #ifdef XSS_COMPILE_OPENMP |
Member
Author
There was a problem hiding this comment.
I know, this needs a new macro.
There was a problem hiding this comment.
Pull Request Overview
This PR eliminates the dependency on OpenMP by replacing parallelized qsort code with a std::thread implementation using a dedicated threadmanager.
- Updated qsort_ and related functions to use std::thread and threadmanager instead of OpenMP tasks.
- Removed conditional OpenMP code blocks and adjusted thread management in both common and AVX512-specific qsort headers.
Reviewed Changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/xss-common-qsort.h | Replaced OpenMP tasks with std::thread calls and threadmanager usage. |
| src/xss-common-includes.h | Added necessary thread and mutex includes; introduced threadmanager struct. |
| src/avx512-16bit-qsort.hpp | Updated qsort calls to use threadmanager instead of OpenMP threshold. |
Files not reviewed (2)
- Makefile: Language not supported
- scripts/bench-compare.sh: Language not supported
| #ifdef XSS_COMPILE_OPENMP | ||
| max_thread_count = 8; | ||
| #else | ||
| max_thread_count = 0; |
There was a problem hiding this comment.
Setting max_thread_count to 0 in non-OpenMP builds disables parallel thread spawning. Consider initializing it with a non-zero value (e.g., based on std::thread::hardware_concurrency()) to enable proper multithreading.
Member
Author
|
Closing this in favor of #201 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First stab at switching to std::threads to not have the dependency on openmp. These look like about 10-20% slower than the openmp version, not entirely sure why: