mirror of
https://github.com/facebookresearch/faiss.git
synced 2026-10-12 02:13:36 +00:00
Alibek Zhakubayev
796977a1ab
Summary: Pull Request resolved: https://github.com/facebookresearch/faiss/pull/5100 Fix two issues in `IndexBinaryMultiHash` related to incorrect use of range-based for loops over the `maps` member (`std::vector<std::unordered_map<idx_t, std::vector<idx_t>>>`). ## Bug 1: `reset()` does not actually clear the hash maps (correctness) In `IndexBinaryMultiHash::reset()`, the loop `for (auto map : maps)` iterates by **value**, copying each `unordered_map` into a local variable. `map.clear()` clears the copy; the original maps in the vector are never modified. This is a direct consequence of C++ range-based for semantics ([stmt.ranged]): `auto map` deduces to `Map` (value type), and copy-initialization creates an independent object. Mutations to `map` do not affect the container element. **Proof:** ```cpp std::vector<std::unordered_map<int, int>> v(1); v[0][42] = 1; for (auto m : v) { m.clear(); } assert(v[0].size() == 1); // original untouched for (auto& m : v) { m.clear(); } assert(v[0].size() == 0); // original cleared ``` **Impact:** After `reset()` + `add()`, stale hash map entries still point to old vector IDs. Search returns phantom results from pre-reset data — silent data corruption. **Fix:** `for (auto map : maps)` → `for (auto& map : maps)`. ## Bug 2: `hashtable_size()` copies all hash maps unnecessarily (performance) Same `for (auto map : maps)` pattern copies each entire `unordered_map` (with all buckets and stored `vector<idx_t>` values) just to call `.size()`. O(total entries) of needless allocation per call. **Fix:** `for (auto map : maps)` → `for (const auto& map : maps)`. `const` is correct since `hashtable_size()` is a `const` method. ## Test Added `MultiHashResetClearsMaps` unit test with three phases: 1. **Pre-reset**: Add a vector, assert `hashtable_size() > 0` (precondition — maps are populated). 2. **Reset verification**: Call `reset()`, assert `ntotal == 0` and `hashtable_size() == 0`. Then search for the old vector, assert `labels[0] == -1` (not found). 3. **Post-reset smoke test**: Add a different vector, search for it, assert exact match at ID 0 — verifies the index is fully functional after reset. Reviewed By: limqiying Differential Revision: D100797480 fbshipit-source-id: 7313d103371a8442c00bbff5d0c60b6bead8e7c6
108 lines
3.0 KiB
C++
108 lines
3.0 KiB
C++
/*
|
|
* Copyright (c) Meta Platforms, Inc. and affiliates.
|
|
*
|
|
* This source code is licensed under the MIT license found in the
|
|
* LICENSE file in the root directory of this source tree.
|
|
*/
|
|
|
|
#include <cstdint>
|
|
#include <vector>
|
|
|
|
#include <gtest/gtest.h>
|
|
|
|
#include <faiss/IndexBinaryHash.h>
|
|
|
|
// These use a single 2-byte vector (d=16, code_size=2) so that the
|
|
// resulting index data is not an even multiple of sizeof(uint64_t),
|
|
// testing that the uint64_t sized hash key extraction at the end of
|
|
// the allocation boundary doesn't trigger a heap-buffer-overflow under
|
|
// ASAN.
|
|
//
|
|
// NOTE: These are not python based tests because we have more control
|
|
// over allocation behavior in C++, ensuring these tests
|
|
// deterministically fail without proper handling of access near
|
|
// the end of the allocation.
|
|
|
|
TEST(BinaryHash, SmallCodeSizeRoundTrip) {
|
|
int d = 16;
|
|
int b = 5; // hash on 5 bits
|
|
faiss::IndexBinaryHash idx(d, b);
|
|
idx.nflip = 1;
|
|
|
|
int n = 1;
|
|
std::vector<uint8_t> data(n * idx.code_size);
|
|
data[0] = 0xAA;
|
|
data[1] = 0x55;
|
|
idx.add(n, data.data());
|
|
EXPECT_EQ(idx.ntotal, n);
|
|
|
|
int k = 1;
|
|
std::vector<int32_t> distances(k);
|
|
std::vector<faiss::idx_t> labels(k);
|
|
idx.search(1, data.data(), k, distances.data(), labels.data());
|
|
EXPECT_EQ(distances[0], 0);
|
|
EXPECT_EQ(labels[0], 0);
|
|
}
|
|
|
|
TEST(BinaryHash, MultiHashSmallCodeSizeRoundTrip) {
|
|
int d = 16;
|
|
int nhash = 2;
|
|
int b = 4; // 2 hashes * 4 bits = 8 bits <= d=16
|
|
faiss::IndexBinaryMultiHash idx(d, nhash, b);
|
|
idx.nflip = 1;
|
|
|
|
int n = 1;
|
|
std::vector<uint8_t> data(n * idx.code_size);
|
|
data[0] = 0xAA;
|
|
data[1] = 0x55;
|
|
idx.add(n, data.data());
|
|
EXPECT_EQ(idx.ntotal, n);
|
|
|
|
int k = 1;
|
|
std::vector<int32_t> distances(k);
|
|
std::vector<faiss::idx_t> labels(k);
|
|
idx.search(1, data.data(), k, distances.data(), labels.data());
|
|
EXPECT_EQ(distances[0], 0);
|
|
EXPECT_EQ(labels[0], 0);
|
|
}
|
|
|
|
TEST(BinaryHash, MultiHashResetClearsMaps) {
|
|
int d = 16;
|
|
int nhash = 2;
|
|
int b = 4;
|
|
faiss::IndexBinaryMultiHash idx(d, nhash, b);
|
|
idx.nflip = 0;
|
|
|
|
// Add a vector
|
|
int n = 1;
|
|
std::vector<uint8_t> data(n * idx.code_size);
|
|
data[0] = 0xAA;
|
|
data[1] = 0x55;
|
|
idx.add(n, data.data());
|
|
EXPECT_EQ(idx.ntotal, 1);
|
|
EXPECT_GT(idx.hashtable_size(), 0u);
|
|
|
|
// Reset should clear everything
|
|
idx.reset();
|
|
EXPECT_EQ(idx.ntotal, 0);
|
|
EXPECT_EQ(idx.hashtable_size(), 0u);
|
|
|
|
// Searching for the old vector after reset should not find it
|
|
int k = 1;
|
|
std::vector<int32_t> distances(k);
|
|
std::vector<faiss::idx_t> labels(k);
|
|
idx.search(1, data.data(), k, distances.data(), labels.data());
|
|
EXPECT_EQ(labels[0], -1);
|
|
|
|
// After reset, add a new vector and verify the index is functional
|
|
std::vector<uint8_t> data2(n * idx.code_size);
|
|
data2[0] = 0x55;
|
|
data2[1] = 0xAA;
|
|
idx.add(n, data2.data());
|
|
EXPECT_EQ(idx.ntotal, 1);
|
|
|
|
idx.search(1, data2.data(), k, distances.data(), labels.data());
|
|
EXPECT_EQ(distances[0], 0);
|
|
EXPECT_EQ(labels[0], 0);
|
|
}
|