From c35c1d7b479e34114db8245bbea6d8fb36ffb59a Mon Sep 17 00:00:00 2001 From: Rafik Saliev Date: Tue, 6 Oct 2026 06:24:31 -0700 Subject: [PATCH 1/4] [C API] Add OpenMP support validation in ThreadPoolBuilder --- bindings/c/CMakeLists.txt | 19 +++++----- bindings/c/docs/C_API_Design.md | 2 +- bindings/c/include/svs/c/svs_c.h | 17 +++++++-- bindings/c/src/threadpool.hpp | 12 ++++++ bindings/c/tests/c_api_index.cpp | 47 ++++++++++++++---------- bindings/c/tests/c_api_index_builder.cpp | 10 ++++- 6 files changed, 71 insertions(+), 36 deletions(-) diff --git a/bindings/c/CMakeLists.txt b/bindings/c/CMakeLists.txt index 51a5541bd..3045b8df8 100644 --- a/bindings/c/CMakeLists.txt +++ b/bindings/c/CMakeLists.txt @@ -68,16 +68,17 @@ target_include_directories(${TARGET_NAME} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/src ) -find_package(OpenMP REQUIRED) -# PRIVATE: OpenMP is an implementation detail linked into the shared library. -# Exporting it would force consumers of the C ABI to resolve a C++ OpenMP -# target they never asked for. -target_link_libraries(${TARGET_NAME} PRIVATE OpenMP::OpenMP_CXX) +# Set default visibility to hidden for all symbols in the shared library. +target_compile_options(${TARGET_NAME} PRIVATE -fvisibility=hidden) -target_compile_options(${TARGET_NAME} PRIVATE - -DSVS_ENABLE_OMP=1 - -fvisibility=hidden -) +find_package(OpenMP) +if (OpenMP_CXX_FOUND) + # PRIVATE: OpenMP is an implementation detail linked into the shared library. + # Exporting it would force consumers of the C ABI to resolve a C++ OpenMP + # target they never asked for. + target_link_libraries(${TARGET_NAME} PRIVATE OpenMP::OpenMP_CXX) + target_compile_definitions(${TARGET_NAME} PRIVATE SVS_ENABLE_OMP=1) +endif() # OpenMP_CXX_FOUND if(UNIX AND NOT APPLE) # Don't export 3rd-party symbols from the lib diff --git a/bindings/c/docs/C_API_Design.md b/bindings/c/docs/C_API_Design.md index 12616d158..fcd14c5a2 100644 --- a/bindings/c/docs/C_API_Design.md +++ b/bindings/c/docs/C_API_Design.md @@ -402,7 +402,7 @@ Controls parallelization strategy for index operations. | Type | Configuration | Use Case | |------|---------------|----------| | **Native** | Thread count | Default SVS thread pool (recommended) | -| **OpenMP** | Uses OMP_NUM_THREADS | Integration with OpenMP applications | +| **OpenMP** | Thread count | Integration with OpenMP applications; requires a build with OpenMP support, otherwise `SVS_ERROR_NOT_IMPLEMENTED` | | **Single Thread** | No parallelization | Debugging or minimal overhead | | **Custom** | User-defined interface | Custom scheduling/work-stealing | diff --git a/bindings/c/include/svs/c/svs_c.h b/bindings/c/include/svs/c/svs_c.h index 6ed11d189..a42ce6c8b 100644 --- a/bindings/c/include/svs/c/svs_c.h +++ b/bindings/c/include/svs/c/svs_c.h @@ -137,6 +137,8 @@ enum svs_storage_kind { }; /// @brief Thread pool implementation used for parallel operations. +/// @remarks SVS_THREADPOOL_KIND_OMP is available only if the library was built with +/// OpenMP support; otherwise selecting it fails with SVS_ERROR_NOT_IMPLEMENTED. enum svs_threadpool_kind { SVS_THREADPOOL_KIND_NATIVE = 0, SVS_THREADPOOL_KIND_OMP = 1, @@ -1065,9 +1067,15 @@ SVS_API bool svs_index_builder_set_storage( /// @brief Set the thread pool configuration for the index builder /// @param builder The index builder handle /// @param kind The kind of thread pool to use -/// @param num_threads The number of threads to use (if applicable) +/// @param num_threads The number of threads to use; must be greater than zero (ignored +/// for SVS_THREADPOOL_KIND_SINGLE_THREAD) /// @param out_err An optional error handle to capture errors /// @return true on success, false on failure +/// @error On failure, if out_err is provided, it will contain: +/// - SVS_ERROR_INVALID_ARGUMENT if builder is NULL, num_threads is zero, or @p kind is +/// SVS_THREADPOOL_KIND_CUSTOM (use svs_index_builder_set_threadpool_custom() instead) +/// - SVS_ERROR_NOT_IMPLEMENTED if @p kind is SVS_THREADPOOL_KIND_OMP and the library was +/// built without OpenMP support SVS_API bool svs_index_builder_set_threadpool( svs_index_builder_h builder, svs_threadpool_kind_t kind, @@ -1658,9 +1666,10 @@ SVS_API bool svs_index_get_num_threads( /// @param out_err An optional error handle to capture errors /// @return true on success, false on failure /// @remarks This function is only supported for indices built with threadpool kinds -/// SVS_THREADPOOL_KIND_NATIVE or SVS_THREADPOOL_KIND_OMP. Attempting to call this -/// function on indices built with SVS_THREADPOOL_KIND_CUSTOM or -/// SVS_THREADPOOL_KIND_SINGLE_THREAD will fail and return false. +/// SVS_THREADPOOL_KIND_NATIVE or SVS_THREADPOOL_KIND_OMP (when built with OpenMP +/// support). Attempting to call this function on indices built with +/// SVS_THREADPOOL_KIND_CUSTOM or SVS_THREADPOOL_KIND_SINGLE_THREAD will fail and return +/// false. /// @error On failure, if out_err is provided, it will contain: /// - SVS_ERROR_INVALID_OPERATION if the index's threadpool kind is unresizable /// - SVS_ERROR_INVALID_ARGUMENT if num_threads is invalid or zero diff --git a/bindings/c/src/threadpool.hpp b/bindings/c/src/threadpool.hpp index e0d3e4e90..c43c26382 100644 --- a/bindings/c/src/threadpool.hpp +++ b/bindings/c/src/threadpool.hpp @@ -112,6 +112,13 @@ class ThreadPoolBuilder { "SVS_THREADPOOL_KIND_CUSTOM cannot be built automatically." ); } + + // Validate that SVS_OMP macro is properly defined. + SVS_VALIDATE_BOOL_ENV(SVS_OMP) + + if (!SVS_OMP && kind == SVS_THREADPOOL_KIND_OMP) { + throw svs::c_runtime::not_implemented("OpenMP support is not enabled."); + } } ThreadPoolBuilder(svs_threadpool_i pool) @@ -158,7 +165,12 @@ class ThreadPoolBuilder { case SVS_THREADPOOL_KIND_NATIVE: return ThreadPoolHandle(NativeThreadPool(num_threads)); case SVS_THREADPOOL_KIND_OMP: +#if SVS_OMP + // OMPThreadPool is only available if OpenMP support is enabled. return ThreadPoolHandle(OMPThreadPool(num_threads)); +#else + throw svs::c_runtime::not_implemented("OpenMP support is not enabled."); +#endif case SVS_THREADPOOL_KIND_SINGLE_THREAD: return ThreadPoolHandle(SequentialThreadPool()); case SVS_THREADPOOL_KIND_CUSTOM: diff --git a/bindings/c/tests/c_api_index.cpp b/bindings/c/tests/c_api_index.cpp index 9a7213f3f..000c38360 100644 --- a/bindings/c/tests/c_api_index.cpp +++ b/bindings/c/tests/c_api_index.cpp @@ -692,31 +692,38 @@ CATCH_TEST_CASE("C API Threadpool Management", "[c_api][index][threadpool]") { // Set OMP threadpool bool success = svs_index_builder_set_threadpool(builder, SVS_THREADPOOL_KIND_OMP, 3, error); - CATCH_REQUIRE(success); - CATCH_REQUIRE(svs_error_ok(error)); + // If OMP is not implemented, success will be false and the error code will be + // SVS_ERROR_NOT_IMPLEMENTED. Otherwise, success should be true and the error should + // be OK. + if (!success) { + CATCH_REQUIRE(svs_error_get_code(error) == SVS_ERROR_NOT_IMPLEMENTED); + } else { + CATCH_REQUIRE(svs_error_ok(error)); - svs_index_h index = svs_index_build(builder, data.data(), NUM_VECTORS, error); - CATCH_REQUIRE(index != nullptr); - CATCH_REQUIRE(svs_error_ok(error)); + svs_index_h index = svs_index_build(builder, data.data(), NUM_VECTORS, error); + CATCH_REQUIRE(index != nullptr); + CATCH_REQUIRE(svs_error_ok(error)); - // Get current number of threads - size_t num_threads = 0; - success = svs_index_get_num_threads(index, &num_threads, error); - CATCH_REQUIRE(success); - CATCH_REQUIRE(svs_error_ok(error)); - CATCH_REQUIRE(num_threads == 3); + // Get current number of threads + size_t num_threads = 0; + success = svs_index_get_num_threads(index, &num_threads, error); + CATCH_REQUIRE(success); + CATCH_REQUIRE(svs_error_ok(error)); + CATCH_REQUIRE(num_threads == 3); - // Set to different number of threads - success = svs_index_set_num_threads(index, 5, error); - CATCH_REQUIRE(success); - CATCH_REQUIRE(svs_error_ok(error)); + // Set to different number of threads + success = svs_index_set_num_threads(index, 5, error); + CATCH_REQUIRE(success); + CATCH_REQUIRE(svs_error_ok(error)); - // Verify the change - success = svs_index_get_num_threads(index, &num_threads, error); - CATCH_REQUIRE(success); - CATCH_REQUIRE(num_threads == 5); + // Verify the change + success = svs_index_get_num_threads(index, &num_threads, error); + CATCH_REQUIRE(success); + CATCH_REQUIRE(num_threads == 5); + + svs_index_free(index); + } - svs_index_free(index); svs_index_builder_free(builder); svs_algorithm_free(algorithm); svs_error_free(error); diff --git a/bindings/c/tests/c_api_index_builder.cpp b/bindings/c/tests/c_api_index_builder.cpp index 435026e5b..4ac5004e8 100644 --- a/bindings/c/tests/c_api_index_builder.cpp +++ b/bindings/c/tests/c_api_index_builder.cpp @@ -125,8 +125,14 @@ CATCH_TEST_CASE("C API Index Builder", "[c_api][index_builder]") { bool success = svs_index_builder_set_threadpool(builder, SVS_THREADPOOL_KIND_OMP, 2, error); - CATCH_REQUIRE(success == true); - CATCH_REQUIRE(svs_error_ok(error) == true); + // If OMP is not implemented, success will be false and the error code will be + // SVS_ERROR_NOT_IMPLEMENTED. Otherwise, success should be true and the error should + // be OK. + if (!success) { + CATCH_REQUIRE(svs_error_get_code(error) == SVS_ERROR_NOT_IMPLEMENTED); + } else { + CATCH_REQUIRE(svs_error_ok(error) == true); + } svs_index_builder_free(builder); svs_algorithm_free(algorithm); From 388d4da2868ddec0fa2c719592657c9eccc97a5e Mon Sep 17 00:00:00 2001 From: Rafik Saliev Date: Mon, 12 Oct 2026 03:21:45 -0700 Subject: [PATCH 2/4] Improve OMP support detection in C API tests --- bindings/c/tests/CMakeLists.txt | 4 ++++ bindings/c/tests/c_api_index.cpp | 10 +++------- bindings/c/tests/c_api_index_builder.cpp | 9 +-------- bindings/c/tests/c_api_test_utils.h | 11 +++++++++++ 4 files changed, 19 insertions(+), 15 deletions(-) diff --git a/bindings/c/tests/CMakeLists.txt b/bindings/c/tests/CMakeLists.txt index 48a527e7c..b94fe131d 100644 --- a/bindings/c/tests/CMakeLists.txt +++ b/bindings/c/tests/CMakeLists.txt @@ -75,6 +75,10 @@ if(SVS_RUNTIME_ENABLE_LVQ_LEANVEC) target_compile_definitions(${TARGET_NAME} PRIVATE SVS_TEST_EXPECT_LVQ_LEANVEC) endif() +if(OpenMP_CXX_FOUND) + target_compile_definitions(${TARGET_NAME} PRIVATE SVS_TEST_EXPECT_OPENMP) +endif() + # Set C++ standard target_compile_features(${TARGET_NAME} PRIVATE cxx_std_20) set_target_properties(${TARGET_NAME} PROPERTIES diff --git a/bindings/c/tests/c_api_index.cpp b/bindings/c/tests/c_api_index.cpp index 000c38360..46fd1215c 100644 --- a/bindings/c/tests/c_api_index.cpp +++ b/bindings/c/tests/c_api_index.cpp @@ -692,14 +692,10 @@ CATCH_TEST_CASE("C API Threadpool Management", "[c_api][index][threadpool]") { // Set OMP threadpool bool success = svs_index_builder_set_threadpool(builder, SVS_THREADPOOL_KIND_OMP, 3, error); - // If OMP is not implemented, success will be false and the error code will be - // SVS_ERROR_NOT_IMPLEMENTED. Otherwise, success should be true and the error should - // be OK. - if (!success) { - CATCH_REQUIRE(svs_error_get_code(error) == SVS_ERROR_NOT_IMPLEMENTED); - } else { - CATCH_REQUIRE(svs_error_ok(error)); + CATCH_REQUIRE(check_openmp_support(success, error)); + // if OMP is not supported the test will skip the following block. + if (success) { svs_index_h index = svs_index_build(builder, data.data(), NUM_VECTORS, error); CATCH_REQUIRE(index != nullptr); CATCH_REQUIRE(svs_error_ok(error)); diff --git a/bindings/c/tests/c_api_index_builder.cpp b/bindings/c/tests/c_api_index_builder.cpp index 4ac5004e8..549f27281 100644 --- a/bindings/c/tests/c_api_index_builder.cpp +++ b/bindings/c/tests/c_api_index_builder.cpp @@ -125,14 +125,7 @@ CATCH_TEST_CASE("C API Index Builder", "[c_api][index_builder]") { bool success = svs_index_builder_set_threadpool(builder, SVS_THREADPOOL_KIND_OMP, 2, error); - // If OMP is not implemented, success will be false and the error code will be - // SVS_ERROR_NOT_IMPLEMENTED. Otherwise, success should be true and the error should - // be OK. - if (!success) { - CATCH_REQUIRE(svs_error_get_code(error) == SVS_ERROR_NOT_IMPLEMENTED); - } else { - CATCH_REQUIRE(svs_error_ok(error) == true); - } + CATCH_REQUIRE(check_openmp_support(success, error)); svs_index_builder_free(builder); svs_algorithm_free(algorithm); diff --git a/bindings/c/tests/c_api_test_utils.h b/bindings/c/tests/c_api_test_utils.h index e6e070b38..52cd18526 100644 --- a/bindings/c/tests/c_api_test_utils.h +++ b/bindings/c/tests/c_api_test_utils.h @@ -266,3 +266,14 @@ inline void* failing_allocator_allocate( inline void failing_allocator_deallocate( void* /*self*/, void* /*ptr*/, size_t /*size*/, size_t /*alignment*/ ) {} + +// If OMP is not implemented, success will be false and the error code will be +// SVS_ERROR_NOT_IMPLEMENTED. Otherwise, success should be true and the error should +// be OK. +inline bool check_openmp_support(bool success, svs_error_h error) { +#ifdef SVS_TEST_EXPECT_OPENMP + return success && svs_error_ok(error); +#else + return !success && svs_error_get_code(error) == SVS_ERROR_NOT_IMPLEMENTED; +#endif +} From 1674200e4b3ede8cc2bf75c83bd83212c5a966a8 Mon Sep 17 00:00:00 2001 From: Rafik Saliev Date: Thu, 15 Oct 2026 10:11:39 -0700 Subject: [PATCH 3/4] Address code review --- bindings/c/CMakeLists.txt | 23 +++++++++++++++-------- bindings/c/include/svs/c/svs_c.h | 3 +-- bindings/c/src/svs_c.cpp | 8 +++----- bindings/c/src/threadpool.hpp | 4 ++-- bindings/c/tests/CMakeLists.txt | 1 + cmake/options.cmake | 5 +++++ 6 files changed, 27 insertions(+), 17 deletions(-) diff --git a/bindings/c/CMakeLists.txt b/bindings/c/CMakeLists.txt index 3045b8df8..36cd0b16c 100644 --- a/bindings/c/CMakeLists.txt +++ b/bindings/c/CMakeLists.txt @@ -71,14 +71,21 @@ target_include_directories(${TARGET_NAME} # Set default visibility to hidden for all symbols in the shared library. target_compile_options(${TARGET_NAME} PRIVATE -fvisibility=hidden) -find_package(OpenMP) -if (OpenMP_CXX_FOUND) - # PRIVATE: OpenMP is an implementation detail linked into the shared library. - # Exporting it would force consumers of the C ABI to resolve a C++ OpenMP - # target they never asked for. - target_link_libraries(${TARGET_NAME} PRIVATE OpenMP::OpenMP_CXX) - target_compile_definitions(${TARGET_NAME} PRIVATE SVS_ENABLE_OMP=1) -endif() # OpenMP_CXX_FOUND +if(NOT DEFINED SVS_ENABLE_OMP) + option(SVS_ENABLE_OMP + "Enable OpenMP threadpool support for parallelism." ON) +endif() + +if(SVS_ENABLE_OMP) + find_package(OpenMP REQUIRED) + if (OpenMP_CXX_FOUND) + # PRIVATE: OpenMP is an implementation detail linked into the shared library. + # Exporting it would force consumers of the C ABI to resolve a C++ OpenMP + # target they never asked for. + target_link_libraries(${TARGET_NAME} PRIVATE OpenMP::OpenMP_CXX) + target_compile_definitions(${TARGET_NAME} PRIVATE SVS_ENABLE_OMP=1) + endif() # OpenMP_CXX_FOUND +endif() # SVS_ENABLE_OMP if(UNIX AND NOT APPLE) # Don't export 3rd-party symbols from the lib diff --git a/bindings/c/include/svs/c/svs_c.h b/bindings/c/include/svs/c/svs_c.h index a42ce6c8b..7380d173f 100644 --- a/bindings/c/include/svs/c/svs_c.h +++ b/bindings/c/include/svs/c/svs_c.h @@ -1067,8 +1067,7 @@ SVS_API bool svs_index_builder_set_storage( /// @brief Set the thread pool configuration for the index builder /// @param builder The index builder handle /// @param kind The kind of thread pool to use -/// @param num_threads The number of threads to use; must be greater than zero (ignored -/// for SVS_THREADPOOL_KIND_SINGLE_THREAD) +/// @param num_threads The number of threads to use (if applicable) /// @param out_err An optional error handle to capture errors /// @return true on success, false on failure /// @error On failure, if out_err is provided, it will contain: diff --git a/bindings/c/src/svs_c.cpp b/bindings/c/src/svs_c.cpp index 184822103..809dc8b33 100644 --- a/bindings/c/src/svs_c.cpp +++ b/bindings/c/src/svs_c.cpp @@ -629,15 +629,13 @@ extern "C" bool svs_index_builder_set_threadpool( size_t num_threads, svs_error_h out_err ) { - if (builder == nullptr) { - SET_ERROR(out_err, SVS_ERROR_INVALID_ARGUMENT, "Invalid argument"); - return false; - } using namespace svs::c_runtime; return wrap_exceptions( [&]() { EXPECT_ARG_NOT_NULL(builder); - EXPECT_ARG_GT_THAN(num_threads, 0); + if (kind != SVS_THREADPOOL_KIND_SINGLE_THREAD) { + EXPECT_ARG_GT_THAN(num_threads, 0); + } builder->impl->set_threadpool_builder({kind, num_threads}); return true; }, diff --git a/bindings/c/src/threadpool.hpp b/bindings/c/src/threadpool.hpp index c43c26382..6f2ae55ae 100644 --- a/bindings/c/src/threadpool.hpp +++ b/bindings/c/src/threadpool.hpp @@ -114,7 +114,7 @@ class ThreadPoolBuilder { } // Validate that SVS_OMP macro is properly defined. - SVS_VALIDATE_BOOL_ENV(SVS_OMP) + SVS_VALIDATE_BOOL_ENV(SVS_OMP); if (!SVS_OMP && kind == SVS_THREADPOOL_KIND_OMP) { throw svs::c_runtime::not_implemented("OpenMP support is not enabled."); @@ -169,7 +169,7 @@ class ThreadPoolBuilder { // OMPThreadPool is only available if OpenMP support is enabled. return ThreadPoolHandle(OMPThreadPool(num_threads)); #else - throw svs::c_runtime::not_implemented("OpenMP support is not enabled."); + assert(false && "OpenMP support is not enabled."); #endif case SVS_THREADPOOL_KIND_SINGLE_THREAD: return ThreadPoolHandle(SequentialThreadPool()); diff --git a/bindings/c/tests/CMakeLists.txt b/bindings/c/tests/CMakeLists.txt index b94fe131d..2b86b8d29 100644 --- a/bindings/c/tests/CMakeLists.txt +++ b/bindings/c/tests/CMakeLists.txt @@ -75,6 +75,7 @@ if(SVS_RUNTIME_ENABLE_LVQ_LEANVEC) target_compile_definitions(${TARGET_NAME} PRIVATE SVS_TEST_EXPECT_LVQ_LEANVEC) endif() +find_package(OpenMP QUIET) if(OpenMP_CXX_FOUND) target_compile_definitions(${TARGET_NAME} PRIVATE SVS_TEST_EXPECT_OPENMP) endif() diff --git a/cmake/options.cmake b/cmake/options.cmake index 75be25326..bfc3ebf71 100644 --- a/cmake/options.cmake +++ b/cmake/options.cmake @@ -73,6 +73,11 @@ option(SVS_INITIALIZE_LOGGER ON # enabled by default ) +option(SVS_ENABLE_OMP + "Enable OpenMP threadpool support for parallelism." + ON # enabled by default +) + ##### ##### Experimental ##### From 019b585da0e1df21cdddb6d5436a7f9e979c4321 Mon Sep 17 00:00:00 2001 From: ethanglaser <42726565+ethanglaser@users.noreply.github.com> Date: Thu, 15 Oct 2026 15:19:03 -0700 Subject: [PATCH 4/4] base OpenMP test expectation on SVS_ENABLE_OMP --- bindings/c/tests/CMakeLists.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bindings/c/tests/CMakeLists.txt b/bindings/c/tests/CMakeLists.txt index 2b86b8d29..f9abb9f3a 100644 --- a/bindings/c/tests/CMakeLists.txt +++ b/bindings/c/tests/CMakeLists.txt @@ -75,8 +75,7 @@ if(SVS_RUNTIME_ENABLE_LVQ_LEANVEC) target_compile_definitions(${TARGET_NAME} PRIVATE SVS_TEST_EXPECT_LVQ_LEANVEC) endif() -find_package(OpenMP QUIET) -if(OpenMP_CXX_FOUND) +if(SVS_ENABLE_OMP) target_compile_definitions(${TARGET_NAME} PRIVATE SVS_TEST_EXPECT_OPENMP) endif()