feat: add high-performance HNSW RaBitQ index - #1798
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CLiqing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@CLiqing 🔍 Important: PR Classification Needed! For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:
For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”. Thanks for your efforts and contribution to the community!. |
e778c24 to
aae6c99
Compare
|
/hold |
|
@CLiqing just in case, is this PR compatible with facebookresearch/faiss#5526 ? |
abfc167 to
a9c7389
Compare
|
@alexanderguzhva It is not index-format or runtime-behavior compatible with #5526 because the construction and search contracts differ. This PR builds the graph with exact FP32 distances and then attaches The implementations can coexist in source: the Knowhere type is under |
a9c7389 to
89f7b5d
Compare
| // Store one complete scalar code per dimension in a dense n-bit stream. | ||
| // This combines the sign bit and extra bits at build time while retaining | ||
| // the exact nbits/dimension budget. For nbits=8 this is one byte/dimension. | ||
| bool dense_layout = false; |
There was a problem hiding this comment.
RaBitQuantizer::dense_layout (RaBitQuantizer.h:53) changes the on-disk code layout but is not part of the standard wire format: index_write.cpp:1027 tags every multi-bit IndexRaBitQ as Ixrr and index_read.cpp:2856 default-constructs the index, so a faiss::write_index/read_index round-trip restores dense_layout=false and reinterprets dense codes with packed offsets. read_RaBitQuantizer does not recompute code_size, and for any d % 8 == 0 (64, 128, 768, 1536) the dense and packed sizes are byte-identical, so nothing rejects the file — every distance is silently wrong. IndexRaBitQFastScan::IndexRaBitQFastScan(const IndexRaBitQ&, int) (IndexRaBitQFastScan.cpp:106) copies the quantizer and then reads sign bits at orig_code[j/8] >> (j%8), which is garbage for dense codes, also without a guard.
There was a problem hiding this comment.
Fixed. Dense multi-bit IndexRaBitQ now uses the distinct Ixrd tag; the standard reader restores dense_layout, recomputes and validates code_size, and validates the code buffer length. Converting a dense index to IndexRaBitQFastScan now throws explicitly. Cross-reader round-trip tests cover the standard and cppcontrib readers.
| REQUIRE(index.Build(train, json) == knowhere::Status::success); | ||
| REQUIRE(index.Count() == kNb); | ||
|
|
||
| const auto before = index.Search(query, json, nullptr); |
There was a problem hiding this comment.
Every assertion in test_hnsw_rabitq.cc is self-referential: CheckValidKnnResult (:65) only checks id range and isfinite, CheckKnnOrder (:78) only checks monotonicity, the serialization round-trip (:274) compares the index against itself, CalcDistByIDs (:253) compares two paths that resolve to the same distance computer, and the dense-vs-packed case (:282) compares two layouts of the same algorithm. If the query were rotated with a different matrix than the base vectors, or the shared RaBitQ distance formula had a sign error, all five TEST_CASEs still pass and the index ships returning wrong neighbors. HNSW_RABITQ appears in no other test file, so this suite is the only coverage.
There was a problem hiding this comment.
Fixed. The test now computes an independent FP32 brute-force oracle and checks recall for L2/IP, RBQ1/RBQ4/RBQ8, and aligned/unaligned dimensions. It also verifies COSINE storage/refine distances against original vectors, so rotation, sign, or metric mistakes are no longer self-validating.
| } | ||
| } | ||
|
|
||
| template <SIMDLevel SL> |
There was a problem hiding this comment.
tests/faiss/CMakeLists.txt builds faiss_tests from a hardcoded FAISS_TEST_SRCS list that does not contain test_rabitq_simd.cpp, and this PR does not add it, so the 174 lines of new AVX2/AVX-512 comparison tests never run in knowhere CI. The only remaining coverage for the ~1400 new kernel lines is the dense-vs-packed case in test_hnsw_rabitq.cc:282, which exercises exactly one runtime-detected SIMD level: on an AVX-512 CI host the AVX2 dense kernels are never executed, yet they are the ones that run on AVX2-only production hosts, where a kernel bug produces wrong distances undetected.
There was a problem hiding this comment.
Fixed. test_rabitq_simd.cpp is now listed in tests/faiss/CMakeLists.txt. I built faiss_tests locally and ran 10 RaBitQ tests, including explicit AVX2/AVX-512 single and batch-4 equivalence and the non-BMI2 fallback.
|
|
||
| IndexHNSWRaBitQ::IndexHNSWRaBitQ() = default; | ||
|
|
||
| IndexHNSWRaBitQ::IndexHNSWRaBitQ(faiss::IndexPreTransform* storage_in, int M) |
There was a problem hiding this comment.
IndexHNSWRaBitQ(IndexPreTransform*, int) delegates to IndexHNSW(storage, M) (IndexHNSW.cpp:241), which only builds an empty hnsw(M); entry_point stays -1 so HNSW.cpp:1006 returns immediately and every search yields no results, while the constructor still sets ntotal from the storage and presents the index as populated — and add() is overridden to throw, so the graph can never be filled in. It also sets own_fields = true at line 19 before calling validate_storage() at line 22: if validation throws, ~IndexHNSW runs and deletes the storage the caller still owns, giving a double free. Neither this constructor nor the non-const pretransform_index()/rabitq_index() overloads have any caller.
There was a problem hiding this comment.
Fixed. The unused pointer constructor and the unused non-const accessors were removed, eliminating both the empty-graph behavior and the exception-path ownership hazard.
|
|
||
| // Private Knowhere serialization tag. Upstream Faiss reserves "IHNr" for | ||
| // its incompatible direct-build/staged-search IndexHNSWRaBitQ format. | ||
| inline constexpr char kHnswRaBitQFourcc[] = "IHRK"; |
There was a problem hiding this comment.
this should not be here. It needs to be in index_read.cpp and index_write.cpp. Yes, let it be duplicate. Also, please make sure that our four-CC codes do not intersect with Faiss ones. So, a given four-CC should not be used by Faiss.
There was a problem hiding this comment.
Fixed. Knowhere HNSW RaBitQ serialization is now implemented in the standard Faiss index_read.cpp/index_write.cpp, while the cppcontrib implementation remains duplicated as requested. The tags are IHRK, IHRC, and IRKC; I checked them against the bundled/upstream Faiss tag set. Cross-reader tests cover standard Faiss and cppcontrib IO.
|
@CLiqing Please create a corresponding PR in the baseline faiss code and we'll ask Faiss guys to take a look. Thanks. |
89f7b5d to
f78e4ac
Compare
|
Created the requested baseline Faiss PR: facebookresearch/faiss#5552 It contains the reusable dense multi-bit RaBitQ layout, serialization, SIMD single/batch-4 scorers, BMI2 runtime fallback, and focused tests. The Knowhere-specific HNSW integration remains in this PR. |
Signed-off-by: ChenLiqing <23721160+CLiqing@users.noreply.github.com>
f78e4ac to
c74c993
Compare
issue: #1747
Summary
HNSW_RABITQwith 1-8 bit dense database codes and FP32 queries (qb=0), supporting L2, inner product, and cosine similarityRandomRotationMatrix -> IndexRaBitQstorage for searchCompatibility and scope
HNSW_RABITQV1 is immutable after its initial build and supports L2/IP/COSINErbq_bits_queryis rejected explicitlyIndexRaBitQretains its existing 1-9 bit rangefaiss::cppcontrib::knowhereand usesIHRK/IHRC, whileIHNrremains reserved for #5526; shared bundled-Faiss quantizer/SIMD changes will still require normal reconciliation during future Faiss updates