feat(concurrency): Expose WITH_OPENMP option and implement native C++11 std::thread fallback#104
Conversation
5568d79 to
4516215
Compare
Provide type-safe C++11 std::thread fallback block under #if defined(_OPENMP) in process.cpp, and add WITH_OPENMP CMake option in CMakeLists.txt. Strips all OpenMP compiler/linker bindings when compiled with -DWITH_OPENMP=OFF, allowing stable parallel concurrency out-of-the-box in secure sandboxes, macOS, and WebAssembly. fix(concurrency): Implement 64-bit LL integer overflow safeguard inside loop index math
08f9cca to
20eec72
Compare
20eec72 to
5abb356
Compare
f8570e4 to
96ed730
Compare
…dence with sequential fallback
…ndard threads fallback
|
@SarahWeiii Any chance you can take a look at this? |
|
Hi @clintmorris229, thanks for the PR — parallelism on macOS/sandboxed builds is genuinely valuable. But the current implementation has a few problems that need fixing before merge:
Requested change: make this a true fallback — in CMake, only define WITH_STD_THREADS when OpenMP is not used, and put #if defined(_OPENMP) first in all conditionals so they agree. With that, the exception handling, and an updated benchmark, I'd be happy to merge. |
- Make WITH_STD_THREADS a true fallback in CMake. - Default WITH_OPENMP to OFF on macOS. - Enable POSITION_INDEPENDENT_CODE globally in CMake. - Reorder conditional blocks in process.cpp to prioritize OpenMP checks. - Capture and rethrow worker thread exceptions in std::thread fallback. - Add mutex to wrap i4_sobol calls in model_obj.cpp to fix data races on its static variables. TAG=agy CONV=a67ee12d-d8d2-49db-ba0a-2b332d53d03c
- Make WITH_STD_THREADS a true fallback in CMake. - Default WITH_OPENMP to OFF on macOS. - Enable POSITION_INDEPENDENT_CODE globally in CMake. - Reorder conditional blocks in process.cpp to prioritize OpenMP checks. - Capture and rethrow worker thread exceptions in std::thread fallback. - Add mutex to wrap i4_sobol calls in model_obj.cpp to fix data races on its static variables. TAG=agy CONV=a67ee12d-d8d2-49db-ba0a-2b332d53d03c
2633ff0 to
73e7f66
Compare
…t splitting The recent redirect of windows-2025 runners to windows-2025-vs2026 (using CMake 4.3) introduced an issue where -DCMAKE_POLICY_VERSION_MINIMUM=3.5 was split into two arguments (-DCMAKE_POLICY_VERSION_MINIMUM=3 and .5), causing CMake errors on Windows. Quoting the argument ensures it is parsed as a single string. TAG=agy CONV=a67ee12d-d8d2-49db-ba0a-2b332d53d03c
@SarahWeiii, thanks for the feedback! I have updated the PR to address your comments:
|
) This PR adds a new version `1.0.11.bcr.3` for the `coacd` module. The previous version `1.0.11.bcr.2` enabled OpenMP by default. However, OpenMP support can be difficult to configure and build portably across all platforms in the BCR (especially on macOS with default Clang toolchains). This release configures the Bazel build to run without OpenMP and instead utilizes a standard C++ threading fallback, ensuring wider compatibility and easier integration across Linux, macOS, and Windows. #### Changes * Added `std_threads.patch`: * Backports changes to support standard C++17 `std::thread` multi-threading when OpenMP is disabled. * Implements thread-safe guards (e.g., locking the shared `i4_sobol` quasirandom generator) to ensure correctness during parallel execution. * Updated `overlay/BUILD.bazel`: * Removed OpenMP compiler flags (`-fopenmp`, `/openmp`) and the `@openmp` dependency. * Added `WITH_STD_THREADS=1` to the compiler defines to enable the standard library threading path. * Simplified platform-specific configuration for Windows, macOS, and Linux. * Updated `metadata.json`: Registered the new `1.0.11.bcr.3` version. Reference this [PR](SarahWeiii/CoACD#104) for benchmarking info
The Issue
CoACD currently uses OpenMP for concurrency. This causes issues in specific environments:
libomp.sovslibgomp.so.1), resulting inSIGSEGVcrashes.Changes
Introduced a C++17
std::threadfallback for environments where OpenMP is unavailable or problematic.WITH_OPENMP(defaultON, butOFFon macOS) andWITH_STD_THREADS(defaultON, auto-falls back if OpenMP is disabled).src/process.cppusingstd::thread.<thread>,<mutex>,<chrono>) in#if defined(WITH_STD_THREADS)to prevent compile-time overhead when disabled.std::thread, or Sequential execution.std::mutexandstd::lock_guardfor safe concurrent vector aggregation insrc/process.cpp.std::exception_ptrto safely capture and rethrow worker thread exceptions in the main thread.i4_sobolfunction insrc/model_obj.cppwith astd::mutexto prevent data races on its internal static state variables during parallel execution.CMAKE_POSITION_INDEPENDENT_CODE ONto resolve static linking conflicts with FetchContent dependencies.CMAKE_POLICY_VERSION_MINIMUMin.github/workflows/build.ymlto prevent argument splitting on the updated Windows runner image (CMake 4.3).Outcomes
std::threadfallback.Benchmark: C++17
std::threadvs OpenMP (Threshold = 0.05)Benchmarked on 16 cores using various models, including the Stanford Armadillo (99,976 faces).
Table 1: Convex Hull Limit = 1
Table 2: Convex Hull Limit = 10
Table 3: Convex Hull Limit = -1 (Unlimited)