Repository navigation
DiskSeismic: Add disk_seismic_index - #32
Conversation
Signed-off-by: Zirui Song <zrsong@amazon.com>
74fbcb6 to
cf5ca78
Compare
|
| const IDSelector* id_selector, size_t element_size, | ||
| detail::TopKHolder<idx_t>& heap, | ||
| absl::flat_hash_set<idx_t>& visited) { | ||
| if (fwd != nullptr) { |
There was a problem hiding this comment.
what about throw exceptions if either fwd or vectors is null as I think you can do nothing without them
There was a problem hiding this comment.
It will throw, but in search() before entering OpenMP. If we throw in score_block(), it will go out of OMP parallel.
| const auto data = vectors->get_all_data(); | ||
| for (const idx_t doc_id : clusters[pl].get_docs(cid)) { | ||
| const idx_t start = data.indptr_data[doc_id]; | ||
| const size_t len = data.indptr_data[doc_id + 1] - start; |
There was a problem hiding this comment.
let's assign variables outside of for loop
const idx_t* const indptr = data.indptr_data;
const idx_t* const indices = data.indices_data;
const float* const values = data.values_data;
There was a problem hiding this comment.
Good suggestions. Ack
| throw_if_any_null(indptr, indices, values); | ||
| const size_t indptr_size = n + 1; | ||
| const size_t nnz = indptr[n]; | ||
| if (vectors_ == nullptr) { |
There was a problem hiding this comment.
reset num_vectors_ to 0 here?
| SearchParameters* search_parameters) | ||
| -> pair_of_score_id_vectors_t { | ||
| if (num_vectors_ == 0 || n == 0) { | ||
| return {std::vector<std::vector<float>>(n), |
There was a problem hiding this comment.
we use INVALID_IDX as not found doc, please check other indices
| dynamic_cast<const SeismicSearchParameters*>( | ||
| search_parameters)) { | ||
| cut = seismic_parameters->cut; | ||
| } |
There was a problem hiding this comment.
what about else situation?
There was a problem hiding this comment.
cut/k_prime has been initialized with default. else should be fine with nothing.
| search_parameters)) { | ||
| cut = seismic_parameters->cut; | ||
| } | ||
| if (k_prime <= 0) { |
There was a problem hiding this comment.
what if k_prime is smaller than k?
There was a problem hiding this comment.
k_prime here refers to the number of blocks to be fully examined. It has nothing to do with k.
| } | ||
|
|
||
| std::vector<std::vector<float>> result_distances(n); | ||
| std::vector<std::vector<idx_t>> result_labels(n); |
There was a problem hiding this comment.
here set default labels to 0
There was a problem hiding this comment.
Use INVALID_IDX now.
| // score. score_summaries_transposed dots the full query with each cluster | ||
| // summary, so scores are comparable across posting lists for the global | ||
| // ranking. | ||
| std::vector<BlockCandidate> candidates; |
There was a problem hiding this comment.
are sizes of both variables known already? can you call .reserve()
There was a problem hiding this comment.
Now add a pre-scan:
size_t total_clusters = 0;
size_t max_clusters = 0;
for (const term_t term : cuts) {
if (term < clustered_inverted_lists.size()) {
const size_t nc = clustered_inverted_lists[term].cluster_size();
total_clusters += nc;
max_clusters = std::max(max_clusters, nc);
}
}
candidates.reserve(total_clusters);
score_scratch.reserve(max_clusters);
There was a problem hiding this comment.
no, no, I mean if you already know the size, if not, we don't do this just to calculate the size and reserve which could be even heavier.
There was a problem hiding this comment.
Got it. That makes sense.
| } | ||
|
|
||
| for (size_t i = 0; i < q_len; ++i) { | ||
| dense[q_indices[i]] = 0.0F; |
There was a problem hiding this comment.
this for loop may have multiple cache misses, is the performance of this for loop fine?
There was a problem hiding this comment.
We only reset the number of q_len position in query, which is quite small. This is much cheaper than memset the whole dim table.
|
Thanks for @chishui 's suggestions, a benchmark result can be seen here: 1. Iso-recall readout along the two Pareto frontiers
Ratio > 1 = DiskSeismic faster. The p50 margin is 1.2–1.5× everywhere and narrows above recall 0.97; 2. Footprint
Exact sizes: 3. Two secondary results
|
So you pick the storage by building "seismic,…" vs "disk_seismic,…" — there's no store=inline|forward parameter on one index. |
| void DiskSeismicIndex::write_index(IOWriter* io_writer) { | ||
| const uint64_t nv = num_vectors_; | ||
| io_writer->write(const_cast<uint64_t*>(&nv), sizeof(uint64_t), 1); | ||
| SeismicInvertedListsWriter inv_list_writer(clustered_inverted_lists); |
There was a problem hiding this comment.
check if clustered list are written twice
There was a problem hiding this comment.
Good catch, partially. The summaries are written once (only in the invlists section) and the per-doc vectors are written once (only in the inline forward section — DiskSeismic drops plain seismic's full-CSR forward write). What is written twice is the per-cluster doc-id membership: docs_/offsets_ in the invlists section and again as per-block doc_id[] in the inline forward. And on the mmap search path the invlists docs_/offsets_ are never read (membership comes from the inline doc_id[]; only the summaries + cluster counts are used) — so that copy is dead weight on disk. It's ~M×4 bytes (M = total doc-cluster memberships, ~0.4% of the file here), so it's a cleanliness/correctness fix rather than a size lever (quantization is the size lever). I can add a summaries-only serialize/deserialize path for the DiskSeismic invlists so docs_/offsets_ aren't persisted — docs_ is still needed in RAM at build time to lay out the blocks, just not on disk.
But the cost of that change is not small. Maybe we can leave it a separate commit later.
b84974e to
b3183f2
Compare
| const detail::InlineForwardIndex* fwd = | ||
| fwd_.num_blocks() > 0 ? &fwd_ : nullptr; | ||
| const SparseVectors* vectors = fwd == nullptr ? vectors_.get() : nullptr; | ||
| const size_t element_size = fwd != nullptr |
There was a problem hiding this comment.
so here, can you throw when fwd and vectors are null?
There was a problem hiding this comment.
but can you quit early though?
There was a problem hiding this comment.
Done — switched from throwing to quitting early. The no-source case (fwd_.num_blocks()==0 && vectors_==nullptr) is now folded into the early-return at the top of search(), alongside num_vectors_==0 || n==0, returning the k-length padded empty result. A plain return is safe even though the scoring runs under OpenMP (only an escaping throw would be a problem), and it's checked once before the parallel region rather than per block.
Signed-off-by: Zirui Song <zrsong@amazon.com>
b3183f2 to
e76c7cd
Compare
chishui
left a comment
There was a problem hiding this comment.
Please update the duplicate serialization in next PR and also include a python integration tests.
Description
Added the main classes for DiskSeismicIndex
Issues Resolved
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.