Skip to content

Add inline, remove unused exception variable name, safe float computation#324

Open
mnorris11 wants to merge 1 commit into
intel:mainfrom
mnorris11:main
Open

Add inline, remove unused exception variable name, safe float computation#324
mnorris11 wants to merge 1 commit into
intel:mainfrom
mnorris11:main

Conversation

@mnorris11
Copy link
Copy Markdown

@mnorris11 mnorris11 commented Apr 29, 2026

This is a patch to get it to work internally. It would be easier if things are in SVS, so next version upgrade we don't need to apply this patch.

Reason for changes:

  1. inlining: This is from -Werror,-Winline-namespace-reopened-noninline.
  2. Unused exception parameter: -Werror,-Wunused-exception-parameter.
  3. Float change: acccording to AI: the shift 125 - e goes out of range for uint32_t when e is outside [102, 112]. The result is multiplied by zero so it's "dead" at runtime, but the shift itself is still UB. UBSAN catches it and 5 FP16 tests fail: WriteAndReadIndexSVSFP16, VamanaFP16TrainSaveLoadAndAdd, WriteAndReadStaticIVFFP16, WriteAndReadIndexSVSIVFFP16, IVFFP16TrainAndAdd.
  4. index.h Thread clamping: per AI: "compute_centroid_distances (in common.h:675) partitions centroids across threads using StaticPartition(num_centroids). If you have more threads than centroids, some threads get an empty partition. The callback then calls batch.start() and batch.size() on those empty ranges, leading to OOB access into centroids.get_datum() and matmul_results[tid]. The fix clamps the thread pool size to at most the number of centroids at construction time, so every thread gets at least one centroid to work on."
  5. sorted_buffer.h change: " back() returns candidates_[size_ - 1]. When the buffer is empty (size_ == 0), that underflows to candidates_[SIZE_MAX] — an OOB read. The original code evaluated back() before checking full(), so on an empty buffer it would access garbage memory before full() returned false and short-circuited the &&. Swapping the operands ensures full() is checked first; if the buffer isn't full, back() is never called. Classic short-circuit evaluation fix."

Copy link
Copy Markdown
Member

@rfsaliev rfsaliev left a comment

Choose a reason for hiding this comment

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

Thank you @mnorris11 , for the contribution.
There are some suggestions, proposals and comments

Comment thread include/svs/lib/float16.h
try {
unsafe_assign(fn);
} catch (const ThreadCrashedError& err) {
} catch (const ThreadCrashedError&) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
} catch (const ThreadCrashedError&) {
} catch (const ThreadCrashedError& SVS_UNUSED(err)) {

Comment on lines +858 to +861
throw ANNEXCEPTION(
"Expected to get an exception from a crashed thread but no "
"exception was thrown!"
);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please, use SVS formatting rules.

Comment on lines 33 to +35
namespace svs {
namespace runtime {
namespace v0 {
inline namespace v0 {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMHO, this change will negatively affect usability and maintainability for further API major versions.
Instead, a macro can be used here which value defined in version.h depending on SVS_RUNTIME_API_VERSION, e.g.:

#if (SVS_RUNTIME_API_VERSION == 0)
...
#define SVS_API_V0 inline namespace v0
...
#endif

With usage aka:

Suggested change
namespace svs {
namespace runtime {
namespace v0 {
inline namespace v0 {
namespace svs {
namespace runtime {
SVS_API_V0 {
...
} // SVS_API_V0

rfsaliev added a commit that referenced this pull request May 12, 2026
…ids (#329)

This pull request updates the initialization logic for the inter-query
thread pool in the `IVFIndex` class to better handle cases where the
number of threads may exceed the number of centroids, and to provide
improved validation and logging. The changes introduce a new static
factory method, `make_inter_query_threadpool`, with multiple overloads
to handle different types of thread pool prototypes and to ensure the
thread pool size does not exceed the number of centroids.

Note: Change in PR is inspired by #324 made by @mnorris11

**Thread pool initialization improvements:**

* Added a generic static method `make_inter_query_threadpool` that
validates the requested thread pool size against the number of
centroids, throwing an exception if there are too many threads.
([include/svs/index/ivf/index.hR577-R629](diffhunk://#diff-1615eba8a8acc0ba0e76533aceb833421c0d885ebd224543e46490f6b8c9a50dR577-R629))
* Added a specialization for `size_t` thread pool prototypes, which
automatically reduces the thread pool size to match the number of
centroids and logs a warning if resizing is necessary. (
[include/svs/index/ivf/index.hR577-R629](diffhunk://#diff-1615eba8a8acc0ba0e76533aceb833421c0d885ebd224543e46490f6b8c9a50dR577-R629)
)
* Added a specialization for thread pool prototypes that support
resizing, which also logs a warning and resizes the thread pool if
needed.
([include/svs/index/ivf/index.hR577-R629](diffhunk://#diff-1615eba8a8acc0ba0e76533aceb833421c0d885ebd224543e46490f6b8c9a50dR577-R629))

**Integration with constructor:**

* Updated the `IVFIndex` constructor to use the new
`make_inter_query_threadpool` method, ensuring thread pool size
validation and logging are applied during initialization.
([include/svs/index/ivf/index.hL151-R153](diffhunk://#diff-1615eba8a8acc0ba0e76533aceb833421c0d885ebd224543e46490f6b8c9a50dL151-R153))

---------

Co-authored-by: mnorris11 <mnorris@meta.com>
@rfsaliev
Copy link
Copy Markdown
Member

Thank you @mnorris11 for the PR.
Unfortunately, more complicated changes had to be applied for proper fixes.
There are created 3 separate PRs based on your proposal:

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.

3 participants