Skip to content

[C-API] Add getter and setter for index threadpool size#305

Open
rfsaliev wants to merge 4 commits intodev/c-apifrom
rfsaliev/c-api-index-num-threads
Open

[C-API] Add getter and setter for index threadpool size#305
rfsaliev wants to merge 4 commits intodev/c-apifrom
rfsaliev/c-api-index-num-threads

Conversation

@rfsaliev
Copy link
Copy Markdown
Member

@rfsaliev rfsaliev commented Mar 27, 2026

Adds svs_index_get_num_threads / svs_index_set_num_threads to the C API, enabling dynamic inspection and resizing of the search threadpool after index construction.

ThreadPoolBuilder

  • Added get_threads_num() — delegates to the custom pool's size() op when kind == CUSTOM, otherwise returns the stored count
  • Added resize(n) — updates stored thread count; throws std::invalid_argument for n == 0, SINGLE_THREAD, or CUSTOM kinds (surfaced as SVS_ERROR_INVALID_ARGUMENT through wrap_exceptions)

Index wrappers (index.hpp)

  • Index stores a ThreadPoolBuilder; get_num_threads() is pure-virtual — implemented in IndexVamana and DynamicIndexVamana by delegating to the wrapped svs::Vamana / svs::DynamicVamana instance, so the value reflects actual runtime state
  • set_num_threads(n) calls pool_builder.resize(n) then rebuilds and installs the threadpool via set_threadpool()

C API (svs_c.cpp / svs_c.h)

  • Both entry points validate index->impl non-null before dereferencing (consistent with existing handle-check pattern)
  • Public header documents supported kinds and expected error codes for unsupported configurations

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds C API support to query and modify the search thread pool size for an SVS index after construction, by persisting thread pool configuration alongside the index implementation.

Changes:

  • Added svs_index_get_num_threads / svs_index_set_num_threads to the public C API.
  • Extended C-binding index wrappers to retain a ThreadPoolBuilder and rebuild the underlying thread pool on resize.
  • Enhanced ThreadPoolBuilder with thread-count querying and resize support.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
bindings/c/src/threadpool.hpp Adds threadpool kind/user-threadpool tracking plus get_threads_num() and resize() support.
bindings/c/src/svs_c.cpp Implements the new C API getter/setter wrappers.
bindings/c/src/index_builder.hpp Passes ThreadPoolBuilder into built/loaded index wrappers.
bindings/c/src/index.hpp Stores ThreadPoolBuilder inside index wrappers; adds virtual get/set thread count hooks.
bindings/c/include/svs/c_api/svs_c.h Declares and documents the new C API functions.

@rfsaliev rfsaliev force-pushed the rfsaliev/c-api-index-num-threads branch from fc0d9fa to 07a2493 Compare March 27, 2026 12:33
@rfsaliev rfsaliev requested a review from Copilot March 27, 2026 16:09
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@mihaic
Copy link
Copy Markdown
Member

mihaic commented Mar 27, 2026

@rfsaliev, when you are happy with the Copilot review, please ask Kelly for a review as well.

@rfsaliev
Copy link
Copy Markdown
Member Author

@klmckeig, can you please review the feature?
Thank you.

@mihaic
Copy link
Copy Markdown
Member

mihaic commented Apr 7, 2026

@klmckeig, we would still like your review for this PR.

@klmckeig
Copy link
Copy Markdown

klmckeig commented Apr 7, 2026

@klmckeig, we would still like your review for this PR.

@mihaic on it. will get back to you before Thursday

Copy link
Copy Markdown

@matt-welch matt-welch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perspective from downstream C API integrations

This fills a genuine gap. The current C API only allows setting the thread pool through the builder before load, which means a loaded index's parallelism is fixed for its lifetime. Having set_num_threads on the live index handle is the right approach for integrations that need to honor runtime configuration changes without tearing down and reloading the index.

Concurrency contract is the main question this raises at the integration level. Database workloads call svs_index_search() concurrently with configuration changes — a session may call set_num_threads() on the same index handle that another thread is actively searching. The PR doesn't document whether resize() is safe to call concurrently with in-flight searches, or whether the caller is expected to quiesce the index first. Making that contract explicit in the API documentation would allow integrations to make correct decisions about whether additional locking is needed on their side.

Getter reliability: get_num_threads() reads from the retained ThreadPoolBuilder rather than the live pool. For an integration tracking applied state, this means the getter can't be used to verify that a set took effect — the integration has to track the last-applied value itself. That's workable, but it limits the practical utility of the getter for anything beyond initial inspection.

These are design-level observations, not blockers. The core API shape is exactly what's needed.

@rfsaliev
Copy link
Copy Markdown
Member Author

rfsaliev commented Apr 8, 2026

Thank you @matt-welch for the review comment.

According to the concurrency issues, the comment is right. This limitation is applicable to all index modifications in all SVS APIs. We will reflected it in the main documentation in form of "SVS indices generally not thread-safe for concurrent modifications".

According to the "getter reliability", thank you for the confirmation of Copilot's comment. Will fix it soon.

Copilot AI changed the title [C-API] Add getter and setter for index threadpool size [C-API] Fix get_num_threads() to return real runtime threadpool size Apr 8, 2026
@rfsaliev rfsaliev changed the title [C-API] Fix get_num_threads() to return real runtime threadpool size [C-API] Add getter and setter for index threadpool size Apr 8, 2026
@rfsaliev rfsaliev force-pushed the rfsaliev/c-api-index-num-threads branch from 534161d to 7fdd3fb Compare April 8, 2026 14:56
@rfsaliev
Copy link
Copy Markdown
Member Author

rfsaliev commented Apr 8, 2026

@copilot I want to restore the initial PR description prior to Copilot changes made today

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 8, 2026

@copilot I want to restore the initial PR description prior to Copilot changes made today

Done — the PR description has been restored to the original content, with the get_num_threads() fix noted as an additional completed item.

Copilot AI changed the title [C-API] Add getter and setter for index threadpool size [C-API] Add getter and setter for index threadpool size Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants