Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mnorris11 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bindings/cpp/include/svs/runtime/api_defs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@

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

@rfsaliev rfsaliev Apr 30, 2026

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


Check notice on line 36 in bindings/cpp/include/svs/runtime/api_defs.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/api_defs.h#L36 

Redundant blank line at the start of a code block should be deleted. (whitespace/blank_line)
class OptionalBool {
enum class Value : int8_t { Undef = -1, True = 1, False = 0 };
Value value_;
Expand Down Expand Up @@ -206,7 +206,7 @@
return this->allocate(result_counts);
}
};

Check notice on line 209 in bindings/cpp/include/svs/runtime/api_defs.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/api_defs.h#L209 

Redundant blank line at the end of a code block should be deleted. (whitespace/blank_line)
} // namespace v0
} // namespace runtime
} // namespace svs
6 changes: 3 additions & 3 deletions bindings/cpp/include/svs/runtime/dynamic_ivf_index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@

namespace svs {
namespace runtime {
namespace v0 {
inline namespace v0 {

Check notice on line 28 in bindings/cpp/include/svs/runtime/dynamic_ivf_index.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/dynamic_ivf_index.h#L28 

Redundant blank line at the start of a code block should be deleted. (whitespace/blank_line)
/// @brief Abstract interface for dynamic IVF indices (supports add/delete).
struct SVS_RUNTIME_API DynamicIVFIndex : public IVFIndex {
/// @brief Utility function to check storage kind support.
Expand Down Expand Up @@ -69,8 +69,8 @@
/// @param x Pointer to vector data (row-major, n x dimensions).
/// @param reuse_empty Whether to reuse empty slots from deleted vectors.
/// @return Status indicating success or error.
virtual Status
add(size_t n, const size_t* labels, const float* x, bool reuse_empty = false
virtual Status add(
size_t n, const size_t* labels, const float* x, bool reuse_empty = false
) noexcept = 0;

/// @brief Remove vectors from the index by ID.
Expand Down Expand Up @@ -160,7 +160,7 @@
size_t intra_query_threads = 1
) noexcept;
};

Check notice on line 163 in bindings/cpp/include/svs/runtime/dynamic_ivf_index.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/dynamic_ivf_index.h#L163 

Redundant blank line at the end of a code block should be deleted. (whitespace/blank_line)
} // namespace v0
} // namespace runtime
} // namespace svs
6 changes: 3 additions & 3 deletions bindings/cpp/include/svs/runtime/dynamic_vamana_index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@

namespace svs {
namespace runtime {
namespace v0 {
inline namespace v0 {

Check notice on line 29 in bindings/cpp/include/svs/runtime/dynamic_vamana_index.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/dynamic_vamana_index.h#L29 

Redundant blank line at the start of a code block should be deleted. (whitespace/blank_line)
// Abstract interface for Dynamic Vamana-based indexes.
struct SVS_RUNTIME_API DynamicVamanaIndex : public VamanaIndex {
virtual Status add(size_t n, const size_t* labels, const float* x) noexcept = 0;
Expand All @@ -39,8 +39,8 @@
// Utility function to check storage kind support
static Status check_storage_kind(StorageKind storage_kind) noexcept;

static Status check_params(const VamanaIndex::DynamicIndexParams& dynamic_index_params
) noexcept;
static Status
check_params(const VamanaIndex::DynamicIndexParams& dynamic_index_params) noexcept;

// Static constructors and destructors
// ABI backward compatibility
Expand Down Expand Up @@ -131,7 +131,7 @@
const VamanaIndex::DynamicIndexParams& dynamic_index_params
) noexcept;
};

Check notice on line 134 in bindings/cpp/include/svs/runtime/dynamic_vamana_index.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/dynamic_vamana_index.h#L134 

Redundant blank line at the end of a code block should be deleted. (whitespace/blank_line)
} // namespace v0
} // namespace runtime
} // namespace svs
2 changes: 1 addition & 1 deletion bindings/cpp/include/svs/runtime/flat_index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@

namespace svs {
namespace runtime {
namespace v0 {
inline namespace v0 {

Check notice on line 27 in bindings/cpp/include/svs/runtime/flat_index.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/flat_index.h#L27 

Redundant blank line at the start of a code block should be deleted. (whitespace/blank_line)
// Abstract interface for Flat indices.
struct SVS_RUNTIME_API FlatIndex {
// Utility function to check storage kind support
Expand All @@ -45,7 +45,7 @@
virtual Status save(std::ostream& out) const noexcept = 0;
static Status load(FlatIndex** index, std::istream& in, MetricType metric) noexcept;
};

Check notice on line 48 in bindings/cpp/include/svs/runtime/flat_index.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/flat_index.h#L48 

Redundant blank line at the end of a code block should be deleted. (whitespace/blank_line)
} // namespace v0
} // namespace runtime
} // namespace svs
2 changes: 1 addition & 1 deletion bindings/cpp/include/svs/runtime/ivf_index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@

namespace svs {
namespace runtime {
namespace v0 {
inline namespace v0 {

Check notice on line 28 in bindings/cpp/include/svs/runtime/ivf_index.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/ivf_index.h#L28 

Redundant blank line at the start of a code block should be deleted. (whitespace/blank_line)
// Abstract interface for IVF (Inverted File) indices.
struct SVS_RUNTIME_API IVFIndex {
virtual ~IVFIndex();
Expand Down Expand Up @@ -151,7 +151,7 @@
size_t intra_query_threads = 1
) noexcept;
};

Check notice on line 154 in bindings/cpp/include/svs/runtime/ivf_index.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/ivf_index.h#L154 

Redundant blank line at the end of a code block should be deleted. (whitespace/blank_line)
} // namespace v0
} // namespace runtime
} // namespace svs
2 changes: 1 addition & 1 deletion bindings/cpp/include/svs/runtime/training.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@

namespace svs {
namespace runtime {
namespace v0 {
inline namespace v0 {

Check notice on line 27 in bindings/cpp/include/svs/runtime/training.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/training.h#L27 

Redundant blank line at the start of a code block should be deleted. (whitespace/blank_line)
struct SVS_RUNTIME_API LeanVecTrainingData {
virtual ~LeanVecTrainingData();

Expand Down Expand Up @@ -70,7 +70,7 @@
virtual Status save(std::ostream& out) const noexcept = 0;
static Status load(LeanVecTrainingData** training_data, std::istream& in) noexcept;
};

Check notice on line 73 in bindings/cpp/include/svs/runtime/training.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/training.h#L73 

Redundant blank line at the end of a code block should be deleted. (whitespace/blank_line)
} // namespace v0
} // namespace runtime
} // namespace svs
2 changes: 1 addition & 1 deletion bindings/cpp/include/svs/runtime/vamana_index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@

namespace svs {
namespace runtime {
namespace v0 {
inline namespace v0 {

Check notice on line 27 in bindings/cpp/include/svs/runtime/vamana_index.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/vamana_index.h#L27 

Redundant blank line at the start of a code block should be deleted. (whitespace/blank_line)
namespace detail {
struct VamanaBuildParameters {
size_t graph_max_degree = Unspecify<size_t>();
Expand Down Expand Up @@ -128,7 +128,7 @@
const VamanaIndex::SearchParams& default_search_params = {}
) noexcept;
};

Check notice on line 131 in bindings/cpp/include/svs/runtime/vamana_index.h

View check run for this annotation

codefactor.io / CodeFactor

bindings/cpp/include/svs/runtime/vamana_index.h#L131 

Redundant blank line at the end of a code block should be deleted. (whitespace/blank_line)
} // namespace v0
} // namespace runtime
} // namespace svs
4 changes: 2 additions & 2 deletions bindings/cpp/include/svs/runtime/version.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ inline namespace v0 {
///
/// @brief Version information structure for runtime queries
///
namespace svs::runtime::v0 {
namespace svs::runtime::inline v0 {

struct VersionInfo {
static constexpr int major = SVS_RUNTIME_VERSION_MAJOR;
Expand All @@ -110,4 +110,4 @@ struct VersionInfo {
}
};

} // namespace svs::runtime::v0
} // namespace svs::runtime::inline v0
40 changes: 27 additions & 13 deletions include/svs/index/ivf/index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,12 @@ class IVFIndex {
, inter_query_threadpool_{threads::as_threadpool(std::move(threadpool_proto))}
, intra_query_thread_count_{intra_query_thread_count}
, logger_{std::move(logger)} {
// Clamp thread pool: more threads than centroids causes OOB in
// compute_centroid_distances and wastes resources.
if (inter_query_threadpool_.size() > centroids_.size()) {
inter_query_threadpool_ =
InterQueryThreadPool(threads::DefaultThreadPool(centroids_.size()));
}
validate_thread_configuration();
initialize_thread_pools();
initialize_search_buffers();
Expand Down Expand Up @@ -269,7 +275,8 @@ class IVFIndex {
return scratchspace_type{
create_centroid_buffer(sp.n_probes_),
create_leaf_buffers(buffer_leaves_size),
extensions::per_thread_batch_search_setup(cluster0_, distance_)};
extensions::per_thread_batch_search_setup(cluster0_, distance_)
};
}

/// @brief Return scratch space resources for external threading with default parameters
Expand Down Expand Up @@ -540,7 +547,8 @@ class IVFIndex {
mutable std::vector<size_t> id_in_cluster_{};
// Thread-safe initialization flag for ID mapping (wrapped in unique_ptr for movability)
mutable std::unique_ptr<std::once_flag> id_mapping_init_flag_{
std::make_unique<std::once_flag>()};
std::make_unique<std::once_flag>()
};

///// Threading Infrastructure /////
InterQueryThreadPool inter_query_threadpool_; // Handles parallelism across queries
Expand All @@ -567,9 +575,11 @@ class IVFIndex {
void initialize_thread_pools() {
// Create thread pools for intra-query (cluster-level) parallelism
for (size_t i = 0; i < inter_query_threadpool_.size(); i++) {
intra_query_threadpools_.push_back(threads::ThreadPoolHandle(
threads::DefaultThreadPool(intra_query_thread_count_)
));
intra_query_threadpools_.push_back(
threads::ThreadPoolHandle(
threads::DefaultThreadPool(intra_query_thread_count_)
)
);
}
}

Expand Down Expand Up @@ -627,11 +637,13 @@ class IVFIndex {

void validate_query_batch_size(size_t query_size) const {
if (query_size > MAX_QUERY_BATCH_SIZE) {
throw std::runtime_error(fmt::format(
"Query batch size {} exceeds maximum allowed {}",
query_size,
MAX_QUERY_BATCH_SIZE
));
throw std::runtime_error(
fmt::format(
"Query batch size {} exceeds maximum allowed {}",
query_size,
MAX_QUERY_BATCH_SIZE
)
);
}
}

Expand All @@ -645,9 +657,11 @@ class IVFIndex {
std::vector<SortedBuffer<Idx, distance::compare_t<Dist>>> buffers;
buffers.reserve(intra_query_thread_count_);
for (size_t j = 0; j < intra_query_thread_count_; j++) {
buffers.push_back(SortedBuffer<Idx, distance::compare_t<Dist>>(
buffer_size, distance::comparator(distance_)
));
buffers.push_back(
SortedBuffer<Idx, distance::compare_t<Dist>>(
buffer_size, distance::comparator(distance_)
)
);
}
return buffers;
}
Expand Down
2 changes: 1 addition & 1 deletion include/svs/index/ivf/sorted_buffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ template <typename Idx, typename Cmp = std::less<>> class SortedBuffer {
/// @brief Return ``true`` if a neighbor with the given distance can be skipped.
///
bool can_skip(float distance) const {
return compare_(back().distance(), distance) && full();
return full() && compare_(back().distance(), distance);
}

///
Expand Down
6 changes: 5 additions & 1 deletion include/svs/lib/float16.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,14 @@ inline uint16_t float_to_float16_untyped_slow(const float x) {
const uint32_t b = bitcast_float_to_uint32(x) + 0x00001000;
const uint32_t e = (b & 0x7F800000) >> 23; // exponent
const uint32_t m = b & 0x007FFFFF; // mantissa
// Compute denormalized shift safely: only valid when 101 < e < 113,
// which means shift amount (125 - e) is in range [13, 23].
// Clamp e to avoid undefined behavior when e > 125 or e < 102.
const uint32_t safe_e = (e > 101 && e < 113) ? e : 112;
return (b & 0x80000000) >> 16 |
static_cast<uint32_t>(e > 112) * ((((e - 112) << 10) & 0x7C00) | m >> 13) |
static_cast<uint32_t>((e < 113) && (e > 101)) *
((((0x007FF000 + m) >> (125 - e)) + 1) >> 1) |
((((0x007FF000 + m) >> (125 - safe_e)) + 1) >> 1) |
Copy link
Copy Markdown
Member

@rfsaliev rfsaliev Apr 30, 2026

Choose a reason for hiding this comment

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

Hm, in case when (e < 101 || e > 113), this component is always 0:

return (b & 0x80000000) >> 16 |
           ... |
           static_cast<uint32_t>((e < 113) && (e > 101)) *
               ((((0x007FF000 + m) >> (125 - safe_e)) + 1) >> 1) |

// equal to:
return (b & 0x80000000) >> 16 |
           ... |
           static_cast<uint32_t>(e > 101 && e < 113) *
               ((((0x007FF000 + m) >> (125 - safe_e)) + 1) >> 1) |
// equal to:
return (b & 0x80000000) >> 16 |
           ... |
           static_cast<uint32_t>(false) *
               ((((0x007FF000 + m) >> (125 - safe_e)) + 1) >> 1) |
//equal to:
return (b & 0x80000000) >> 16 |
           ... |
           0 *
               ((((0x007FF000 + m) >> (125 - safe_e)) + 1) >> 1) |

Copy link
Copy Markdown
Member

@ibhati ibhati Apr 30, 2026

Choose a reason for hiding this comment

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

@rfsaliev please read this comment above:
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.

static_cast<uint32_t>(e > 143) *
0x7FFF; // sign : normalized : denormalized : saturate
}
Expand Down
8 changes: 5 additions & 3 deletions include/svs/lib/threads/thread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,8 +855,10 @@ template <typename T = telemetry::NoTelemetry> class ThreadImpl {
// Wait for the future to become available and rethrow the exception.
wait_for_result();
get_result();
throw ANNEXCEPTION("Expected to get an exception from a crashed thread but no "
"exception was thrown!");
throw ANNEXCEPTION(
"Expected to get an exception from a crashed thread but no "
"exception was thrown!"
);
Comment on lines +858 to +861
Copy link
Copy Markdown
Member

@rfsaliev rfsaliev Apr 30, 2026

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.

}

// Assign Work
Expand Down Expand Up @@ -888,7 +890,7 @@ template <typename T = telemetry::NoTelemetry> class ThreadImpl {
// * Catch this exception and wrap its message inside a `ThreadError`.
try {
unsafe_assign(fn);
} catch (const ThreadCrashedError& err) {
} catch (const ThreadCrashedError&) {
Copy link
Copy Markdown
Member

@rfsaliev rfsaliev Apr 30, 2026

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)) {

try {
unsafe_get_exception();
} catch (const std::exception& inner_error) { throw ThreadError{inner_error}; }
Expand Down
Loading