From 1aa94b71895e2d6ccd15964937264899c0247329 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 3 May 2026 09:53:23 +0300 Subject: [PATCH 1/8] MOD-13837: Separate query and storage alignment requirements Split the single alignment hint carried by PreprocessorsContainerAbstract into query_alignment and storage_alignment so different SIMD kernels (e.g. SQ8 storage vs FP32 query) can declare different alignment needs. - spaces/spaces.h: add combineAlignments() helper and document the GetDistFunc asymmetric-types contract (alignment hint refers to the first / storage operand only). - spaces/IP_space.cpp, spaces/L2_space.cpp: SQ8<->FP32 and SQ8<->SQ8 dispatchers now publish per-kernel alignment hints. - spaces/computer/preprocessors.h: PreprocessorInterface preprocess() and preprocessForStorage() carry storage_alignment / query_alignment. CosinePreprocessor uses combineAlignments() for the shared-buffer path. Removed the same-size preprocess() overload from the interface. - spaces/computer/preprocessor_container.{h,cpp}: split alignment field into query_alignment and storage_alignment; legacy single-arg constructors retained as wrappers. Added getQueryAlignment() / getStorageAlignment(); getAlignment() kept as a deprecated alias returning storage_alignment. - index_factories/components/preprocessors_factory.h: extend PreprocessorsContainerParams and CreatePreprocessorsContainer overloads to accept both alignments; single-alignment overloads kept for callers that don't need the distinction. - vec_sim_index.h: DataBlocksContainer now built with getStorageAlignment(); added getQueryAlignment() / getStorageAlignment() accessors. - tests/unit/test_components.cpp, tests/unit/test_hnsw_tiered.cpp: updated mock preprocessors and fixtures to the new interface. - tests/unit/test_spaces.cpp: SelfDistanceCosine / SelfDistanceL2 for SQ8<->FP32 now disable AVX2_FMA / AVX2 / SSE4 before the no-optimization assertion, mirroring the rest of the spaces tests. --- .../components/preprocessors_factory.h | 53 +++++--- src/VecSim/spaces/IP_space.cpp | 24 ++++ src/VecSim/spaces/L2_space.cpp | 13 +- .../computer/preprocessor_container.cpp | 8 +- .../spaces/computer/preprocessor_container.h | 32 ++++- src/VecSim/spaces/computer/preprocessors.h | 103 +++++++-------- src/VecSim/spaces/spaces.h | 18 +++ src/VecSim/vec_sim_index.h | 10 +- tests/unit/test_components.cpp | 121 +++++++----------- tests/unit/test_hnsw_tiered.cpp | 21 ++- tests/unit/test_spaces.cpp | 54 ++++++++ 11 files changed, 290 insertions(+), 167 deletions(-) diff --git a/src/VecSim/index_factories/components/preprocessors_factory.h b/src/VecSim/index_factories/components/preprocessors_factory.h index c91863ea0..bd52b7341 100644 --- a/src/VecSim/index_factories/components/preprocessors_factory.h +++ b/src/VecSim/index_factories/components/preprocessors_factory.h @@ -14,24 +14,21 @@ struct PreprocessorsContainerParams { VecSimMetric metric; size_t dim; - unsigned char alignment; + unsigned char query_alignment; + unsigned char storage_alignment; size_t processed_bytes_count; }; /** * @brief Creates parameters for a preprocessors container based on the given metric, dimension, - * normalization flag, and alignment. + * normalization flag, and alignments. * * @tparam DataType The data type of the vector elements (e.g., float, int). * @param metric The similarity metric to be used (e.g., Cosine, Inner Product). * @param dim The dimensionality of the vectors. * @param is_normalized A flag indicating whether the vectors are already normalized. - * @param alignment The alignment requirement for the data. - * @return A PreprocessorsContainerParams object containing the processed parameters: - * - metric: The adjusted metric based on the input and normalization flag. - * - dim: The dimensionality of the vectors. - * - alignment: The alignment requirement for the data. - * - processed_bytes_count: The size of the processed data blob in bytes. + * @param query_alignment The alignment requirement for query blobs. + * @param storage_alignment The alignment requirement for storage blobs. * * @details * If the metric is Cosine and the data type is integral, the processed bytes count may include @@ -42,7 +39,8 @@ struct PreprocessorsContainerParams { template PreprocessorsContainerParams CreatePreprocessorsContainerParams(VecSimMetric metric, size_t dim, bool is_normalized, - unsigned char alignment) { + unsigned char query_alignment, + unsigned char storage_alignment) { // By default the processed blob size is the same as the original blob size. size_t processed_bytes_count = dim * sizeof(DataType); @@ -61,18 +59,29 @@ PreprocessorsContainerParams CreatePreprocessorsContainerParams(VecSimMetric met } return {.metric = pp_metric, .dim = dim, - .alignment = alignment, + .query_alignment = query_alignment, + .storage_alignment = storage_alignment, .processed_bytes_count = processed_bytes_count}; } +// Single-alignment overload: applies the same alignment to both query and storage (homogeneous +// case). Most existing callers use this form. +template +PreprocessorsContainerParams CreatePreprocessorsContainerParams(VecSimMetric metric, size_t dim, + bool is_normalized, + unsigned char alignment) { + return CreatePreprocessorsContainerParams(metric, dim, is_normalized, alignment, + alignment); +} + template PreprocessorsContainerAbstract * CreatePreprocessorsContainer(std::shared_ptr allocator, PreprocessorsContainerParams params) { if (params.metric == VecSimMetric_Cosine) { - auto multiPPContainer = - new (allocator) MultiPreprocessorsContainer(allocator, params.alignment); + auto multiPPContainer = new (allocator) MultiPreprocessorsContainer( + allocator, params.query_alignment, params.storage_alignment); auto cosine_preprocessor = new (allocator) CosinePreprocessor(allocator, params.dim, params.processed_bytes_count); int next_valid_pp_index = multiPPContainer->addPreprocessor(cosine_preprocessor); @@ -81,19 +90,31 @@ CreatePreprocessorsContainer(std::shared_ptr allocator, return multiPPContainer; } - return new (allocator) PreprocessorsContainerAbstract(allocator, params.alignment); + return new (allocator) PreprocessorsContainerAbstract(allocator, params.query_alignment, + params.storage_alignment); } template PreprocessorsContainerAbstract * CreatePreprocessorsContainer(std::shared_ptr allocator, VecSimMetric metric, - size_t dim, bool is_normalized, unsigned char alignment) { + size_t dim, bool is_normalized, unsigned char query_alignment, + unsigned char storage_alignment) { - PreprocessorsContainerParams ppParams = - CreatePreprocessorsContainerParams(metric, dim, is_normalized, alignment); + PreprocessorsContainerParams ppParams = CreatePreprocessorsContainerParams( + metric, dim, is_normalized, query_alignment, storage_alignment); return CreatePreprocessorsContainer(allocator, ppParams); } +// Single-alignment overload: applies the same alignment to both query and storage (homogeneous +// case). Most existing callers use this form. +template +PreprocessorsContainerAbstract * +CreatePreprocessorsContainer(std::shared_ptr allocator, VecSimMetric metric, + size_t dim, bool is_normalized, unsigned char alignment) { + return CreatePreprocessorsContainer(allocator, metric, dim, is_normalized, alignment, + alignment); +} + template size_t EstimatePreprocessorsContainerMemory(VecSimMetric metric, bool is_normalized = false) { size_t allocations_overhead = VecSimAllocator::getAllocationOverheadSize(); diff --git a/src/VecSim/spaces/IP_space.cpp b/src/VecSim/spaces/IP_space.cpp index 859b90271..d15009495 100644 --- a/src/VecSim/spaces/IP_space.cpp +++ b/src/VecSim/spaces/IP_space.cpp @@ -70,23 +70,32 @@ dist_func_t IP_SQ8_FP32_GetDistFunc(size_t dim, unsigned char *alignment, if (dim < 16) { return ret_dist_func; } + // Alignment hints below refer to the SQ8 (first) operand per the GetDistFunc contract. #ifdef OPT_AVX512_F_BW_VL_VNNI if (features.avx512f && features.avx512bw && features.avx512vnni) { + if (dim % 16 == 0) // SQ8 chunk = 16 bytes + *alignment = 16 * sizeof(uint8_t); return Choose_SQ8_FP32_IP_implementation_AVX512F_BW_VL_VNNI(dim); } #endif #ifdef OPT_AVX2_FMA if (features.avx2 && features.fma3) { + if (dim % 8 == 0) // SQ8 chunk = 8 bytes + *alignment = 8 * sizeof(uint8_t); return Choose_SQ8_FP32_IP_implementation_AVX2_FMA(dim); } #endif #ifdef OPT_AVX2 if (features.avx2) { + if (dim % 8 == 0) // SQ8 chunk = 8 bytes + *alignment = 8 * sizeof(uint8_t); return Choose_SQ8_FP32_IP_implementation_AVX2(dim); } #endif #ifdef OPT_SSE4 if (features.sse4_1) { + if (dim % 4 == 0) // SQ8 chunk = 4 bytes + *alignment = 4 * sizeof(uint8_t); return Choose_SQ8_FP32_IP_implementation_SSE4(dim); } #endif @@ -129,23 +138,32 @@ dist_func_t Cosine_SQ8_FP32_GetDistFunc(size_t dim, unsigned char *alignm if (dim < 16) { return ret_dist_func; } + // Alignment hints below refer to the SQ8 (first) operand per the GetDistFunc contract. #ifdef OPT_AVX512_F_BW_VL_VNNI if (features.avx512f && features.avx512bw && features.avx512vnni) { + if (dim % 16 == 0) // SQ8 chunk = 16 bytes + *alignment = 16 * sizeof(uint8_t); return Choose_SQ8_FP32_Cosine_implementation_AVX512F_BW_VL_VNNI(dim); } #endif #ifdef OPT_AVX2_FMA if (features.avx2 && features.fma3) { + if (dim % 8 == 0) // SQ8 chunk = 8 bytes + *alignment = 8 * sizeof(uint8_t); return Choose_SQ8_FP32_Cosine_implementation_AVX2_FMA(dim); } #endif #ifdef OPT_AVX2 if (features.avx2) { + if (dim % 8 == 0) // SQ8 chunk = 8 bytes + *alignment = 8 * sizeof(uint8_t); return Choose_SQ8_FP32_Cosine_implementation_AVX2(dim); } #endif #ifdef OPT_SSE4 if (features.sse4_1) { + if (dim % 4 == 0) // SQ8 chunk = 4 bytes + *alignment = 4 * sizeof(uint8_t); return Choose_SQ8_FP32_Cosine_implementation_SSE4(dim); } #endif @@ -190,7 +208,10 @@ dist_func_t IP_SQ8_SQ8_GetDistFunc(size_t dim, unsigned char *alignment, #ifdef CPU_FEATURES_ARCH_X86_64 #ifdef OPT_AVX512_F_BW_VL_VNNI + // AVX512 VNNI SQ8_SQ8 uses 64-element chunks; residual handling is in 32-byte sub-chunks. if (dim >= 64 && features.avx512f && features.avx512bw && features.avx512vnni) { + if (dim % 32 == 0) // align to 256 bits when there is no offsetting residual + *alignment = 32 * sizeof(uint8_t); return Choose_SQ8_SQ8_IP_implementation_AVX512F_BW_VL_VNNI(dim); } #endif @@ -234,7 +255,10 @@ dist_func_t Cosine_SQ8_SQ8_GetDistFunc(size_t dim, unsigned char *alignme #ifdef CPU_FEATURES_ARCH_X86_64 #ifdef OPT_AVX512_F_BW_VL_VNNI + // AVX512 VNNI SQ8_SQ8 uses 64-element chunks; residual handling is in 32-byte sub-chunks. if (dim >= 64 && features.avx512f && features.avx512bw && features.avx512vnni) { + if (dim % 32 == 0) // align to 256 bits when there is no offsetting residual + *alignment = 32 * sizeof(uint8_t); return Choose_SQ8_SQ8_Cosine_implementation_AVX512F_BW_VL_VNNI(dim); } #endif diff --git a/src/VecSim/spaces/L2_space.cpp b/src/VecSim/spaces/L2_space.cpp index dcccd513f..f111a1a6a 100644 --- a/src/VecSim/spaces/L2_space.cpp +++ b/src/VecSim/spaces/L2_space.cpp @@ -70,23 +70,32 @@ dist_func_t L2_SQ8_FP32_GetDistFunc(size_t dim, unsigned char *alignment, if (dim < 16) { return ret_dist_func; } + // Alignment hints below refer to the SQ8 (first) operand per the GetDistFunc contract. #ifdef OPT_AVX512_F_BW_VL_VNNI if (features.avx512f && features.avx512bw && features.avx512vnni) { + if (dim % 16 == 0) // SQ8 chunk = 16 bytes; no point in aligning if there's a residual + *alignment = 16 * sizeof(uint8_t); return Choose_SQ8_FP32_L2_implementation_AVX512F_BW_VL_VNNI(dim); } #endif #ifdef OPT_AVX2_FMA if (features.avx2 && features.fma3) { + if (dim % 8 == 0) // SQ8 chunk = 8 bytes + *alignment = 8 * sizeof(uint8_t); return Choose_SQ8_FP32_L2_implementation_AVX2_FMA(dim); } #endif #ifdef OPT_AVX2 if (features.avx2) { + if (dim % 8 == 0) // SQ8 chunk = 8 bytes + *alignment = 8 * sizeof(uint8_t); return Choose_SQ8_FP32_L2_implementation_AVX2(dim); } #endif #ifdef OPT_SSE4 if (features.sse4_1) { + if (dim % 4 == 0) // SQ8 chunk = 4 bytes + *alignment = 4 * sizeof(uint8_t); return Choose_SQ8_FP32_L2_implementation_SSE4(dim); } #endif @@ -456,8 +465,10 @@ dist_func_t L2_SQ8_SQ8_GetDistFunc(size_t dim, unsigned char *alignment, #ifdef CPU_FEATURES_ARCH_X86_64 #ifdef OPT_AVX512_F_BW_VL_VNNI - // AVX512 VNNI SQ8_SQ8 uses 64-element chunks + // AVX512 VNNI SQ8_SQ8 uses 64-element chunks; residual handling is in 32-byte sub-chunks. if (dim >= 64 && features.avx512f && features.avx512bw && features.avx512vnni) { + if (dim % 32 == 0) // align to 256 bits when there is no offsetting residual + *alignment = 32 * sizeof(uint8_t); return Choose_SQ8_SQ8_L2_implementation_AVX512F_BW_VL_VNNI(dim); } #endif diff --git a/src/VecSim/spaces/computer/preprocessor_container.cpp b/src/VecSim/spaces/computer/preprocessor_container.cpp index 37d678206..94092f61b 100644 --- a/src/VecSim/spaces/computer/preprocessor_container.cpp +++ b/src/VecSim/spaces/computer/preprocessor_container.cpp @@ -31,11 +31,13 @@ void PreprocessorsContainerAbstract::preprocessStorageInPlace(void *blob, MemoryUtils::unique_blob PreprocessorsContainerAbstract::maybeCopyToAlignedMem( const void *original_blob, size_t input_blob_size, bool force_copy) const { - bool needs_copy = - force_copy || (this->alignment && ((uintptr_t)original_blob % this->alignment != 0)); + // This helper aligns query buffers; storage allocation paths use storage_alignment elsewhere. + bool needs_copy = force_copy || (this->query_alignment && + ((uintptr_t)original_blob % this->query_alignment != 0)); if (needs_copy) { - auto aligned_mem = this->allocator->allocate_aligned(input_blob_size, this->alignment); + auto aligned_mem = + this->allocator->allocate_aligned(input_blob_size, this->query_alignment); memcpy(aligned_mem, original_blob, input_blob_size); return this->wrapAllocated(aligned_mem); } diff --git a/src/VecSim/spaces/computer/preprocessor_container.h b/src/VecSim/spaces/computer/preprocessor_container.h index 454504bb3..90b7d8d88 100644 --- a/src/VecSim/spaces/computer/preprocessor_container.h +++ b/src/VecSim/spaces/computer/preprocessor_container.h @@ -19,9 +19,17 @@ struct ProcessedBlobs; class PreprocessorsContainerAbstract : public VecsimBaseObject { public: + // Legacy ctor: same value applies to both query and storage alignment (homogeneous case). PreprocessorsContainerAbstract(std::shared_ptr allocator, unsigned char alignment) - : VecsimBaseObject(allocator), alignment(alignment) {} + : PreprocessorsContainerAbstract(allocator, alignment, alignment) {} + + PreprocessorsContainerAbstract(std::shared_ptr allocator, + unsigned char query_alignment, + unsigned char storage_alignment) + : VecsimBaseObject(allocator), query_alignment(query_alignment), + storage_alignment(storage_alignment) {} + // It is assumed that the resulted query blob is aligned. virtual ProcessedBlobs preprocess(const void *original_blob, size_t input_blob_size) const; @@ -35,10 +43,14 @@ class PreprocessorsContainerAbstract : public VecsimBaseObject { virtual void preprocessStorageInPlace(void *blob, size_t input_blob_size) const; - unsigned char getAlignment() const { return alignment; } + unsigned char getQueryAlignment() const { return query_alignment; } + unsigned char getStorageAlignment() const { return storage_alignment; } + // TODO(MOD-13837): remove after callers migrate to getStorageAlignment / getQueryAlignment. + unsigned char getAlignment() const { return storage_alignment; } protected: - const unsigned char alignment; + const unsigned char query_alignment; + const unsigned char storage_alignment; // Allocate and copy the blob only if the original blob is not aligned. MemoryUtils::unique_blob maybeCopyToAlignedMem(const void *original_blob, @@ -61,8 +73,13 @@ class MultiPreprocessorsContainer : public PreprocessorsContainerAbstract { std::array preprocessors; public: + // Legacy ctor: same value applies to both query and storage alignment (homogeneous case). MultiPreprocessorsContainer(std::shared_ptr allocator, unsigned char alignment) - : PreprocessorsContainerAbstract(allocator, alignment) { + : MultiPreprocessorsContainer(allocator, alignment, alignment) {} + + MultiPreprocessorsContainer(std::shared_ptr allocator, + unsigned char query_alignment, unsigned char storage_alignment) + : PreprocessorsContainerAbstract(allocator, query_alignment, storage_alignment) { assert(n_preprocessors); std::fill_n(preprocessors.begin(), n_preprocessors, nullptr); } @@ -178,7 +195,7 @@ MultiPreprocessorsContainer::preprocess(const void *o if (!pp) break; pp->preprocess(original_blob, storage_blob, query_blob, storage_blob_size, query_blob_size, - this->alignment); + this->storage_alignment, this->query_alignment); } // At least one blob was allocated. @@ -214,7 +231,8 @@ MultiPreprocessorsContainer::preprocessForStorage( for (auto pp : preprocessors) { if (!pp) break; - pp->preprocessForStorage(original_blob, storage_blob, input_blob_size); + pp->preprocessForStorage(original_blob, storage_blob, input_blob_size, + this->storage_alignment); } return storage_blob ? std::move(this->wrapAllocated(storage_blob)) @@ -230,7 +248,7 @@ MemoryUtils::unique_blob MultiPreprocessorsContainer: if (!pp) break; // modifies the memory in place - pp->preprocessQuery(original_blob, query_blob, input_blob_size, this->alignment); + pp->preprocessQuery(original_blob, query_blob, input_blob_size, this->query_alignment); } return query_blob ? std::move(this->wrapAllocated(query_blob)) diff --git a/src/VecSim/spaces/computer/preprocessors.h b/src/VecSim/spaces/computer/preprocessors.h index 5954b3fc1..229e4922a 100644 --- a/src/VecSim/spaces/computer/preprocessors.h +++ b/src/VecSim/spaces/computer/preprocessors.h @@ -24,18 +24,20 @@ class PreprocessorInterface : public VecsimBaseObject { public: PreprocessorInterface(std::shared_ptr allocator) : VecsimBaseObject(allocator) {} - // Note: input_blob_size is relevant for both storage blob and query blob, as we assume results - // are the same size. - // Use the overload below for different sizes. - virtual void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, - size_t &input_blob_size, unsigned char alignment) const = 0; + // Combined preprocessing into both storage and query blobs. storage_alignment applies to any + // newly allocated storage blob; query_alignment applies to any newly allocated query blob. + // Implementations that allocate a single shared buffer for both must align it to satisfy both + // requirements (use combineAlignments). virtual void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char alignment) const = 0; + unsigned char storage_alignment, + unsigned char query_alignment) const = 0; virtual void preprocessForStorage(const void *original_blob, void *&storage_blob, - size_t &input_blob_size) const = 0; + size_t &input_blob_size, + unsigned char storage_alignment) const = 0; virtual void preprocessQuery(const void *original_blob, void *&query_blob, - size_t &input_blob_size, unsigned char alignment) const = 0; + size_t &input_blob_size, + unsigned char query_alignment) const = 0; virtual void preprocessStorageInPlace(void *original_blob, size_t input_blob_size) const = 0; }; @@ -51,66 +53,55 @@ class CosinePreprocessor : public PreprocessorInterface { void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char alignment) const override { - // This assert verifies that the current use of this function is for blobs of the same - // size, which is the case for the Cosine preprocessor. If we ever need to support different - // sizes for storage and query blobs, we can remove the assert and implement the logic to - // handle different sizes. + unsigned char storage_alignment, + unsigned char query_alignment) const override { + // CosinePreprocessor produces equally-sized storage and query blobs. assert(storage_blob_size == query_blob_size); - - preprocess(original_blob, storage_blob, query_blob, storage_blob_size, alignment); - // Ensure both blobs have the same size after processing. - query_blob_size = storage_blob_size; - } - - void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, - size_t &input_blob_size, unsigned char alignment) const override { - // This assert verifies that if a blob was allocated by a previous preprocessor, its - // size matches our expected processed size. Therefore, it is safe to skip re-allocation and - // process it inplace. Supporting dynamic resizing would require additional size checks (if - // statements) and memory management logic, which could impact performance. Currently, no - // code path requires this capability. If resizing becomes necessary in the future, remove - // the assertions and implement appropriate allocation handling with performance - // considerations. - assert(storage_blob == nullptr || input_blob_size == processed_bytes_count); - assert(query_blob == nullptr || input_blob_size == processed_bytes_count); + // see assert docs below + assert(storage_blob == nullptr || storage_blob_size == processed_bytes_count); + assert(query_blob == nullptr || query_blob_size == processed_bytes_count); // Case 1: Blobs are different (one might be null, or both are allocated and processed // separately). if (storage_blob != query_blob) { // If one of them is null, allocate memory for it and copy the original_blob to it. if (storage_blob == nullptr) { - storage_blob = this->allocator->allocate(processed_bytes_count); - memcpy(storage_blob, original_blob, input_blob_size); + storage_blob = allocateBlob(processed_bytes_count, storage_alignment); + memcpy(storage_blob, original_blob, storage_blob_size); } else if (query_blob == nullptr) { - query_blob = this->allocator->allocate_aligned(processed_bytes_count, alignment); - memcpy(query_blob, original_blob, input_blob_size); + query_blob = allocateBlob(processed_bytes_count, query_alignment); + memcpy(query_blob, original_blob, query_blob_size); } // Normalize both blobs. normalize_func(storage_blob, this->dim); normalize_func(query_blob, this->dim); } else { // Case 2: Blobs are the same (either both are null or processed in the same way). - if (query_blob == nullptr) { // If both blobs are null, allocate query_blob and set - // storage_blob to point to it. - query_blob = this->allocator->allocate_aligned(processed_bytes_count, alignment); - memcpy(query_blob, original_blob, input_blob_size); + if (query_blob == nullptr) { + // Single buffer must satisfy both the storage and the query alignment hint. + const unsigned char shared_alignment = + spaces::combineAlignments(storage_alignment, query_alignment); + query_blob = allocateBlob(processed_bytes_count, shared_alignment); + memcpy(query_blob, original_blob, storage_blob_size); storage_blob = query_blob; } // normalize one of them (since they point to the same memory). normalize_func(query_blob, this->dim); } - input_blob_size = processed_bytes_count; + storage_blob_size = processed_bytes_count; + query_blob_size = processed_bytes_count; } - void preprocessForStorage(const void *original_blob, void *&blob, - size_t &input_blob_size) const override { - // see assert docs in preprocess + void preprocessForStorage(const void *original_blob, void *&blob, size_t &input_blob_size, + unsigned char storage_alignment) const override { + // The asserts here verify that if a blob was allocated by a previous preprocessor, its size + // matches our expected processed size, allowing in-place normalization. Dynamic resizing is + // intentionally not supported (see commit history for rationale). assert(blob == nullptr || input_blob_size == processed_bytes_count); if (blob == nullptr) { - blob = this->allocator->allocate(processed_bytes_count); + blob = allocateBlob(processed_bytes_count, storage_alignment); memcpy(blob, original_blob, input_blob_size); } normalize_func(blob, this->dim); @@ -136,6 +127,12 @@ class CosinePreprocessor : public PreprocessorInterface { } private: + // Allocate a blob, honoring alignment when non-zero. + void *allocateBlob(size_t size, unsigned char alignment) const { + return alignment ? this->allocator->allocate_aligned(size, alignment) + : this->allocator->allocate(size); + } + spaces::normalizeVector_f normalize_func; const size_t dim; const size_t processed_bytes_count; @@ -369,12 +366,6 @@ class QuantPreprocessor : public PreprocessorInterface { "QuantPreprocessor only supports floating-point types"); } - void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, - size_t &input_blob_size, unsigned char alignment) const override { - assert(false && - "QuantPreprocessor does not support identical size for storage and query blobs"); - } - /** * Preprocesses the original blob into separate storage and query blobs. * @@ -396,7 +387,8 @@ class QuantPreprocessor : public PreprocessorInterface { */ void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char alignment) const override { + unsigned char storage_alignment, + unsigned char query_alignment) const override { // CASE 1: STORAGE BLOB NEEDS ALLOCATION - the only implemented case assert(!storage_blob && "CASE 1: storage_blob must be nullptr"); assert(!query_blob && "CASE 1: query_blob must be nullptr"); @@ -414,14 +406,17 @@ class QuantPreprocessor : public PreprocessorInterface { // We can quantize the storage blob in-place (if we already checked storage_blob_size is // sufficient) - preprocessForStorage(original_blob, storage_blob, storage_blob_size); - preprocessQuery(original_blob, query_blob, query_blob_size, alignment); + preprocessForStorage(original_blob, storage_blob, storage_blob_size, storage_alignment); + preprocessQuery(original_blob, query_blob, query_blob_size, query_alignment); } - void preprocessForStorage(const void *original_blob, void *&blob, - size_t &input_blob_size) const override { + void preprocessForStorage(const void *original_blob, void *&blob, size_t &input_blob_size, + unsigned char storage_alignment) const override { assert(!blob && "storage_blob must be nullptr"); + // Storage alignment hint is plumbed but not yet honored here; aligned storage allocation + // lands in a dedicated commit (see MOD-13837 plan). + (void)storage_alignment; blob = this->allocator->allocate(storage_bytes_count); // Cast to appropriate types const DataType *input = static_cast(original_blob); diff --git a/src/VecSim/spaces/spaces.h b/src/VecSim/spaces/spaces.h index 982d3f749..11b0f9801 100644 --- a/src/VecSim/spaces/spaces.h +++ b/src/VecSim/spaces/spaces.h @@ -11,6 +11,8 @@ #include "VecSim/vec_sim_common.h" // enum VecSimMetric #include "space_includes.h" +#include + namespace spaces { template @@ -20,9 +22,25 @@ using dist_func_t = RET_TYPE (*)(const void *, const void *, size_t); // and dimension. The returned function has the signature: dist(VecType1*, VecType2*, size_t) -> // DistType. VecType2 defaults to VecType1 when both vectors are of the same type. The alignment // hint is set based on the chosen implementation and available optimizations. +// +// Asymmetric-types contract (e.g. VecType1 = SQ8 storage, VecType2 = FP32 query): +// The returned alignment hint refers to the FIRST operand only (the storage operand). +// The query operand alignment is governed by the symmetric query-type dispatcher +// (e.g. GetDistFunc). Callers that need both operand alignments must +// query both dispatchers and combine the results with combineAlignments(). template dist_func_t GetDistFunc(VecSimMetric metric, size_t dim, unsigned char *alignment); +// Combine two alignment hints into the strictest requirement that satisfies both. +// Each input must be a power of two or zero (zero means "no alignment requirement"). +// The result is the maximum of the two, which for power-of-two values is also the LCM +// and therefore the smallest alignment that simultaneously satisfies both consumers. +static inline unsigned char combineAlignments(unsigned char a, unsigned char b) { + assert((a == 0 || (a & (a - 1)) == 0) && "alignment must be a power of two or zero"); + assert((b == 0 || (b & (b - 1)) == 0) && "alignment must be a power of two or zero"); + return a > b ? a : b; +} + template using normalizeVector_f = void (*)(void *input_vector, const size_t dim); diff --git a/src/VecSim/vec_sim_index.h b/src/VecSim/vec_sim_index.h index e5a5183b7..122b21a41 100644 --- a/src/VecSim/vec_sim_index.h +++ b/src/VecSim/vec_sim_index.h @@ -133,8 +133,12 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { assert(VecSimType_sizeof(vecType)); assert(storedDataSize); assert(inputBlobSize); - this->vectors = new (this->allocator) DataBlocksContainer( - this->blockSize, this->storedDataSize, this->allocator, this->getAlignment()); + // DataBlocksContainer holds the persistent storage vectors, so it must honor the storage + // alignment hint (not the query alignment). Today this only aligns the block-base address; + // per-element stride padding is a follow-up (see MOD-13837). + this->vectors = new (this->allocator) + DataBlocksContainer(this->blockSize, this->storedDataSize, this->allocator, + this->getStorageAlignment()); } /** @@ -203,6 +207,8 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { inline size_t getInputBlobSize() const { return inputBlobSize; } inline size_t getBlockSize() const { return blockSize; } inline auto getAlignment() const { return this->preprocessors->getAlignment(); } + inline auto getQueryAlignment() const { return this->preprocessors->getQueryAlignment(); } + inline auto getStorageAlignment() const { return this->preprocessors->getStorageAlignment(); } virtual inline VecSimIndexStatsInfo statisticInfo() const override { return VecSimIndexStatsInfo{ diff --git a/tests/unit/test_components.cpp b/tests/unit/test_components.cpp index 2f91d2411..aab10be2c 100644 --- a/tests/unit/test_components.cpp +++ b/tests/unit/test_components.cpp @@ -72,22 +72,15 @@ class DummyStoragePreprocessor : public PreprocessorInterface { } void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char alignment) const override { - // This assert verifies that there's no use for this function for now - different sizes for - // storage and query blobs. If such a use case arises, we can remove the assert and - // implement the logic to handle different sizes. + unsigned char storage_alignment, + unsigned char query_alignment) const override { assert(storage_blob_size == query_blob_size); - - preprocess(original_blob, storage_blob, query_blob, storage_blob_size, alignment); - } - void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, - size_t &input_blob_size, unsigned char alignment) const override { - - this->preprocessForStorage(original_blob, storage_blob, input_blob_size); + this->preprocessForStorage(original_blob, storage_blob, storage_blob_size, + storage_alignment); } - void preprocessForStorage(const void *original_blob, void *&blob, - size_t &input_blob_size) const override { + void preprocessForStorage(const void *original_blob, void *&blob, size_t &input_blob_size, + unsigned char storage_alignment) const override { // If the blob was not allocated yet, allocate it. if (blob == nullptr) { blob = this->allocator->allocate(input_blob_size); @@ -125,22 +118,14 @@ class DummyQueryPreprocessor : public PreprocessorInterface { void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char alignment) const override { - // This assert verifies that there's no use for this function for now - different sizes for - // storage and query blobs. If such a use case arises, we can remove the assert and - // implement the logic to handle different sizes. + unsigned char storage_alignment, + unsigned char query_alignment) const override { assert(storage_blob_size == query_blob_size); - - preprocess(original_blob, storage_blob, query_blob, storage_blob_size, alignment); + this->preprocessQuery(original_blob, query_blob, query_blob_size, query_alignment); } - void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, - size_t &input_blob_size, unsigned char alignment) const override { - this->preprocessQuery(original_blob, query_blob, input_blob_size, alignment); - } - - void preprocessForStorage(const void *original_blob, void *&blob, - size_t &input_blob_size) const override { + void preprocessForStorage(const void *original_blob, void *&blob, size_t &input_blob_size, + unsigned char storage_alignment) const override { /* do nothing*/ } @@ -170,29 +155,27 @@ class DummyMixedPreprocessor : public PreprocessorInterface { value_to_add_query(value_to_add_query) {} void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char alignment) const override { - preprocess(original_blob, storage_blob, query_blob, storage_blob_size, alignment); - } - - void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, - size_t &input_blob_size, unsigned char alignment) const override { + unsigned char storage_alignment, + unsigned char query_alignment) const override { + assert(storage_blob_size == query_blob_size); // One blob was already allocated by a previous preprocessor(s) that process both blobs the // same. The blobs are pointing to the same memory, we need to allocate another memory slot // to split them. if ((storage_blob == query_blob) && (query_blob != nullptr)) { - storage_blob = this->allocator->allocate(input_blob_size); - memcpy(storage_blob, query_blob, input_blob_size); + storage_blob = this->allocator->allocate(storage_blob_size); + memcpy(storage_blob, query_blob, storage_blob_size); } // Either both are nullptr or they are pointing to different memory slots. Both cases are // handled by the designated functions. - this->preprocessForStorage(original_blob, storage_blob, input_blob_size); - this->preprocessQuery(original_blob, query_blob, input_blob_size, alignment); + this->preprocessForStorage(original_blob, storage_blob, storage_blob_size, + storage_alignment); + this->preprocessQuery(original_blob, query_blob, query_blob_size, query_alignment); } - void preprocessForStorage(const void *original_blob, void *&blob, - size_t &input_blob_size) const override { + void preprocessForStorage(const void *original_blob, void *&blob, size_t &input_blob_size, + unsigned char storage_alignment) const override { // If the blob was not allocated yet, allocate it. if (blob == nullptr) { blob = this->allocator->allocate(input_blob_size); @@ -234,27 +217,22 @@ class DummyChangeAllocSizePreprocessor : public PreprocessorInterface { void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char alignment) const override { - // if the blobs are equal, + unsigned char storage_alignment, + unsigned char query_alignment) const override { + // if the blobs are equal, allocate a single shared buffer aligned to satisfy both hints. if (storage_blob == query_blob) { - preprocessGeneral(original_blob, storage_blob, storage_blob_size, alignment); + assert(storage_blob_size == query_blob_size); + const unsigned char shared_alignment = + spaces::combineAlignments(storage_alignment, query_alignment); + preprocessGeneral(original_blob, storage_blob, storage_blob_size, shared_alignment); query_blob = storage_blob; query_blob_size = storage_blob_size; - } - } - - // If the input blob size is not enough - void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, - size_t &input_blob_size, unsigned char alignment) const override { - // if the blobs are equal, - if (storage_blob == query_blob) { - preprocessGeneral(original_blob, storage_blob, input_blob_size, alignment); - query_blob = storage_blob; return; } // The blobs are not equal - auto alloc_and_process = [&](void *&blob) { + auto alloc_and_process = [&](void *&blob, size_t &input_blob_size, + unsigned char alignment) { // If the input blob size is not enough if (input_blob_size < processed_bytes_count) { auto new_blob = this->allocator->allocate_aligned(processed_bytes_count, alignment); @@ -277,19 +255,17 @@ class DummyChangeAllocSizePreprocessor : public PreprocessorInterface { input_blob_size - processed_bytes_count); } } + input_blob_size = processed_bytes_count; }; - alloc_and_process(storage_blob); - alloc_and_process(query_blob); - - // update the input blob size - input_blob_size = processed_bytes_count; + alloc_and_process(storage_blob, storage_blob_size, storage_alignment); + alloc_and_process(query_blob, query_blob_size, query_alignment); } - void preprocessForStorage(const void *original_blob, void *&blob, - size_t &input_blob_size) const override { + void preprocessForStorage(const void *original_blob, void *&blob, size_t &input_blob_size, + unsigned char storage_alignment) const override { - this->preprocessGeneral(original_blob, blob, input_blob_size); + this->preprocessGeneral(original_blob, blob, input_blob_size, storage_alignment); } void preprocessStorageInPlace(void *blob, size_t input_blob_size) const override { @@ -344,7 +320,7 @@ TEST(PreprocessorsTest, PreprocessorsTestBasicAlignmentTest) { using namespace dummyPreprocessors; std::shared_ptr allocator = VecSimAllocator::newVecsimAllocator(); - unsigned char alignment = 5; + unsigned char alignment = 8; auto preprocessor = PreprocessorsContainerAbstract(allocator, alignment); const int original_blob[4] = {1, 1, 1, 1}; size_t processed_bytes_count = sizeof(original_blob); @@ -551,7 +527,7 @@ void multiPPContainerAlignment(dummyPreprocessors::pp_mode MODE) { using namespace dummyPreprocessors; std::shared_ptr allocator = VecSimAllocator::newVecsimAllocator(); - unsigned char alignment = 5; + unsigned char alignment = 8; constexpr size_t n_preprocessors = 1; int initial_value = 1; int value_to_add = 7; @@ -609,7 +585,7 @@ TEST(PreprocessorsTest, multiPPContainerCosineThenMixedPreprocess) { constexpr size_t n_preprocessors = 2; constexpr size_t dim = 4; - unsigned char alignment = 5; + unsigned char alignment = 8; float initial_value = 1.0f; float normalized_value = 0.5f; @@ -677,7 +653,7 @@ TEST(PreprocessorsTest, multiPPContainerMixedThenCosinePreprocess) { constexpr size_t n_preprocessors = 2; constexpr size_t dim = 4; - unsigned char alignment = 5; + unsigned char alignment = 8; // In this test the first preprocessor allocates the memory for both blobs, according to the // size passed by the pp container. The second preprocessor expects that if the blobs are @@ -749,7 +725,7 @@ TEST(PreprocessorsTest, multiPPContainerMixedThenCosinePreprocess) { ProcessedBlobs processed_blobs = multiPPContainer.preprocess( original_blob, normalized_blob_bytes_count - sizeof(float)); }, - testing::KilledBySignal(SIGABRT), "input_blob_size == processed_bytes_count"); + testing::KilledBySignal(SIGABRT), "blob_size == processed_bytes_count"); #endif // Use the correct size ProcessedBlobs processed_blobs = @@ -786,7 +762,7 @@ void AsymmetricPPThenCosine(dummyPreprocessors::pp_mode MODE) { constexpr size_t n_preprocessors = 2; constexpr size_t dim = 4; - unsigned char alignment = 5; + unsigned char alignment = 8; float original_blob[dim] = {0}; constexpr size_t original_blob_size = dim * sizeof(float); @@ -869,7 +845,7 @@ TEST(PreprocessorsTest, DecreaseSizeThenFloatNormalize) { std::shared_ptr allocator = VecSimAllocator::newVecsimAllocator(); constexpr size_t n_preprocessors = 2; - constexpr size_t alignment = 5; + constexpr size_t alignment = 8; constexpr size_t elements = 8; constexpr size_t decrease_amount = 2; constexpr size_t new_elem_amount = elements - decrease_amount; @@ -929,7 +905,7 @@ TEST(PreprocessorsTest, Int8NormalizeThenIncreaseSize) { std::shared_ptr allocator = VecSimAllocator::newVecsimAllocator(); constexpr size_t n_preprocessors = 2; - constexpr size_t alignment = 5; + constexpr size_t alignment = 8; constexpr size_t elements = 7; // valgrind detects out of bound reads only if the considered memory is allocated on the heap, @@ -995,7 +971,7 @@ TEST(PreprocessorsTest, Int8NormalizeThenIncreaseSize) { TEST(PreprocessorsTest, QuantizationTest) { std::shared_ptr allocator = VecSimAllocator::newVecsimAllocator(); constexpr size_t n_preprocessors = 1; - constexpr size_t alignment = 5; + constexpr size_t alignment = 8; constexpr size_t dim = 6; constexpr size_t original_blob_size = dim * sizeof(float); float original_blob[dim] = {1, 2, 3, 4, 5, 6}; @@ -1113,7 +1089,7 @@ TEST(PreprocessorsTest, QuantizationTestAllEntriesEqual) { size_t query_blob_size = dim * sizeof(float); quant_preprocessor->preprocess(original_blob, storage_blob, query_blob, storage_blob_size, - query_blob_size, alignment); + query_blob_size, alignment, alignment); ASSERT_NE(storage_blob, nullptr); @@ -1200,7 +1176,8 @@ class QuantPreprocessorMetricTest : public testing::TestWithParam size_t query_blob_size = original_blob_size; quant_preprocessor->preprocess(original_blob, storage_blob, query_blob, - storage_blob_size, query_blob_size, alignment); + storage_blob_size, query_blob_size, alignment, + alignment); // Verify storage blob ASSERT_NE(storage_blob, nullptr); @@ -1242,7 +1219,7 @@ class QuantPreprocessorMetricTest : public testing::TestWithParam void *blob = nullptr; size_t blob_size = original_blob_size; - quant_preprocessor->preprocessForStorage(original_blob, blob, blob_size); + quant_preprocessor->preprocessForStorage(original_blob, blob, blob_size, alignment); ASSERT_EQ(blob_size, expected_storage_size); allocator->free_allocation(blob); diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index ffcac02ea..7d83a58cb 100644 --- a/tests/unit/test_hnsw_tiered.cpp +++ b/tests/unit/test_hnsw_tiered.cpp @@ -4246,33 +4246,30 @@ class PreprocessorDoubleValue : public PreprocessorInterface { void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char alignment) const override { + unsigned char storage_alignment, + unsigned char query_alignment) const override { // This assert makes sure the current use of the preprocessor is valid, // i.e., both blobs are of the same size. // In order to use different sizes, the preprocessor should be modified. assert(storage_blob_size == query_blob_size); - preprocess(original_blob, storage_blob, query_blob, storage_blob_size, alignment); - } - - void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, - size_t &input_blob_size, unsigned char alignment) const override { // One blob was already allocated by a previous preprocessor(s) that process both blobs the // same. The blobs are pointing to the same memory, we need to allocate another memory slot // to split them. if ((storage_blob == query_blob) && (query_blob != nullptr)) { - storage_blob = this->allocator->allocate(input_blob_size); - memcpy(storage_blob, query_blob, input_blob_size); + storage_blob = this->allocator->allocate(storage_blob_size); + memcpy(storage_blob, query_blob, storage_blob_size); } // Either both are nullptr or they are pointing to different memory slots. Both cases are // handled by the designated functions. - this->preprocessForStorage(original_blob, storage_blob, input_blob_size); - this->preprocessQuery(original_blob, query_blob, input_blob_size, alignment); + this->preprocessForStorage(original_blob, storage_blob, storage_blob_size, + storage_alignment); + this->preprocessQuery(original_blob, query_blob, query_blob_size, query_alignment); } - void preprocessForStorage(const void *original_blob, void *&blob, - size_t &input_blob_size) const override { + void preprocessForStorage(const void *original_blob, void *&blob, size_t &input_blob_size, + unsigned char storage_alignment) const override { // If the blob was not allocated yet, allocate it. if (blob == nullptr) { blob = this->allocator->allocate(input_blob_size); diff --git a/tests/unit/test_spaces.cpp b/tests/unit/test_spaces.cpp index 75b66febf..6f22af6c0 100644 --- a/tests/unit/test_spaces.cpp +++ b/tests/unit/test_spaces.cpp @@ -2405,6 +2405,33 @@ TEST(SQ8_FP32_EdgeCases, SelfDistanceCosine) { optimization.avx512f = 0; } #endif +#ifdef OPT_AVX2_FMA + if (optimization.avx2 && optimization.fma3) { + unsigned char alignment = 0; + auto arch_opt_func = Cosine_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); + float result = arch_opt_func(v_quantized.data(), v_orig.data(), dim); + ASSERT_NEAR(result, baseline, 0.01f) << "Optimized self-distance should match baseline"; + optimization.fma3 = 0; + } +#endif +#ifdef OPT_AVX2 + if (optimization.avx2) { + unsigned char alignment = 0; + auto arch_opt_func = Cosine_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); + float result = arch_opt_func(v_quantized.data(), v_orig.data(), dim); + ASSERT_NEAR(result, baseline, 0.01f) << "Optimized self-distance should match baseline"; + optimization.avx2 = 0; + } +#endif +#ifdef OPT_SSE4 + if (optimization.sse4_1) { + unsigned char alignment = 0; + auto arch_opt_func = Cosine_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); + float result = arch_opt_func(v_quantized.data(), v_orig.data(), dim); + ASSERT_NEAR(result, baseline, 0.01f) << "Optimized self-distance should match baseline"; + optimization.sse4_1 = 0; + } +#endif unsigned char alignment = 0; auto arch_opt_func = Cosine_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); @@ -2479,6 +2506,33 @@ TEST(SQ8_FP32_EdgeCases, SelfDistanceL2) { optimization.avx512f = 0; } #endif +#ifdef OPT_AVX2_FMA + if (optimization.avx2 && optimization.fma3) { + unsigned char alignment = 0; + auto arch_opt_func = L2_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); + float result = arch_opt_func(v_quantized.data(), v_orig.data(), dim); + ASSERT_NEAR(result, baseline, 0.01f) << "Optimized self-distance should match baseline"; + optimization.fma3 = 0; + } +#endif +#ifdef OPT_AVX2 + if (optimization.avx2) { + unsigned char alignment = 0; + auto arch_opt_func = L2_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); + float result = arch_opt_func(v_quantized.data(), v_orig.data(), dim); + ASSERT_NEAR(result, baseline, 0.01f) << "Optimized self-distance should match baseline"; + optimization.avx2 = 0; + } +#endif +#ifdef OPT_SSE4 + if (optimization.sse4_1) { + unsigned char alignment = 0; + auto arch_opt_func = L2_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); + float result = arch_opt_func(v_quantized.data(), v_orig.data(), dim); + ASSERT_NEAR(result, baseline, 0.01f) << "Optimized self-distance should match baseline"; + optimization.sse4_1 = 0; + } +#endif unsigned char alignment = 0; auto arch_opt_func = L2_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); From a62aefc0c40a02e8a1ee4c7229c0ea8df269762d Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 3 May 2026 11:30:24 +0300 Subject: [PATCH 2/8] MOD-13837: honor storage_alignment in QuantPreprocessor + alignment tests QuantPreprocessor::preprocessForStorage() now allocates the quantized storage blob with allocate_aligned() when storage_alignment != 0, so SQ8 storage actually honors the per-kernel alignment hint plumbed through PreprocessorsContainerAbstract. New tests: - PreprocessorsTest.QuantizationAsymmetricAlignment: drives the quant preprocessor with query_alignment=32 and storage_alignment=16 and asserts that preprocess(), preprocessForStorage(), and preprocessQuery() each return a pointer aligned to its respective hint. - SpacesTest.SQ8_FP32_DispatcherAlignmentHints / SQ8_SQ8_DispatcherAlignmentHints: assert the exact alignment-hint values published by the x86 SQ8 dispatchers (AVX512 -> 16/32, AVX2_FMA -> 8, AVX2 -> 8, SSE4 -> 4), plus the no-optimization 0-case, for IP / L2 / Cosine. --- src/VecSim/spaces/computer/preprocessors.h | 7 +- tests/unit/test_components.cpp | 53 ++++++++++++++ tests/unit/test_spaces.cpp | 84 ++++++++++++++++++++++ 3 files changed, 140 insertions(+), 4 deletions(-) diff --git a/src/VecSim/spaces/computer/preprocessors.h b/src/VecSim/spaces/computer/preprocessors.h index 229e4922a..2825ada57 100644 --- a/src/VecSim/spaces/computer/preprocessors.h +++ b/src/VecSim/spaces/computer/preprocessors.h @@ -414,10 +414,9 @@ class QuantPreprocessor : public PreprocessorInterface { unsigned char storage_alignment) const override { assert(!blob && "storage_blob must be nullptr"); - // Storage alignment hint is plumbed but not yet honored here; aligned storage allocation - // lands in a dedicated commit (see MOD-13837 plan). - (void)storage_alignment; - blob = this->allocator->allocate(storage_bytes_count); + blob = storage_alignment + ? this->allocator->allocate_aligned(storage_bytes_count, storage_alignment) + : this->allocator->allocate(storage_bytes_count); // Cast to appropriate types const DataType *input = static_cast(original_blob); OUTPUT_TYPE *quantized = static_cast(blob); diff --git a/tests/unit/test_components.cpp b/tests/unit/test_components.cpp index aab10be2c..845d283c0 100644 --- a/tests/unit/test_components.cpp +++ b/tests/unit/test_components.cpp @@ -1073,6 +1073,59 @@ TEST(PreprocessorsTest, QuantizationTest) { } } +// Verifies that the QuantPreprocessor honors distinct query_alignment and storage_alignment hints +// independently. This guards the MOD-13837 contract: storage and query buffers can have different +// SIMD alignment requirements (e.g. SQ8 storage vs FP32 query). +TEST(PreprocessorsTest, QuantizationAsymmetricAlignment) { + std::shared_ptr allocator = VecSimAllocator::newVecsimAllocator(); + constexpr size_t n_preprocessors = 1; + constexpr unsigned char query_alignment = 32; + constexpr unsigned char storage_alignment = 16; + constexpr size_t dim = 6; + constexpr size_t original_blob_size = dim * sizeof(float); + float original_blob[dim] = {1, 2, 3, 4, 5, 6}; + + auto quant_preprocessor = + new (allocator) QuantPreprocessor(allocator, dim); + auto multiPPContainer = MultiPreprocessorsContainer( + allocator, query_alignment, storage_alignment); + multiPPContainer.addPreprocessor(quant_preprocessor); + + // preprocess() exercises the joint storage+query allocation path. + { + ProcessedBlobs processed_blobs = + multiPPContainer.preprocess(original_blob, original_blob_size); + const void *storage_blob = processed_blobs.getStorageBlob(); + const void *query_blob = processed_blobs.getQueryBlob(); + + ASSERT_NE(storage_blob, nullptr); + ASSERT_NE(query_blob, nullptr); + ASSERT_NE(storage_blob, query_blob); + + ASSERT_EQ(reinterpret_cast(storage_blob) % storage_alignment, 0u) + << "storage blob not aligned to " << static_cast(storage_alignment); + ASSERT_EQ(reinterpret_cast(query_blob) % query_alignment, 0u) + << "query blob not aligned to " << static_cast(query_alignment); + } + + // preprocessForStorage() is the storage-only path; must honor storage_alignment. + { + auto storage_blob = + multiPPContainer.preprocessForStorage(original_blob, original_blob_size); + ASSERT_NE(storage_blob.get(), nullptr); + ASSERT_EQ(reinterpret_cast(storage_blob.get()) % storage_alignment, 0u) + << "storage blob not aligned to " << static_cast(storage_alignment); + } + + // preprocessQuery() is the query-only path; must honor query_alignment. + { + auto query_blob = multiPPContainer.preprocessQuery(original_blob, original_blob_size); + ASSERT_NE(query_blob.get(), nullptr); + ASSERT_EQ(reinterpret_cast(query_blob.get()) % query_alignment, 0u) + << "query blob not aligned to " << static_cast(query_alignment); + } +} + // Test edge case where all entries are equal TEST(PreprocessorsTest, QuantizationTestAllEntriesEqual) { std::shared_ptr allocator = VecSimAllocator::newVecsimAllocator(); diff --git a/tests/unit/test_spaces.cpp b/tests/unit/test_spaces.cpp index 6f22af6c0..446f1288b 100644 --- a/tests/unit/test_spaces.cpp +++ b/tests/unit/test_spaces.cpp @@ -3915,3 +3915,87 @@ TEST(SQ8_SQ8_EdgeCases, L2ExtremeValuesTest) { ASSERT_NEAR(result, baseline, 0.01f) << "Extreme values L2 should match baseline"; } + +// MOD-13837: assert the exact alignment-hint values published by the SQ8 distance dispatchers. +// The hint refers to the SQ8 (first / storage) operand per the GetDistFunc contract documented +// in spaces/spaces.h. These tests guard against silent regressions of the per-kernel hints used +// by the preprocessor pipeline to align the storage blob. +#ifdef CPU_FEATURES_ARCH_X86_64 +TEST_F(SpacesTest, SQ8_FP32_DispatcherAlignmentHints) { + // dim divisible by 16 (and therefore 8 and 4) so every x86 path sets a non-zero hint. + constexpr size_t dim = 64; + auto features = getCpuOptimizationFeatures(); + + auto check = [&](const char *kind, + spaces::dist_func_t (*get)(size_t, unsigned char *, const void *)) { + auto opt = features; +#ifdef OPT_AVX512_F_BW_VL_VNNI + if (opt.avx512f && opt.avx512bw && opt.avx512vnni) { + unsigned char alignment = 0; + (void)get(dim, &alignment, &opt); + ASSERT_EQ(alignment, 16u) << kind << ": AVX512 SQ8_FP32 hint should be 16"; + opt.avx512f = 0; + } +#endif +#ifdef OPT_AVX2_FMA + if (opt.avx2 && opt.fma3) { + unsigned char alignment = 0; + (void)get(dim, &alignment, &opt); + ASSERT_EQ(alignment, 8u) << kind << ": AVX2_FMA SQ8_FP32 hint should be 8"; + opt.fma3 = 0; + } +#endif +#ifdef OPT_AVX2 + if (opt.avx2) { + unsigned char alignment = 0; + (void)get(dim, &alignment, &opt); + ASSERT_EQ(alignment, 8u) << kind << ": AVX2 SQ8_FP32 hint should be 8"; + opt.avx2 = 0; + } +#endif +#ifdef OPT_SSE4 + if (opt.sse4_1) { + unsigned char alignment = 0; + (void)get(dim, &alignment, &opt); + ASSERT_EQ(alignment, 4u) << kind << ": SSE4 SQ8_FP32 hint should be 4"; + opt.sse4_1 = 0; + } +#endif + // No-optimization path must leave the hint at 0. + unsigned char alignment = 0; + (void)get(dim, &alignment, &opt); + ASSERT_EQ(alignment, 0u) << kind << ": no-optimization hint should be 0"; + }; + + check("IP", &spaces::IP_SQ8_FP32_GetDistFunc); + check("L2", &spaces::L2_SQ8_FP32_GetDistFunc); + check("Cosine", &spaces::Cosine_SQ8_FP32_GetDistFunc); +} + +TEST_F(SpacesTest, SQ8_SQ8_DispatcherAlignmentHints) { + // dim divisible by 32 so the AVX512 SQ8_SQ8 path sets the hint (otherwise it stays at 0). + constexpr size_t dim = 64; + auto features = getCpuOptimizationFeatures(); + + auto check = [&](const char *kind, + spaces::dist_func_t (*get)(size_t, unsigned char *, const void *)) { + auto opt = features; +#ifdef OPT_AVX512_F_BW_VL_VNNI + if (opt.avx512f && opt.avx512bw && opt.avx512vnni) { + unsigned char alignment = 0; + (void)get(dim, &alignment, &opt); + ASSERT_EQ(alignment, 32u) << kind << ": AVX512 SQ8_SQ8 hint should be 32"; + opt.avx512f = 0; + } +#endif + // No-optimization path must leave the hint at 0. + unsigned char alignment = 0; + (void)get(dim, &alignment, &opt); + ASSERT_EQ(alignment, 0u) << kind << ": no-optimization hint should be 0"; + }; + + check("IP", &spaces::IP_SQ8_SQ8_GetDistFunc); + check("L2", &spaces::L2_SQ8_SQ8_GetDistFunc); + check("Cosine", &spaces::Cosine_SQ8_SQ8_GetDistFunc); +} +#endif // CPU_FEATURES_ARCH_X86_64 From 7d4551497811ab20cf86fe933ec9a934f8c1effc Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 3 May 2026 12:20:18 +0300 Subject: [PATCH 3/8] MOD-13837: drop CosinePreprocessor::allocateBlob helper VecSimAllocator::allocate_aligned(size, 0) already falls through to allocate(size), so the helper's alignment ternary was redundant. Inline the four call sites to a direct allocate_aligned() call. --- src/VecSim/spaces/computer/preprocessors.h | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/VecSim/spaces/computer/preprocessors.h b/src/VecSim/spaces/computer/preprocessors.h index 2825ada57..56811e253 100644 --- a/src/VecSim/spaces/computer/preprocessors.h +++ b/src/VecSim/spaces/computer/preprocessors.h @@ -66,10 +66,12 @@ class CosinePreprocessor : public PreprocessorInterface { if (storage_blob != query_blob) { // If one of them is null, allocate memory for it and copy the original_blob to it. if (storage_blob == nullptr) { - storage_blob = allocateBlob(processed_bytes_count, storage_alignment); + storage_blob = + this->allocator->allocate_aligned(processed_bytes_count, storage_alignment); memcpy(storage_blob, original_blob, storage_blob_size); } else if (query_blob == nullptr) { - query_blob = allocateBlob(processed_bytes_count, query_alignment); + query_blob = + this->allocator->allocate_aligned(processed_bytes_count, query_alignment); memcpy(query_blob, original_blob, query_blob_size); } @@ -81,7 +83,8 @@ class CosinePreprocessor : public PreprocessorInterface { // Single buffer must satisfy both the storage and the query alignment hint. const unsigned char shared_alignment = spaces::combineAlignments(storage_alignment, query_alignment); - query_blob = allocateBlob(processed_bytes_count, shared_alignment); + query_blob = + this->allocator->allocate_aligned(processed_bytes_count, shared_alignment); memcpy(query_blob, original_blob, storage_blob_size); storage_blob = query_blob; } @@ -101,7 +104,7 @@ class CosinePreprocessor : public PreprocessorInterface { assert(blob == nullptr || input_blob_size == processed_bytes_count); if (blob == nullptr) { - blob = allocateBlob(processed_bytes_count, storage_alignment); + blob = this->allocator->allocate_aligned(processed_bytes_count, storage_alignment); memcpy(blob, original_blob, input_blob_size); } normalize_func(blob, this->dim); @@ -127,12 +130,6 @@ class CosinePreprocessor : public PreprocessorInterface { } private: - // Allocate a blob, honoring alignment when non-zero. - void *allocateBlob(size_t size, unsigned char alignment) const { - return alignment ? this->allocator->allocate_aligned(size, alignment) - : this->allocator->allocate(size); - } - spaces::normalizeVector_f normalize_func; const size_t dim; const size_t processed_bytes_count; From fc1fe2859d5e8e3512ebe6347e2979b073d2d3a8 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 3 May 2026 12:40:06 +0300 Subject: [PATCH 4/8] MOD-13837: drop deprecated getAlignment() alias Remove the temporary getAlignment() forwarders from PreprocessorsContainerAbstract and VecSimIndexAbstract. All callers in test_bruteforce, test_hnsw_tiered, and test_allocator now use the explicit getStorageAlignment() accessor, which is what every site already meant (DataBlock allocation overhead, brute-force storage, homogeneous-alignment fixture). --- .../spaces/computer/preprocessor_container.h | 2 -- src/VecSim/vec_sim_index.h | 1 - tests/unit/test_allocator.cpp | 20 +++++++++---------- tests/unit/test_bruteforce.cpp | 2 +- tests/unit/test_hnsw_tiered.cpp | 2 +- 5 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/VecSim/spaces/computer/preprocessor_container.h b/src/VecSim/spaces/computer/preprocessor_container.h index 90b7d8d88..b9d98a415 100644 --- a/src/VecSim/spaces/computer/preprocessor_container.h +++ b/src/VecSim/spaces/computer/preprocessor_container.h @@ -45,8 +45,6 @@ class PreprocessorsContainerAbstract : public VecsimBaseObject { unsigned char getQueryAlignment() const { return query_alignment; } unsigned char getStorageAlignment() const { return storage_alignment; } - // TODO(MOD-13837): remove after callers migrate to getStorageAlignment / getQueryAlignment. - unsigned char getAlignment() const { return storage_alignment; } protected: const unsigned char query_alignment; diff --git a/src/VecSim/vec_sim_index.h b/src/VecSim/vec_sim_index.h index 122b21a41..c61812f08 100644 --- a/src/VecSim/vec_sim_index.h +++ b/src/VecSim/vec_sim_index.h @@ -206,7 +206,6 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { inline size_t getStoredDataSize() const { return storedDataSize; } inline size_t getInputBlobSize() const { return inputBlobSize; } inline size_t getBlockSize() const { return blockSize; } - inline auto getAlignment() const { return this->preprocessors->getAlignment(); } inline auto getQueryAlignment() const { return this->preprocessors->getQueryAlignment(); } inline auto getStorageAlignment() const { return this->preprocessors->getStorageAlignment(); } diff --git a/tests/unit/test_allocator.cpp b/tests/unit/test_allocator.cpp index 525f8f4b7..6aa4a0d0b 100644 --- a/tests/unit/test_allocator.cpp +++ b/tests/unit/test_allocator.cpp @@ -144,8 +144,8 @@ TYPED_TEST(IndexAllocatorTest, test_bf_index_block_size_1) { (vectors_blocks->capacity() - vectors_blocks_capacity) * sizeof(DataBlock) + vecsimAllocationOverhead; // New vectors blocks expectedAllocationDelta += blockSize * sizeof(TEST_DATA_T) * dim + vecsimAllocationOverhead + - bfIndex->getAlignment(); // block vectors buffer - expectedAllocationDelta += hashTableNodeSize; // New node in the label lookup + bfIndex->getStorageAlignment(); // block vectors buffer + expectedAllocationDelta += hashTableNodeSize; // New node in the label lookup // Account for the allocation of a new buckets in the labels_lookup hash table. expectedAllocationDelta += (bfIndex->labelToIdLookup.bucket_count() - buckets_num_before) * sizeof(size_t); @@ -180,7 +180,7 @@ TYPED_TEST(IndexAllocatorTest, test_bf_index_block_size_1) { expectedAllocationDelta += 2 * sizeof(labelType); // resize idToLabelMapping expectedAllocationDelta += 2 * (blockSize * sizeof(TEST_DATA_T) * dim + vecsimAllocationOverhead + - bfIndex->getAlignment()); // Two block vectors buffer + bfIndex->getStorageAlignment()); // Two block vectors buffer expectedAllocationDelta += 2 * hashTableNodeSize; // New nodes in the label lookup expectedAllocationDelta += (bfIndex->labelToIdLookup.bucket_count() - buckets_num_before) * sizeof(size_t); @@ -214,7 +214,7 @@ TYPED_TEST(IndexAllocatorTest, test_bf_index_block_size_1) { ASSERT_EQ(vectors_blocks->capacity(), vectors_blocks_capacity); expectedAllocationDelta -= blockSize * sizeof(TEST_DATA_T) * dim + vecsimAllocationOverhead + - bfIndex->getAlignment(); // Free the vector buffer in the vector block + bfIndex->getStorageAlignment(); // Free the vector buffer in the vector block expectedAllocationDelta -= hashTableNodeSize; // Remove node from the label lookup // idToLabelMapping and label:id should not change since count > capacity - 2 * blockSize ASSERT_EQ(bfIndex->labelToIdLookup.bucket_count(), buckets_num_before); @@ -244,8 +244,8 @@ TYPED_TEST(IndexAllocatorTest, test_bf_index_block_size_1) { expectedAllocationDelta += (vectors_blocks->capacity() - vectors_blocks_capacity) * sizeof(DataBlock); // New vector block expectedAllocationDelta += blockSize * sizeof(TEST_DATA_T) * dim + vecsimAllocationOverhead + - bfIndex->getAlignment(); // block vectors buffer - expectedAllocationDelta += hashTableNodeSize; // New node in the label lookup + bfIndex->getStorageAlignment(); // block vectors buffer + expectedAllocationDelta += hashTableNodeSize; // New node in the label lookup { SCOPED_TRACE( "Verifying allocation delta for adding a vector to index size 2 with capacity 3"); @@ -283,7 +283,7 @@ TYPED_TEST(IndexAllocatorTest, test_bf_index_block_size_1) { ASSERT_EQ(vectors_blocks->capacity(), vectors_blocks_capacity); expectedAllocationDelta -= 2 * (blockSize * sizeof(TEST_DATA_T) * dim + vecsimAllocationOverhead + - bfIndex->getAlignment()); // Free the vector buffer in the vector block + bfIndex->getStorageAlignment()); // Free the vector buffer in the vector block expectedAllocationDelta -= 2 * hashTableNodeSize; // Remove nodes from the label lookup // idToLabelMapping and label:id should shrink by block since count >= capacity - 2 * // blockSize @@ -318,7 +318,7 @@ TYPED_TEST(IndexAllocatorTest, test_bf_index_block_size_1) { ASSERT_EQ(vectors_blocks->capacity(), vectors_blocks_capacity); expectedAllocationDelta -= (blockSize * sizeof(TEST_DATA_T) * dim + vecsimAllocationOverhead + - bfIndex->getAlignment()); // Free the vector buffer in the vector block + bfIndex->getStorageAlignment()); // Free the vector buffer in the vector block expectedAllocationDelta -= hashTableNodeSize; // Remove nodes from the label lookup // idToLabelMapping and label:id should shrink by block since count >= capacity - 2 * // blockSize @@ -567,7 +567,7 @@ TYPED_TEST(IndexAllocatorTest, test_hnsw_reclaim_memory) { // except for the bucket count of the labels_lookup hash table that is calculated separately. // Calculate the expected memory delta for adding a block. size_t data_containers_block_mem = - 2 * (sizeof(DataBlock) + vecsimAllocationOverhead) + hnswIndex->getAlignment(); + 2 * (sizeof(DataBlock) + vecsimAllocationOverhead) + hnswIndex->getStorageAlignment(); size_t size_total_data_per_element = hnswIndex->elementGraphDataSize + hnswIndex->getStoredDataSize(); data_containers_block_mem += size_total_data_per_element * block_size; @@ -592,7 +592,7 @@ TYPED_TEST(IndexAllocatorTest, test_hnsw_reclaim_memory) { verify_containers_size(block_size, 1, 2 * block_size); size_t expected_allocation_size = - before_delete_mem - last_vec_graph_data_mem - hnswIndex->getAlignment(); + before_delete_mem - last_vec_graph_data_mem - hnswIndex->getStorageAlignment(); // Free the buffer of the last block in both data containers. expected_allocation_size -= size_total_data_per_element * block_size + 2 * vecsimAllocationOverhead; diff --git a/tests/unit/test_bruteforce.cpp b/tests/unit/test_bruteforce.cpp index 56307bf88..abf7e9855 100644 --- a/tests/unit/test_bruteforce.cpp +++ b/tests/unit/test_bruteforce.cpp @@ -529,7 +529,7 @@ TYPED_TEST(BruteForceTest, AlignmentSanity) { auto *bf = this->CastToBF(index); // Assuming we have some optimizations (at least SSE), the alignment should be non-zero // (Register byte size) - ASSERT_NE(bf->getAlignment(), 0); + ASSERT_NE(bf->getStorageAlignment(), 0); VecSimIndex_Free(index); } #endif diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index 7d83a58cb..04087a563 100644 --- a/tests/unit/test_hnsw_tiered.cpp +++ b/tests/unit/test_hnsw_tiered.cpp @@ -4329,7 +4329,7 @@ TYPED_TEST(HNSWTieredIndexTestBasic, HNSWWithPreprocessor) { // a preprocessor container that is able to hold a preprocessor array. constexpr size_t n_preprocessors = 1; auto multiPPContainer = new (allocator) - MultiPreprocessorsContainer(allocator, hnsw_index->getAlignment()); + MultiPreprocessorsContainer(allocator, hnsw_index->getStorageAlignment()); auto pp_double_value = new (allocator) PreprocessorDoubleValue(allocator, dim); ASSERT_EQ(multiPPContainer->addPreprocessor(pp_double_value), 0); From b9f8af9a46cc68ed80eb2655384af2bd749e408d Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 3 May 2026 14:57:55 +0300 Subject: [PATCH 5/8] Refactor alignment parameters in preprocessors for consistency --- .../components/preprocessors_factory.h | 11 +++++------ src/VecSim/spaces/computer/preprocessor_container.h | 3 +-- src/VecSim/spaces/computer/preprocessors.h | 9 +++------ src/VecSim/vec_sim_index.h | 5 ++--- tests/unit/test_components.cpp | 12 ++++-------- tests/unit/test_hnsw_tiered.cpp | 3 +-- 6 files changed, 16 insertions(+), 27 deletions(-) diff --git a/src/VecSim/index_factories/components/preprocessors_factory.h b/src/VecSim/index_factories/components/preprocessors_factory.h index bd52b7341..c9e73036b 100644 --- a/src/VecSim/index_factories/components/preprocessors_factory.h +++ b/src/VecSim/index_factories/components/preprocessors_factory.h @@ -37,10 +37,9 @@ struct PreprocessorsContainerParams { * redundant normalization during preprocessing. */ template -PreprocessorsContainerParams CreatePreprocessorsContainerParams(VecSimMetric metric, size_t dim, - bool is_normalized, - unsigned char query_alignment, - unsigned char storage_alignment) { +PreprocessorsContainerParams +CreatePreprocessorsContainerParams(VecSimMetric metric, size_t dim, bool is_normalized, + unsigned char query_alignment, unsigned char storage_alignment) { // By default the processed blob size is the same as the original blob size. size_t processed_bytes_count = dim * sizeof(DataType); @@ -90,8 +89,8 @@ CreatePreprocessorsContainer(std::shared_ptr allocator, return multiPPContainer; } - return new (allocator) PreprocessorsContainerAbstract(allocator, params.query_alignment, - params.storage_alignment); + return new (allocator) + PreprocessorsContainerAbstract(allocator, params.query_alignment, params.storage_alignment); } template diff --git a/src/VecSim/spaces/computer/preprocessor_container.h b/src/VecSim/spaces/computer/preprocessor_container.h index b9d98a415..52565c72c 100644 --- a/src/VecSim/spaces/computer/preprocessor_container.h +++ b/src/VecSim/spaces/computer/preprocessor_container.h @@ -25,8 +25,7 @@ class PreprocessorsContainerAbstract : public VecsimBaseObject { : PreprocessorsContainerAbstract(allocator, alignment, alignment) {} PreprocessorsContainerAbstract(std::shared_ptr allocator, - unsigned char query_alignment, - unsigned char storage_alignment) + unsigned char query_alignment, unsigned char storage_alignment) : VecsimBaseObject(allocator), query_alignment(query_alignment), storage_alignment(storage_alignment) {} diff --git a/src/VecSim/spaces/computer/preprocessors.h b/src/VecSim/spaces/computer/preprocessors.h index 56811e253..509c19357 100644 --- a/src/VecSim/spaces/computer/preprocessors.h +++ b/src/VecSim/spaces/computer/preprocessors.h @@ -36,8 +36,7 @@ class PreprocessorInterface : public VecsimBaseObject { size_t &input_blob_size, unsigned char storage_alignment) const = 0; virtual void preprocessQuery(const void *original_blob, void *&query_blob, - size_t &input_blob_size, - unsigned char query_alignment) const = 0; + size_t &input_blob_size, unsigned char query_alignment) const = 0; virtual void preprocessStorageInPlace(void *original_blob, size_t input_blob_size) const = 0; }; @@ -53,8 +52,7 @@ class CosinePreprocessor : public PreprocessorInterface { void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char storage_alignment, - unsigned char query_alignment) const override { + unsigned char storage_alignment, unsigned char query_alignment) const override { // CosinePreprocessor produces equally-sized storage and query blobs. assert(storage_blob_size == query_blob_size); // see assert docs below @@ -384,8 +382,7 @@ class QuantPreprocessor : public PreprocessorInterface { */ void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char storage_alignment, - unsigned char query_alignment) const override { + unsigned char storage_alignment, unsigned char query_alignment) const override { // CASE 1: STORAGE BLOB NEEDS ALLOCATION - the only implemented case assert(!storage_blob && "CASE 1: storage_blob must be nullptr"); assert(!query_blob && "CASE 1: query_blob must be nullptr"); diff --git a/src/VecSim/vec_sim_index.h b/src/VecSim/vec_sim_index.h index c61812f08..eddfcb2c2 100644 --- a/src/VecSim/vec_sim_index.h +++ b/src/VecSim/vec_sim_index.h @@ -136,9 +136,8 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { // DataBlocksContainer holds the persistent storage vectors, so it must honor the storage // alignment hint (not the query alignment). Today this only aligns the block-base address; // per-element stride padding is a follow-up (see MOD-13837). - this->vectors = new (this->allocator) - DataBlocksContainer(this->blockSize, this->storedDataSize, this->allocator, - this->getStorageAlignment()); + this->vectors = new (this->allocator) DataBlocksContainer( + this->blockSize, this->storedDataSize, this->allocator, this->getStorageAlignment()); } /** diff --git a/tests/unit/test_components.cpp b/tests/unit/test_components.cpp index 845d283c0..02c2b801b 100644 --- a/tests/unit/test_components.cpp +++ b/tests/unit/test_components.cpp @@ -72,8 +72,7 @@ class DummyStoragePreprocessor : public PreprocessorInterface { } void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char storage_alignment, - unsigned char query_alignment) const override { + unsigned char storage_alignment, unsigned char query_alignment) const override { assert(storage_blob_size == query_blob_size); this->preprocessForStorage(original_blob, storage_blob, storage_blob_size, storage_alignment); @@ -118,8 +117,7 @@ class DummyQueryPreprocessor : public PreprocessorInterface { void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char storage_alignment, - unsigned char query_alignment) const override { + unsigned char storage_alignment, unsigned char query_alignment) const override { assert(storage_blob_size == query_blob_size); this->preprocessQuery(original_blob, query_blob, query_blob_size, query_alignment); } @@ -155,8 +153,7 @@ class DummyMixedPreprocessor : public PreprocessorInterface { value_to_add_query(value_to_add_query) {} void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char storage_alignment, - unsigned char query_alignment) const override { + unsigned char storage_alignment, unsigned char query_alignment) const override { assert(storage_blob_size == query_blob_size); // One blob was already allocated by a previous preprocessor(s) that process both blobs the @@ -217,8 +214,7 @@ class DummyChangeAllocSizePreprocessor : public PreprocessorInterface { void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char storage_alignment, - unsigned char query_alignment) const override { + unsigned char storage_alignment, unsigned char query_alignment) const override { // if the blobs are equal, allocate a single shared buffer aligned to satisfy both hints. if (storage_blob == query_blob) { assert(storage_blob_size == query_blob_size); diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index 04087a563..6beb20cf0 100644 --- a/tests/unit/test_hnsw_tiered.cpp +++ b/tests/unit/test_hnsw_tiered.cpp @@ -4246,8 +4246,7 @@ class PreprocessorDoubleValue : public PreprocessorInterface { void preprocess(const void *original_blob, void *&storage_blob, void *&query_blob, size_t &storage_blob_size, size_t &query_blob_size, - unsigned char storage_alignment, - unsigned char query_alignment) const override { + unsigned char storage_alignment, unsigned char query_alignment) const override { // This assert makes sure the current use of the preprocessor is valid, // i.e., both blobs are of the same size. // In order to use different sizes, the preprocessor should be modified. From fb292c336dc5c7eba16c81bf0e4c2cf15cb9da89 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 10 May 2026 16:05:59 +0300 Subject: [PATCH 6/8] MOD-13837: address review nits - comment clarifications - test_spaces.cpp: drop MOD-13837 prefix from test header comment - preprocessors.h: inline the rationale for unsupported dynamic resizing instead of pointing at commit history - preprocessor_container.h: rename 'Legacy ctor' to 'Homogeneous ctor' on both PreprocessorsContainerAbstract and MultiPreprocessorsContainer (the homogeneous overload is not deprecated; every in-tree caller uses it) - vec_sim_index.h: replace the vague 'per-element stride padding follow-up' note with a concrete explanation of why only the first vector per block is guaranteed aligned today --- src/VecSim/spaces/computer/preprocessor_container.h | 4 ++-- src/VecSim/spaces/computer/preprocessors.h | 7 ++++--- src/VecSim/vec_sim_index.h | 7 +++++-- tests/unit/test_spaces.cpp | 2 +- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/VecSim/spaces/computer/preprocessor_container.h b/src/VecSim/spaces/computer/preprocessor_container.h index 52565c72c..0154c158d 100644 --- a/src/VecSim/spaces/computer/preprocessor_container.h +++ b/src/VecSim/spaces/computer/preprocessor_container.h @@ -19,7 +19,7 @@ struct ProcessedBlobs; class PreprocessorsContainerAbstract : public VecsimBaseObject { public: - // Legacy ctor: same value applies to both query and storage alignment (homogeneous case). + // Homogeneous ctor: same value applies to both query and storage alignment. PreprocessorsContainerAbstract(std::shared_ptr allocator, unsigned char alignment) : PreprocessorsContainerAbstract(allocator, alignment, alignment) {} @@ -70,7 +70,7 @@ class MultiPreprocessorsContainer : public PreprocessorsContainerAbstract { std::array preprocessors; public: - // Legacy ctor: same value applies to both query and storage alignment (homogeneous case). + // Homogeneous ctor: same value applies to both query and storage alignment. MultiPreprocessorsContainer(std::shared_ptr allocator, unsigned char alignment) : MultiPreprocessorsContainer(allocator, alignment, alignment) {} diff --git a/src/VecSim/spaces/computer/preprocessors.h b/src/VecSim/spaces/computer/preprocessors.h index 509c19357..612356307 100644 --- a/src/VecSim/spaces/computer/preprocessors.h +++ b/src/VecSim/spaces/computer/preprocessors.h @@ -96,9 +96,10 @@ class CosinePreprocessor : public PreprocessorInterface { void preprocessForStorage(const void *original_blob, void *&blob, size_t &input_blob_size, unsigned char storage_alignment) const override { - // The asserts here verify that if a blob was allocated by a previous preprocessor, its size - // matches our expected processed size, allowing in-place normalization. Dynamic resizing is - // intentionally not supported (see commit history for rationale). + // The assert here verifies that if a blob was allocated by a previous preprocessor, its + // size matches our expected processed size, allowing in-place normalization. Dynamic + // resizing is intentionally not supported: handling it would require runtime size checks + // and reallocation logic in a hot path, and no current caller needs it. assert(blob == nullptr || input_blob_size == processed_bytes_count); if (blob == nullptr) { diff --git a/src/VecSim/vec_sim_index.h b/src/VecSim/vec_sim_index.h index eddfcb2c2..77a4fee93 100644 --- a/src/VecSim/vec_sim_index.h +++ b/src/VecSim/vec_sim_index.h @@ -134,8 +134,11 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { assert(storedDataSize); assert(inputBlobSize); // DataBlocksContainer holds the persistent storage vectors, so it must honor the storage - // alignment hint (not the query alignment). Today this only aligns the block-base address; - // per-element stride padding is a follow-up (see MOD-13837). + // alignment hint (not the query alignment). Note: this only aligns the base address of + // each block; vectors inside a block are packed back-to-back at stride `storedDataSize`, + // so only the first vector in every block is guaranteed to be aligned. Aligning every + // vector would require padding `storedDataSize` up to a multiple of the alignment, which + // is a separate change. this->vectors = new (this->allocator) DataBlocksContainer( this->blockSize, this->storedDataSize, this->allocator, this->getStorageAlignment()); } diff --git a/tests/unit/test_spaces.cpp b/tests/unit/test_spaces.cpp index 446f1288b..92e47f3bf 100644 --- a/tests/unit/test_spaces.cpp +++ b/tests/unit/test_spaces.cpp @@ -3916,7 +3916,7 @@ TEST(SQ8_SQ8_EdgeCases, L2ExtremeValuesTest) { ASSERT_NEAR(result, baseline, 0.01f) << "Extreme values L2 should match baseline"; } -// MOD-13837: assert the exact alignment-hint values published by the SQ8 distance dispatchers. +// Assert the exact alignment-hint values published by the SQ8 distance dispatchers. // The hint refers to the SQ8 (first / storage) operand per the GetDistFunc contract documented // in spaces/spaces.h. These tests guard against silent regressions of the per-kernel hints used // by the preprocessor pipeline to align the storage blob. From 509adcb8afbab5da6a80754458ad0a24c66c61b2 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 10 May 2026 16:14:11 +0300 Subject: [PATCH 7/8] MOD-13837: drop ternary in QuantPreprocessor::preprocessForStorage Address Cursor Bugbot review: the storage_alignment ? allocate_aligned : allocate ternary was the last surviving instance of this pattern in the file - QuantPreprocessor::preprocessQuery and CosinePreprocessor both already call allocate_aligned unconditionally. allocate_aligned(size, 0) short-circuits to allocate(size) at the top of VecSimAllocator::allocate_aligned (vecsim_malloc.cpp:47-49), so the behavior is unchanged. --- src/VecSim/spaces/computer/preprocessors.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/VecSim/spaces/computer/preprocessors.h b/src/VecSim/spaces/computer/preprocessors.h index 612356307..14407b465 100644 --- a/src/VecSim/spaces/computer/preprocessors.h +++ b/src/VecSim/spaces/computer/preprocessors.h @@ -409,9 +409,7 @@ class QuantPreprocessor : public PreprocessorInterface { unsigned char storage_alignment) const override { assert(!blob && "storage_blob must be nullptr"); - blob = storage_alignment - ? this->allocator->allocate_aligned(storage_bytes_count, storage_alignment) - : this->allocator->allocate(storage_bytes_count); + blob = this->allocator->allocate_aligned(storage_bytes_count, storage_alignment); // Cast to appropriate types const DataType *input = static_cast(original_blob); OUTPUT_TYPE *quantized = static_cast(blob); From c6443af6b19df0dea0304bdd1a6c77739b7aa968 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 10 May 2026 16:40:29 +0300 Subject: [PATCH 8/8] MOD-13837: update FP16 quant preprocessor tests for split alignment API PR #944 added QuantPreprocessorFP16MetricTest using the old single-alignment signatures. After merging origin/main, update the two new call sites to pass storage and query alignments explicitly: - preprocess(...): pass alignment as both storage_alignment and query_alignment (alignment is 0 in this test, so behavior is unchanged) - preprocessForStorage(...): pass storage_alignment=0 --- tests/unit/test_components.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_components.cpp b/tests/unit/test_components.cpp index 43cd1d702..eb465fa92 100644 --- a/tests/unit/test_components.cpp +++ b/tests/unit/test_components.cpp @@ -1393,7 +1393,8 @@ class QuantPreprocessorFP16MetricTest : public testing::TestWithParampreprocess(original_blob, storage_blob, query_blob, - storage_blob_size, query_blob_size, alignment); + storage_blob_size, query_blob_size, alignment, + alignment); // Verify storage blob layout/size ASSERT_NE(storage_blob, nullptr); @@ -1505,7 +1506,7 @@ TEST(QuantPreprocessorFP16Test, QuantizeReconstructRoundTripL2) { void *storage_blob = nullptr; size_t storage_blob_size = 0; - preprocessor->preprocessForStorage(input, storage_blob, storage_blob_size); + preprocessor->preprocessForStorage(input, storage_blob, storage_blob_size, 0); ASSERT_NE(storage_blob, nullptr); ASSERT_EQ(storage_blob_size, dim * sizeof(uint8_t) + 4 * sizeof(float));