From 309f90652304826ce81bc3e24ca458264cef8d87 Mon Sep 17 00:00:00 2001 From: Rafik Saliev Date: Tue, 6 Oct 2026 06:24:31 -0700 Subject: [PATCH] [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 3b97ec33a..618aa0c7f 100644 --- a/bindings/c/CMakeLists.txt +++ b/bindings/c/CMakeLists.txt @@ -66,16 +66,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 597c120e9..11c6825c6 100644 --- a/bindings/c/docs/C_API_Design.md +++ b/bindings/c/docs/C_API_Design.md @@ -350,7 +350,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 224bf167c..cd8864eb3 100644 --- a/bindings/c/include/svs/c/svs_c.h +++ b/bindings/c/include/svs/c/svs_c.h @@ -117,6 +117,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, @@ -790,9 +792,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, @@ -1222,9 +1230,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);