[MOD-14960] Micro benchmarks for SQ8 tiered HNSW index - #6
xiangze-arm wants to merge 2 commits into
Conversation
Including TopK/Range/Memory tests Also add a script to convert fp32/fp16 index files to sq8
| input, ¶ms, abstractInitParams, components, version, mean_ptr); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This SQ8 load path doesn't reject unsupported type/metric combinations the way the construction path (SQ8ParamsSupported) does. If params.type isn't FLOAT32/FLOAT16, execution falls through here into the non-quantized loader below and misreads SQ8-quantized bytes as raw vectors, instead of rejecting cleanly. Same issue for metric: any non-L2 metric silently gets treated as IP (line 391-405) with no check it's actually IP. Worth adding the same SQ8ParamsSupported-style guard here.
| readySwapJobs(0), isQuantized(false) { | ||
| // For benchmark, we create tiered hnsw index from existing hnsw index | ||
| // primaryIndexParams is nullptr and accumulation phase is skipped for sq8 | ||
| if (!tiered_index_params.primaryIndexParams) { |
There was a problem hiding this comment.
This looks unreachable; TieredHNSWFactory::NewIndex in tiered_factory.h:47 (the function used for exactly that benchmark scenario) unconditionally dereferences the same field before ever calling this constructor. Either this comment is stale, or there's a caller I'm missing, can you point to it?
| assert(hnsw_index->getInputBlobSize() == abstractInitParams.storedDataSize); | ||
| assert(hnsw_index->getStoredDataSize() == abstractInitParams.storedDataSize); | ||
| const HNSWParams *hnsw_params = ¶ms->primaryIndexParams->algoParams.hnswParams; | ||
| assert(hnsw_params->quantType != VecSimQuant_NONE || |
There was a problem hiding this comment.
For quantized indexes this assert skips the storedDataSize consistency check entirely instead of checking against the correct SQ8-adjusted size. Debug-only impact, but a real drop in verification rigor at exactly the place the codebase already cares about keeping construction/size-estimation in sync.
| IndexPtr(HNSWFactory::NewIndex(AttachRootPath(hnsw_sq8_index_file))); | ||
|
|
||
| auto *hnsw_sq8_index = CastToHNSW(indices[INDEX_HNSW_SQ8]); | ||
| size_t ef_r = 10; |
There was a problem hiding this comment.
size_t ef_r = 10; no comment on why 10, and this looks like a no-op for TopK_Tiered_SQ8 (every call there overrides efRuntime per-query anyway via HNSWRuntimeParams). Does this actually matter for Range_Tiered_SQ8, or is it dead? Worth a one-line comment either way.
| std::atomic_int iter = 0; | ||
| auto tiered_index = | ||
| dynamic_cast<TieredHNSWIndex<data_t, dist_t> *>(GET_INDEX(INDEX_TIERED_HNSW_SQ8)); | ||
| size_t total_iters = 50; |
There was a problem hiding this comment.
total_iters = 50 here has to stay in sync with ->Iterations(50) in the separate REGISTER_TopK_Tiered macro — nothing enforces that, and a drift is a stack buffer overflow on all_results[]. I see bm_vecsim_basics.h:526 already uses a shared named constant (BM_VecSimGeneral::block_size) for exactly this kind of coupling elsewhere in the same file family — could this follow that pattern instead of a bare literal?
| } | ||
|
|
||
| // V4 cannot encode the quantization settings needed to reload an SQ8 index. | ||
| TYPED_TEST(HNSWSQ8Test, RejectsSerialization) { |
There was a problem hiding this comment.
This PR adds real V5 save/load serialization support but removes RejectsSerialization with no replacement test that actually calls saveIndex/loads a SQ8 index back. Right now the only thing exercising this new serialization code at all is the benchmark in this PR, which needs multi-GB external data and never runs in make unit_test/CI. Worth a focused round-trip unit test here.
| .flatBufferLimit = block_size, | ||
| .primaryIndexParams = ¶ms, | ||
| .specificParams = { | ||
| TieredHNSWParams{.swapJobThreshold = 0, .QuantNormalizationSetSize = 0}}}; |
There was a problem hiding this comment.
this bypasses the training/accumulation phase entirely, so none of the three new benchmarks exercise it. MOD-14960's own "Remaining work" item 2 calls out "a tiered workload that crosses the accumulation-to-backend transition" as required and still outstanding — this PR doesn't add it.
| // Range SQ8 Tiered | ||
| BENCHMARK_TEMPLATE_DEFINE_F(BM_VecSimBasics, BM_FUNC_NAME(Range, Tiered_SQ8), fp16_index_t) | ||
| (benchmark::State &st) { Range_Tiered_SQ8(st); } | ||
| REGISTER_Range_HNSW(BM_FUNC_NAME(Range, Tiered_SQ8), fp16_index_t); No newline at end of file |
| // Memory SQ8 Tiered | ||
| BENCHMARK_TEMPLATE_DEFINE_F(BM_VecSimCommon, BM_FUNC_NAME(Memory, Tiered_SQ8), fp32_index_t) | ||
| (benchmark::State &st) { Memory(st, INDEX_TIERED_HNSW_SQ8); } | ||
| BENCHMARK_REGISTER_F(BM_VecSimCommon, BM_FUNC_NAME(Memory, Tiered_SQ8))->Iterations(1); | ||
|
|
||
| // TopK SQ8 Tiered | ||
| BENCHMARK_TEMPLATE_DEFINE_F(BM_VecSimCommon, BM_FUNC_NAME(TopK, Tiered_SQ8), fp32_index_t) | ||
| (benchmark::State &st) { TopK_Tiered_SQ8(st); } | ||
| REGISTER_TopK_Tiered(BM_VecSimCommon, BM_FUNC_NAME(TopK, Tiered_SQ8)); | ||
|
|
||
| // Range SQ8 Tiered | ||
| BENCHMARK_TEMPLATE_DEFINE_F(BM_VecSimBasics, BM_FUNC_NAME(Range, Tiered_SQ8), fp32_index_t) | ||
| (benchmark::State &st) { Range_Tiered_SQ8(st); } | ||
| REGISTER_Range_HNSW(BM_FUNC_NAME(Range, Tiered_SQ8), fp32_index_t); |
There was a problem hiding this comment.
Only Memory, TopK, and Range SQ8 benchmarks are added here. MOD-14960's own scope explicitly lists add_label as one of the four required micro-benchmark types (Memory, add_label, TopK, Range) — there's no SQ8 insert/add benchmark anywhere in this PR.
2 participants
Describe the changes in the pull request
Add micro benchmarks for SQ8 tiered HNSW index and add serialization support for SQ8 HNSW index. Also add a script to convert FP32/FP16 index files to SQ8.
Which issues this PR fixes
Main objects this PR modified
HNSWSerializer: save SQ8 index file with V5 version andquantTypeandmeanVectorfield.BM_VecSimIndex: create SQ8 tiered HNSW index from saved SQ8 index file.convert_to_sq8.py: script to convert FP32/FP16 index files to SQ8.Mark if applicable