[WIP] feature : add zero copy support for vector index #1032
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: foxspy 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 |
foxspy
changed the title
|
@foxspy 🔍 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!. |
foxspy
force-pushed
the
zero_copy_support
branch
3 times, most recently
from
5385cae to
dc5a6ac
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1032 +/- ##
=========================================
+ Coverage 0 72.81% +72.81%
=========================================
Files 0 82 +82
Lines 0 7529 +7529
=========================================
+ Hits 0 5482 +5482
- Misses 0 2047 +2047 |
| @@ -81,6 +82,8 @@ struct Index { | |||
| MetricType metric_type; | |||
| float metric_arg; ///< argument of the metric type | |||
|
|
|||
| std::shared_ptr<MmappedFileMappingOwner> mmap_owner; | |||
There was a problem hiding this comment.
this is a very tricky point. Although, I understand why this was introduced, I'd like to avoid adding it here. Basically, the approach is similar to a std::string_view, which does not have a business to maintain the memory. Similar, here: It is not Index's instance business to maintain the view.
I'm open for discussions.
There was a problem hiding this comment.
@foxspy any comments on this? Basically, I doubt that we'll able to introduce this variable in vanilla Faiss when porting back
| } | ||
| } | ||
|
|
||
| ZeroCopyIOReader* zr = dynamic_cast<ZeroCopyIOReader*>(f); |
There was a problem hiding this comment.
this is a todo for the future: basically, a reader need to instantiate an object. But this is a later business, because it needs to be properly synchronized with the vanilla faiss first.
So, let's keep the change in the way you proposed it.
There was a problem hiding this comment.
Yes, so I put the PR up first; zero copy is needed here mainly for
- Optimizing memory expansion caused by copying during loading
- The memory will be managed uniformly through Eyrie later, and the index does not hold data, but only views
- The implementation of mmap and zero copy can be unified (mmap externally first, load through zero copy)
If you have any ideas about this, please feel free to communicate; has faiss considered supporting this mode?
foxspy
force-pushed
the
zero_copy_support
branch
from
dc5a6ac to
7dc2032
Compare
foxspy
force-pushed
the
zero_copy_support
branch
2 times, most recently
from
b58a68a to
91cfcde
Compare
Signed-off-by: xianliang.li <[email protected]>
foxspy
force-pushed
the
zero_copy_support
branch
from
91cfcde to
771d36e
Compare
| if (binary == nullptr) { | ||
| LOG_KNOWHERE_ERROR_ << "Invalid binary set."; | ||
| return Status::invalid_binary_set; | ||
| } | ||
|
|
||
| MemoryIOReader reader(binary->data.get(), binary->size); | ||
| int io_flags = faiss::IO_FLAG_ZERO_COPY; | ||
| faiss::ZeroCopyIOReader reader(binary->data.get(), binary->size); |
There was a problem hiding this comment.
Such an approach won't lead to a memory leak, correct?
There was a problem hiding this comment.
The binary will be held by the index; so there will be no memory leak
| @@ -81,6 +82,8 @@ struct Index { | |||
| MetricType metric_type; | |||
| float metric_arg; ///< argument of the metric type | |||
|
|
|||
| std::shared_ptr<MmappedFileMappingOwner> mmap_owner; | |||
There was a problem hiding this comment.
@foxspy any comments on this? Basically, I doubt that we'll able to introduce this variable in vanilla Faiss when porting back
|
|
||
| namespace faiss { | ||
|
|
||
| ZeroCopyIOReader::ZeroCopyIOReader(uint8_t* data, size_t size) : data_(data), rp_(0), total_(size) { |
There was a problem hiding this comment.
A duplicate code, similar to src/io/memory_io.h
There was a problem hiding this comment.
This is because Knowhere is a hub that is adapted to multiple indexes. For indexes other than faiss, it uses the memory_io.h. Faiss needs to remain independent zerocopy_io.cpp and not rely on related objects of knowhere.
|
Zero copy may cause data misalignment, because data serialization in Faiss does not require alignment, which may cause errors or even crashes in some scenarios. @alexanderguzhva Do you have any suggestions for this? |
issue: #1031