-
Notifications
You must be signed in to change notification settings - Fork 32
[MOD-14956] Add SQ8 quantization support for HNSW index #1007
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.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
274afbc
4d09236
814eab5
7719c58
63271c1
9e69259
d5d93d6
9b1615c
ac05e54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,14 @@ class HNSWIndex_Single : public HNSWIndex<DataType, DistType> { | |
| int addVector(const void *vector_data, labelType label) override; | ||
| vecsim_stl::vector<idType> markDelete(labelType label) override; | ||
| double getDistanceFrom_Unsafe(labelType label, const void *vector_data) const override { | ||
| // The public API documents vector_data as a raw dim-by-type vector, but a quantized index's | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getDataByLabel SQ8 over-readHigh Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit d5d93d6. Configure here.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed. Both Tracked as MOD-17530. Both helpers are behind |
||
| // kernels read query metadata appended past that, so honouring the documented contract here | ||
| // would read out of bounds. There is no public API for producing a quantized query blob, | ||
| // and adding one is a separate decision. Report "no answer" rather than read past the | ||
| // caller's blob. See the note on VecSimIndex_GetDistanceFrom_Unsafe in vec_sim.h. | ||
| if (this->isQuantized) { | ||
| return INVALID_SCORE; | ||
| } | ||
| return getDistanceFromInternal(label, vector_data); | ||
| } | ||
| int removeLabel(labelType label) override { return labelLookup.erase(label); } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | |||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,6 +17,7 @@ | ||||||||||
|
|
|||||||||||
| using bfloat16 = vecsim_types::bfloat16; | |||||||||||
| using float16 = vecsim_types::float16; | |||||||||||
| using sq8 = vecsim_types::sq8; | |||||||||||
|
|
|||||||||||
| namespace HNSWFactory { | |||||||||||
|
|
|||||||||||
|
|
@@ -34,11 +35,166 @@ NewIndex_ChooseMultiOrSingle(const HNSWParams *params, | ||||||||||
| HNSWIndex_Single<DataType, DistType>(params, abstractInitParams, components); | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| template <VecSimMetric Metric> | |||||||||||
| [[nodiscard]] constexpr size_t GetSQ8StoredDataSize(size_t dim, bool with_norm) { | |||||||||||
| static_assert(Metric == VecSimMetric_L2 || Metric == VecSimMetric_IP); | |||||||||||
|
|
|||||||||||
| // WithNorm is a template parameter, so dispatch the runtime flag to the two instantiations. | |||||||||||
| return with_norm ? sq8::storage_bytes_count<Metric, true>(dim) | |||||||||||
| : sq8::storage_bytes_count<Metric, false>(dim); | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| // Alignment required by a query blob of type DataType. Per the asymmetric-types contract in | |||||||||||
| // spaces.h, the hint returned alongside an asymmetric distance function describes its first | |||||||||||
| // (storage) operand, so the query side must be obtained from the symmetric dispatcher for the | |||||||||||
| // query's own type. Only that hint is wanted here, never the function it returns, so the call is | |||||||||||
| // contained in this adapter instead of leaving a discarded value at the call site. | |||||||||||
| template <typename DataType> | |||||||||||
| [[nodiscard]] unsigned char GetQueryAlignment(VecSimMetric metric, size_t dim) { | |||||||||||
| unsigned char alignment = 0; | |||||||||||
| spaces::GetDistFunc<DataType, float>(metric, dim, &alignment); | |||||||||||
| return alignment; | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| // The metric the SQ8 components are actually built for. A Cosine index whose blobs are already | |||||||||||
| // normalized by someone else (the frontend of a tiered index) is built as IP. | |||||||||||
| [[nodiscard]] constexpr VecSimMetric ResolveSQ8Metric(VecSimMetric metric, bool is_normalized) { | |||||||||||
| return (is_normalized && metric == VecSimMetric_Cosine) ? VecSimMetric_IP : metric; | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| // Single definition of the SQ8 parameter combinations this factory can build, taking the metric | |||||||||||
| // already resolved by ResolveSQ8Metric. NewIndex fails closed when this returns false, and | |||||||||||
| // EstimateInitialSize rejects the same set, so it cannot report a size for parameters that cannot | |||||||||||
| // produce an index. | |||||||||||
| [[nodiscard]] constexpr bool SQ8ParamsSupported(VecSimType type, VecSimMetric resolved_metric, | |||||||||||
| bool with_mean) { | |||||||||||
| // Kernels exist for FP32 and FP16 sources only. | |||||||||||
| if (type != VecSimType_FLOAT32 && type != VecSimType_FLOAT16) { | |||||||||||
| return false; | |||||||||||
| } | |||||||||||
| // Only L2 and IP have SQ8 kernels. Cosine needs a normalization step the SQ8 preprocessor does | |||||||||||
| // not perform, and anything outside the enum has to be rejected here as well: the dispatch in | |||||||||||
| // NewIndex would otherwise fall through to its unreachable-branch assert and abort an | |||||||||||
| // assertions-enabled host, where the unquantized path throws and is caught by VecSimIndex_New. | |||||||||||
| if (resolved_metric != VecSimMetric_L2 && resolved_metric != VecSimMetric_IP) { | |||||||||||
| return false; | |||||||||||
| } | |||||||||||
| // Mean-centred FP16 L2 loses correctness: QuantPreprocessor centres the query and narrows the | |||||||||||
| // result back into the FP16 query body, while storage keeps its centred min/delta in FP32. The | |||||||||||
| // two then disagree, so an identical vector and query pair yields a non-zero distance (mean | |||||||||||
| // 10000 gives a per-component error of 1.0), and a large enough mean overflows FP16 to | |||||||||||
| // infinity. Enabling this needs an asymmetric kernel that takes an FP32 centred query. Only the | |||||||||||
| // WithNorm && L2 branch centres the query, so FP16 with a mean and IP is unaffected. | |||||||||||
| if (type == VecSimType_FLOAT16 && with_mean && resolved_metric == VecSimMetric_L2) { | |||||||||||
| return false; | |||||||||||
| } | |||||||||||
| return true; | |||||||||||
|
cursor[bot] marked this conversation as resolved.
|
|||||||||||
| } | |||||||||||
|
|
|||||||||||
| // Helper to build an SQ8-quantized HNSW index given compile-time DataType and Metric. | |||||||||||
| template <typename DataType, VecSimMetric Metric> | |||||||||||
| VecSimIndex *NewIndex_SQ8(const HNSWParams *hnswParams, AbstractIndexInitParams abstractInitParams, | |||||||||||
| const float *mean_ptr) { | |||||||||||
| auto &allocator = abstractInitParams.allocator; | |||||||||||
| const size_t dim = abstractInitParams.dim; | |||||||||||
| const bool with_norm = mean_ptr != nullptr; | |||||||||||
| unsigned char storage_alignment = 0, asym_storage_alignment = 0; | |||||||||||
|
|
|||||||||||
| // Override blob size for the SQ8 storage layout. | |||||||||||
| abstractInitParams.storedDataSize = GetSQ8StoredDataSize<Metric>(dim, with_norm); | |||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The existing test serializer now silently accepts a layout its loader cannot decode. V4 records
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed structurally, and I agree with your proposed fix. This is the same shape as the tiered hazard: a combination that is not wired yet but is silently accepted. The guard belongs in this PR by the same argument, and the I have not done it in this round because the requested scope was the two blocking findings. It is a small follow-up: reject SQ8 in
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 9e69259. One wart to flag rather than hide:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re-verified on
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right and my previous reply on this thread was wrong. I called the leftover stub "fails closed on load" and treated moving the check earlier as a nicety. It truncates a valid snapshot, which is data loss, not a cosmetic wart. Corrected in the carry-forward doc as well, since that is where I recorded the mistaken version. The ordering is as you describe: One thing worth adding, because it changes who owns the fix: this is not specific to the SQ8 guard, and not caused by it. Truncate-before-validate applies to any validation failure inside Tracked as MOD-17529, which had no owning ticket before: HLD section 3.3.6 puts feature persistence in RediSearch RDB (MOD-14958) and says nothing about VecSim's own serializer. The ticket takes your requirement verbatim, that the regression assert the destination is byte-for-byte unchanged rather than merely that the save threw, and prefers your atomic-temp-file option over validate-before-open, since it fixes the whole class instead of one check.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still blocking. The SQ8 serialization guard added here deterministically reaches this pre-existing truncate-before-validate ordering, so an unsupported operation introduced by this PR can destroy an existing snapshot. Tracking the general serializer problem in MOD-17529 is useful, but this PR still needs to validate before opening the destination or save atomically, with a preservation regression. |
|||||||||||
| abstractInitParams.isQuantized = true; | |||||||||||
|
cursor[bot] marked this conversation as resolved.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: the compact storage enabled here also reaches the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed. Both call sites do exactly what you describe: memcpy(vec.data(), this->getDataByInternalId(id), this->dim * sizeof(DataType));
Tracked as MOD-17530 rather than fixed here. Two reasons, and one caveat against myself. MOD-14956 is scoped to the factory path, the The caveat: this is a landmine for the next tickets in the epic. MOD-14957 and MOD-14959 will naturally call
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay to defer to MOD-17530. I rechecked the scope: both affected helpers are compiled only under BUILD_TESTS, no product path reaches them, and this PR does not call them for a quantized index. The ticket should remain a prerequisite for tests in MOD-14957 or MOD-14959, but I do not consider it merge-blocking for this PR. |
|||||||||||
|
|
|||||||||||
| // Symmetric: both stored vectors are SQ8 blobs. | |||||||||||
| auto sym_func = spaces::GetDistFunc<sq8, float>(Metric, dim, &storage_alignment); | |||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: the newly selected symmetric SQ8 kernel can overflow for valid large dimensions. On AVX512 VNNI,
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The mechanism is real and I verified it, but I would like to take it as a separate ticket rather than in this cherry-pick. Two corrections to the scope first, both of which make it worth its own change: It is not AVX512-only. All four SIMD SQ8-to-SQ8 kernels route through The threshold is confirmed: the per-element product caps at My reasoning for separating it: the kernels are pre-existing (they landed with the SQ8 distance-function PRs and are already exercised by I will open a ticket against the kernels with the above. Say the word if you would rather it block this PR and I will pull it in.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I rechecked the exact boundary while re-reviewing the latest head. The original comment's first realizable self-dot overflow at dimension 33,027 is correct. Every non-constant per-vector quantization has at least one
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accepted, your boundary is right and mine was off by one dimension. The reasoning is the part I had missed: the minimum of every non-constant vector maps to Corrected in the carry-forward doc, which had propagated 33,026 from my earlier reply, and MOD-17527 carries the corrected boundary along with the four affected SIMD kernels, the float-accumulating scalar fallback, and the note that the shared uint8 helper means a fix changes int8 and uint8 index behaviour too and needs its own boundary regression.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still blocking. The implementation may predate this PR, but this PR is what selects the symmetric SQ8 kernel for HNSW graph construction. Dimension 33,027 is accepted by the public API and deterministically produces wrapped distances, so MOD-17527 is useful tracking but does not make the new path safe to merge. Please widen or chunk the accumulator, select a safe fallback, or reject SQ8 above a proven safe dimension before enabling it here. |
|||||||||||
| // Asymmetric: stored vector is SQ8 blob, query is DataType. | |||||||||||
| auto asym_func = | |||||||||||
| spaces::GetDistFunc<sq8, float, DataType>(Metric, dim, &asym_storage_alignment); | |||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: selecting these FP32 SQ8 distance functions exposes catastrophic cancellation in both L2 kernels. On this exact head I ran the actual
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed, and I reproduced it independently before deciding anything. Pure FP32 arithmetic, your exact input: Your framing is right and worth restating because it is the part that makes this real: quantization is exact here, so this is the summation form and not the 8 bits. Drop the quantizer entirely and I also measured the onset, which sharpens the exposure:
Onset is around Tracked as MOD-17526 rather than fixed here, with the repro, the boundary table and both fix options in the ticket. My reasoning: the kernels are pre-existing, MOD-14956 is scoped to wiring them into the factory, and the HLD does not specify their numerics. More to the point, the identity is what reduces the SQ8-to-SQ8 inner loop to a uint8 dot product, which is the entire reason VNNI and NEON DOTPROD help. Your preferred fix, a direct-difference formulation, gives that up, and I am not willing to trade a rare wrong answer for a permanent slowdown nobody has measured inside a cherry-pick. The ticket flags a cheaper candidate to measure first: widen the L2 metadata and the final subtraction to FP64, which fixes this case because the cancellation is in that subtraction. Where I think you have the stronger argument, and I want it on the record rather than buried: unlike the int32 overflow, this needs no absurd dimension, just a plausible data distribution. So I am not deferring it on "unreachable", only on "the fix is a kernel design change that needs ARM's benchmark numbers". Say the word and I will pull it in.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still blocking. Null quantParams is an explicitly supported configuration in this PR, and this PR is what wires the inaccurate kernels into both query evaluation and graph construction. The need to benchmark alternative kernel formulations affects which fix to choose, not whether known-wrong distances can ship. Please fix the calculation or reject the affected no-mean SQ8 L2 configuration; MOD-17526 can retain broader optimization work.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: SQ8 L2 mixes metadata from the original stored vector with a cross term computed from its quantized reconstruction, so it can return a negative distance even at small magnitudes. On ac05e54 through the public C API, storing [0, 0.25, 1] and querying [0, 0.2501, 1] makes a radius-0 query return the label with score -0.000490427. This is distinct from MOD-17526: there is no large offset, and the failure comes from the norm and cross term describing different vectors. Please make all terms use the same representation, or use a stable direct-difference formulation, and add this non-grid radius-zero regression. |
|||||||||||
| // Both hints describe the same stored blob, so they must be combined rather than overwritten. | |||||||||||
| storage_alignment = spaces::combineAlignments(storage_alignment, asym_storage_alignment); | |||||||||||
| // Queries stay in DataType and are compared against stored blobs by asym_func. | |||||||||||
| const unsigned char query_alignment = GetQueryAlignment<DataType>(Metric, dim); | |||||||||||
|
|
|||||||||||
| PreprocessorInterface *pp = nullptr; | |||||||||||
| IndexCalculatorInterface<float> *calc = nullptr; | |||||||||||
|
|
|||||||||||
| if (with_norm) { | |||||||||||
| // Mean-centered SQ8 quantization with norm correction. | |||||||||||
| vecsim_stl::vector<float> mean_vec(allocator); | |||||||||||
| mean_vec.assign(mean_ptr, mean_ptr + dim); | |||||||||||
|
|
|||||||||||
| float mean_sum_squares = 0.0f; | |||||||||||
| for (float v : mean_vec) { | |||||||||||
| mean_sum_squares += v * v; | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| pp = new (allocator) QuantPreprocessor<DataType, Metric, true>(allocator, dim, mean_vec); | |||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: FP16 + mean + L2 loses correctness through this instantiation.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed in 63271c1. I reproduced your numbers exactly using the repo's own conversions before changing anything: One qualifier worth recording: at a realistic mean near 1 the error is exactly zero, so this only bites for large mean magnitudes. It is silent when it does, though, so it still needs handling.
Your preferred fix, keeping the centred query in FP32 with a matching asymmetric kernel, is the correct one but it is an ARM design change plus new kernel work, so I have left it for their series rather than doing it in a cherry-pick. FLOAT16-with-mean also leaves the functional type set, since every functional test uses L2; that trades 11 typed tests for correctness, and none of them were exercising a combination that still works. |
|||||||||||
| calc = new (allocator) DistanceCalculatorWithNorm<DataType, float, Metric>( | |||||||||||
| allocator, asym_func, sym_func, mean_sum_squares); | |||||||||||
| } else { | |||||||||||
| // Plain SQ8 quantization without mean centering. | |||||||||||
| pp = new (allocator) QuantPreprocessor<DataType, Metric>(allocator, dim); | |||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: this preprocessor accepts finite FP32 vectors whose scale arithmetic becomes non-finite. On this head, processing
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed from the code. const MetadataType diff = (max_val - min_val);
const MetadataType delta = (diff == 0.0f) ? MetadataType{1} : diff / MetadataType{255};
const MetadataType inv_delta = MetadataType{1} / delta;For Worth stating explicitly since it is what makes this more than a hardening nit: every input value is finite and within the type. Only the derived range is not. So Tracked as MOD-17528 rather than fixed here.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still blocking. This PR is the first change that makes QuantPreprocessor reachable through public AddVector, so finite input now reaches C++ undefined behavior in a supported SQ8 index. The design and performance tradeoff may require more work, but that does not make the public UB safe to defer. Please reformulate or widen the arithmetic, or otherwise prevent the unsafe configuration from being exposed here, with the sanitizer regression in MOD-17528. |
|||||||||||
| // sym_func for storage-storage; asym_func for query-storage. | |||||||||||
| calc = new (allocator) DistanceCalculatorCommon<float>(allocator, sym_func, asym_func); | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| auto *container = new (allocator) | |||||||||||
| MultiPreprocessorsContainer<DataType, 1>(allocator, query_alignment, storage_alignment); | |||||||||||
| [[maybe_unused]] const int ret = container->addPreprocessor(pp); | |||||||||||
| assert(ret != -1 && "SQ8 preprocessor was not added correctly"); | |||||||||||
|
|
|||||||||||
| IndexComponents<DataType, float> components{calc, container}; | |||||||||||
| return NewIndex_ChooseMultiOrSingle<DataType, float>(hnswParams, abstractInitParams, | |||||||||||
| components); | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| VecSimIndex *NewIndex(const VecSimParams *params, bool is_normalized) { | |||||||||||
| const HNSWParams *hnswParams = ¶ms->algoParams.hnswParams; | |||||||||||
| AbstractIndexInitParams abstractInitParams = | |||||||||||
| VecSimFactory::NewAbstractInitParams(hnswParams, params->logCtx, is_normalized); | |||||||||||
|
|
|||||||||||
| if (hnswParams->quantType != VecSimQuant_NONE) { | |||||||||||
| // Any quantization type this factory does not implement must fail closed. Falling through | |||||||||||
| // to the plain path below would silently build a full-precision index for a caller that | |||||||||||
| // asked for a quantized one. Unreachable while VecSimQuantType holds only NONE and SQ8, and | |||||||||||
| // not covered by a test because forming an out-of-range enumerator is undefined behaviour; | |||||||||||
| // the guard is what keeps adding SQ4 to the enum from becoming a silent fallthrough. | |||||||||||
| if (hnswParams->quantType != VecSimQuant_SQ8) { | |||||||||||
| return NULL; | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| const VecSimMetric metric = ResolveSQ8Metric(hnswParams->metric, is_normalized); | |||||||||||
| const float *mean_ptr = static_cast<const float *>(hnswParams->quantParams); | |||||||||||
|
|
|||||||||||
| if (!SQ8ParamsSupported(hnswParams->type, metric, mean_ptr != nullptr)) { | |||||||||||
| return NULL; | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| if (hnswParams->type == VecSimType_FLOAT32) { | |||||||||||
| if (metric == VecSimMetric_L2) { | |||||||||||
| return NewIndex_SQ8<float, VecSimMetric_L2>(hnswParams, abstractInitParams, | |||||||||||
| mean_ptr); | |||||||||||
| } else if (metric == VecSimMetric_IP) { | |||||||||||
| return NewIndex_SQ8<float, VecSimMetric_IP>(hnswParams, abstractInitParams, | |||||||||||
| mean_ptr); | |||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. L2 SQ8 catastrophic cancellationHigh Severity Wiring FP32 SQ8 L2 into HNSW selects kernels that compute L2虏 as Reviewed by Cursor Bugbot for commit d5d93d6. Configure here.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed, and independently reproduced: stored Same finding @lerman25 raised on this PR; the full analysis, including the measured onset around Deferred rather than fixed here: the kernels are pre-existing, MOD-14956 is scoped to wiring them into the factory, and the identity is what makes the SQ8-to-SQ8 inner loop a uint8 dot product that VNNI and NEON DOTPROD accelerate, so replacing it needs benchmark numbers this cherry-pick cannot produce. |
|||||||||||
| } | |||||||||||
| } else if (hnswParams->type == VecSimType_FLOAT16) { | |||||||||||
| if (metric == VecSimMetric_L2) { | |||||||||||
| return NewIndex_SQ8<float16, VecSimMetric_L2>(hnswParams, abstractInitParams, | |||||||||||
| mean_ptr); | |||||||||||
| } else if (metric == VecSimMetric_IP) { | |||||||||||
| return NewIndex_SQ8<float16, VecSimMetric_IP>(hnswParams, abstractInitParams, | |||||||||||
| mean_ptr); | |||||||||||
| } | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| // Unreachable today: the checks above leave only FP32/FP16 x L2/IP. The assert makes a | |||||||||||
| // debug build shout if a new type or metric ever reaches here, and the return keeps a | |||||||||||
| // release build failing closed rather than falling through and silently building an | |||||||||||
| // unquantized index instead. | |||||||||||
| assert(false && "unhandled SQ8 data type and metric combination"); | |||||||||||
| return NULL; | |||||||||||
|
Comment on lines
+189
to
+195
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe assert false here ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call, and it matches repo precedent ( I would like to do both rather than swap one for the other: assert(false && "unhandled SQ8 type/metric combination");
return NULL;The Shout if you would rather have the assert alone and I will drop the return.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 9e69259, kept alongside the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I rechecked this after the assertion was added, and this location is reachable today: the SQ8 branch rejects Cosine but does not reject other out-of-range
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed in 9b1615c. You are right that the assert made this reachable, and right about the asymmetry: the unquantized dispatcher throws and Worth being precise about what happened, since it was my change either way. The refactor in d5d93d6 pulled the fences into a single
if (resolved_metric != VecSimMetric_L2 && resolved_metric != VecSimMetric_IP) {
return false;
}So
|
|||||||||||
| } | |||||||||||
|
cursor[bot] marked this conversation as resolved.
|
|||||||||||
|
|
|||||||||||
|
cursor[bot] marked this conversation as resolved.
|
|||||||||||
| if (hnswParams->type == VecSimType_FLOAT32) { | |||||||||||
| IndexComponents<float, float> indexComponents = CreateIndexComponents<float, float>( | |||||||||||
| abstractInitParams.allocator, hnswParams->metric, hnswParams->dim, is_normalized); | |||||||||||
|
|
@@ -94,7 +250,31 @@ size_t EstimateInitialSize(const HNSWParams *params, bool is_normalized) { | ||||||||||
| size_t allocations_overhead = VecSimAllocator::getAllocationOverheadSize(); | |||||||||||
|
|
|||||||||||
| size_t est = sizeof(VecSimAllocator) + allocations_overhead; | |||||||||||
| if (params->type == VecSimType_FLOAT32) { | |||||||||||
|
|
|||||||||||
| if (params->quantType != VecSimQuant_NONE) { | |||||||||||
| // Reject exactly what NewIndex rejects. Reporting a size for a combination that cannot be | |||||||||||
| // built lets a caller size its capacity from an index it will then fail to create. | |||||||||||
| if (params->quantType != VecSimQuant_SQ8 || | |||||||||||
| !SQ8ParamsSupported(params->type, ResolveSQ8Metric(params->metric, is_normalized), | |||||||||||
| params->quantParams != nullptr)) { | |||||||||||
| throw std::invalid_argument("Unsupported quantization params for HNSW index"); | |||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: this new throw escapes through the public extern C VecSimIndex_EstimateInitialSize wrapper, which has no catch or status channel. I compiled an actual C caller against this head; FP32 plus SQ8 plus Cosine terminates with exit 134 after an uncaught std::invalid_argument. The new EXPECT_THROW tests call the C wrapper from C++ and therefore encode behavior that a C host cannot handle. Please keep the C API non-throwing by catching at the boundary and returning a documented failure value, or introduce a status plus out-parameter API, with a C-facing regression. Deferring the general estimator contract to MOD-14958 does not make the new reachable throw sites safe here. |
|||||||||||
| } | |||||||||||
| // Calculator + preprocessor container + preprocessor. | |||||||||||
| // Use representative types; sizeof is independent of the template parameters. | |||||||||||
| if (params->quantParams) { // mean provided, WithNorm = true | |||||||||||
| est += allocations_overhead + | |||||||||||
| sizeof(DistanceCalculatorWithNorm<float, float, VecSimMetric_L2>); | |||||||||||
| est += allocations_overhead + sizeof(MultiPreprocessorsContainer<float, 1>); | |||||||||||
| est += allocations_overhead + sizeof(QuantPreprocessor<float, VecSimMetric_L2, true>); | |||||||||||
| est += allocations_overhead + | |||||||||||
| params->dim * sizeof(float); // mean vector in QuantPreprocessor | |||||||||||
| } else { | |||||||||||
| est += allocations_overhead + sizeof(DistanceCalculatorCommon<float>); | |||||||||||
| est += allocations_overhead + sizeof(MultiPreprocessorsContainer<float, 1>); | |||||||||||
| est += allocations_overhead + sizeof(QuantPreprocessor<float, VecSimMetric_L2>); | |||||||||||
| } | |||||||||||
| est += EstimateInitialSize_ChooseMultiOrSingle<float>(params->multi); | |||||||||||
|
cursor[bot] marked this conversation as resolved.
|
|||||||||||
| } else if (params->type == VecSimType_FLOAT32) { | |||||||||||
| est += EstimateComponentsMemory<float, float>(params->metric, is_normalized); | |||||||||||
| est += EstimateInitialSize_ChooseMultiOrSingle<float>(params->multi); | |||||||||||
| } else if (params->type == VecSimType_FLOAT64) { | |||||||||||
|
|
@@ -125,9 +305,25 @@ size_t EstimateElementSize(const HNSWParams *params) { | ||||||||||
| size_t M = (params->M) ? params->M : HNSW_DEFAULT_M; | |||||||||||
| size_t elementGraphDataSize = sizeof(ElementGraphData) + sizeof(idType) * M * 2; | |||||||||||
|
|
|||||||||||
| size_t size_total_data_per_element = | |||||||||||
| elementGraphDataSize + | |||||||||||
| VecSimParams_GetStoredDataSize(params->type, params->dim, params->metric); | |||||||||||
| size_t stored_data_size; | |||||||||||
| // Unlike EstimateInitialSize, this does not reject unsupported combinations: the return type is | |||||||||||
| // size_t with no sentinel, and VecSimIndex_EstimateElementSize is extern "C", so throwing here | |||||||||||
| // would carry an exception into the C host. It therefore answers for whatever params it is | |||||||||||
| // handed, exactly as VecSimParams_GetStoredDataSize does on the unquantized path. Index | |||||||||||
| // creation is the boundary that enforces the supported set (see SQ8ParamsSupported). | |||||||||||
| if (params->quantType == VecSimQuant_SQ8) { | |||||||||||
| bool with_norm = params->quantParams != nullptr; | |||||||||||
| if (params->metric == VecSimMetric_L2) { | |||||||||||
| stored_data_size = GetSQ8StoredDataSize<VecSimMetric_L2>(params->dim, with_norm); | |||||||||||
| } else { | |||||||||||
| stored_data_size = GetSQ8StoredDataSize<VecSimMetric_IP>(params->dim, with_norm); | |||||||||||
| } | |||||||||||
|
cursor[bot] marked this conversation as resolved.
|
|||||||||||
| } else { | |||||||||||
| stored_data_size = | |||||||||||
| VecSimParams_GetStoredDataSize(params->type, params->dim, params->metric); | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| size_t size_total_data_per_element = elementGraphDataSize + stored_data_size; | |||||||||||
|
|
|||||||||||
| // when reserving space for new labels in the lookup hash table, each entry is a pointer to a | |||||||||||
| // label node (bucket). | |||||||||||
|
|
|||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -426,8 +426,7 @@ class QuantPreprocessor : public PreprocessorInterface { | |
| QuantPreprocessor(std::shared_ptr<VecSimAllocator> allocator, size_t dim) | ||
| requires(!WithNorm) | ||
| : PreprocessorInterface(allocator), dim(dim), | ||
| storage_bytes_count(dim * sizeof(OUTPUT_TYPE) + | ||
| sq8::storage_metadata_count<Metric>() * sizeof(MetadataType)), | ||
| storage_bytes_count(sq8::storage_bytes_count<Metric>(dim)), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quantization hits UB on finite inputHigh Severity
Reviewed by Cursor Bugbot for commit 9b1615c. Configure here.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed, same defect @lerman25 raised on this PR, tracked as MOD-17528. Mechanism verified from Two things worth adding to the record, because they cut in opposite directions. Against deferring, and I will not lean on "pre-existing" here: For deferring: the fix is not a finiteness check away, which is why it needs the preprocessor owner rather than a cherry-pick. I measured the obvious repair: So the range is not the only thing that overflows: the per-element The ticket carries both the UBSan output and this table, and asks for a regression at |
||
| query_bytes_count(dim * sizeof(DataType) + | ||
| sq8::query_metadata_count<Metric>() * sizeof(MetadataType)) {} | ||
|
|
||
|
|
@@ -436,9 +435,7 @@ class QuantPreprocessor : public PreprocessorInterface { | |
| const vecsim_stl::vector<float> &mean_vec) | ||
| requires(WithNorm) | ||
| : PreprocessorInterface(allocator), mean(mean_vec), dim(dim), | ||
| storage_bytes_count(dim * sizeof(OUTPUT_TYPE) + | ||
| sq8::storage_metadata_count<Metric, WithNorm>() * | ||
| sizeof(MetadataType)), | ||
| storage_bytes_count(sq8::storage_bytes_count<Metric, WithNorm>(dim)), | ||
| query_bytes_count(dim * sizeof(DataType) + | ||
| sq8::query_metadata_count<Metric, WithNorm>() * sizeof(MetadataType)) { | ||
| assert(this->mean.size() == dim && "mean vector size must equal dim"); | ||
|
|
||


Uh oh!
There was an error while loading. Please reload this page.