improve sparse vector index mmap: to mmap almost everything #928
Conversation
|
@zhengbuqian 🔍 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!. |
|
kind/improvement |
|
issue: #927 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #928 +/- ##
=========================================
+ Coverage 0 74.35% +74.35%
=========================================
Files 0 82 +82
Lines 0 6508 +6508
=========================================
+ Hits 0 4839 +4839
- Misses 0 1669 +1669 |
include/knowhere/sparse_utils.h
Outdated
| // | ||
| // Currently only used in sparse InvertedIndex. Move to other places if needed. | ||
| template <typename T> | ||
| class MmapVector { |
There was a problem hiding this comment.
what is the difference between MmapVector and https://en.cppreference.com/w/cpp/container/span ?
Also, technically, this is not a MmapVector, but rather a VectorView (similar to the relation between std::string and std::string_view)
There was a problem hiding this comment.
CMIIW std::span is more like referencing a list of initialized objects? so std::span()::size() always equals to its capacity. while in this case the contents of the containers are built up gradually using emplace_back and size() grows from 0 to capacity. though the initialization is still all done in the Load method.
If we want to use std::span:
template <typename T>
class FixedVector : public std::span<T> {
emplace_back() { // to have a consistent API like vector so we can mix the usage with std::vector
}
size() override {
// return number of constructed elements instead of `byte_size / sizeof(T)`.
}
}Another option is to avoid overloading the meaning of size()(and also avoid subclassing it) by first placement new all objects on the raw ptr, and then just create the spans out of them. But then we won't be able to reuse add_row_to_index.
WDYT?
And yes it is not a MmapedVector indeed. But it is also not a view: it is more like an append-only vector with pre-allocated unextendable memory. How about FixedVector?
There was a problem hiding this comment.
I see. Basically, I got confused by the name of this class :) It totally makes sense to have this separate entity.
Naming is tricky
- it must include the word
view/spanto indicate that an object instance does not own the memory (unlike vector or array that do) - the word
vectorneeds to be included to indicate that this is a contiguous data chunk - the word
arrayis less appropriate, bcz it usually indicates a fixed size, not kindagrowableas of in our use case
Maybe, something like VectorLikeView or GrowableVectorView?
There was a problem hiding this comment.
Using GrowableVectorView, liking it over others.
| // reset reader to the beginning | ||
| reader.seekg(initial_reader_location); | ||
|
|
||
| auto raw_data_size = rows * sizeof(typename decltype(raw_data_)::value_type); |
There was a problem hiding this comment.
maybe, it would be nice if variables could be called as raw_data_size_in_bytes rather than raw_data_size, because the default convension is that *_size is measured in elements, not in bytes.
No big deal, but it just improves the readibility of the code.
Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>
|
@zhengbuqian please let me know if I should lgtm this diff. It looks good, overall. |
buqian-zilliz
force-pushed
the
sparse-mmap
branch
from
b3bd8b5 to
a11a6cd
Compare
it is ready now, thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liliu-z, zhengbuqian The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/kind improvement |
…ch#928) Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>
…ch#928) Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com> Signed-off-by: xianliang.li <xianliang.li@zilliz.com>
* update raft to 24.10 (#914) Signed-off-by: yusheng.ma <yusheng.ma@zilliz.com> Signed-off-by: xianliang.li <xianliang.li@zilliz.com> * fix Index parameters handling and anniterator (#913) Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com> Signed-off-by: xianliang.li <xianliang.li@zilliz.com> * add range check (#915) Signed-off-by: xianliang.li <xianliang.li@zilliz.com> * fix knowhere ut (#918) Signed-off-by: xianliang.li <xianliang.li@zilliz.com> * raft index supports cosine similarity by normalizing the input data. (#924) Signed-off-by: yusheng.ma <yusheng.ma@zilliz.com> Signed-off-by: xianliang.li <xianliang.li@zilliz.com> * compensate for the missing acceleration functions in ARM NEON. (#922) Signed-off-by: yusheng.ma <yusheng.ma@zilliz.com> Signed-off-by: xianliang.li <xianliang.li@zilliz.com> * improve sparse vector index mmap: to mmap almost everything (#928) Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com> Signed-off-by: xianliang.li <xianliang.li@zilliz.com> * move sparse index Add to build pool (#933) Add SparseInvertedIndexNodeCC to allow being thread safe growing index Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com> Signed-off-by: xianliang.li <xianliang.li@zilliz.com> * sparse mmap on disk (#935) Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com> Signed-off-by: xianliang.li <xianliang.li@zilliz.com> * use MAP_PRIVATE for mmapped file (#938) Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com> Signed-off-by: xianliang.li <xianliang.li@zilliz.com> * sparse RangeSearch/AnnIterator to return raw distance (#944) * sparse: make the distance returned by RangeSearch and AnnIterator to be the raw instead of the quantized distance Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com> * sparse: remove mutex in the index: we now use CC index if concurrent read/write is needed Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com> --------- Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com> Signed-off-by: xianliang.li <xianliang.li@zilliz.com> * Add optimized distance functions for PowerPC (#894) Added the PowerPC vector functions in src/simd/distances_powerpc.cc, src/simd/distances_powerpc.h. The hooks to the PowerPC functions are added in src/simd/hook.cc. Signed-off-by: Carl Love <cel@us.ibm.com> Co-authored-by: Carl Love <cel@us.ibm.com> Signed-off-by: xianliang.li <xianliang.li@zilliz.com> * enhance: optimize get norms function (#950) Signed-off-by: cqy123456 <qianya.cheng@zilliz.com> Signed-off-by: xianliang.li <xianliang.li@zilliz.com> --------- Signed-off-by: yusheng.ma <yusheng.ma@zilliz.com> Signed-off-by: xianliang.li <xianliang.li@zilliz.com> Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com> Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com> Signed-off-by: Carl Love <cel@us.ibm.com> Signed-off-by: cqy123456 <qianya.cheng@zilliz.com> Co-authored-by: presburger <yusheng.ma@zilliz.com> Co-authored-by: Alexander Guzhva <alexanderguzhva@gmail.com> Co-authored-by: Buqian Zheng <zhengbuqian@gmail.com> Co-authored-by: carll99 <161512392+carll99@users.noreply.github.com> Co-authored-by: Carl Love <cel@us.ibm.com> Co-authored-by: cqy123456 <39671710+cqy123456@users.noreply.github.com>
issue: #927
kind/improvement