-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Memory-mapping and Zero-copy deserializers #4199
base: main
Are you sure you want to change the base?
Memory-mapping and Zero-copy deserializers #4199
Conversation
Signed-off-by: Alexandr Guzhva <[email protected]>
Signed-off-by: Alexandr Guzhva <[email protected]>
Signed-off-by: Alexandr Guzhva <[email protected]>
Signed-off-by: Alexandr Guzhva <[email protected]>
Signed-off-by: Alexandr Guzhva <[email protected]>
Signed-off-by: Alexandr Guzhva <[email protected]>
@mdouze Would please be able to suggest a solution for the following problem. Because I changed the container type for |
Signed-off-by: Alexandr Guzhva <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this. See a few inline reviews.
As long as the interface of MaybeOwnedVector is sufficiently close of std::vector, it should work seamlessly in Python (duck typing). I'll see if some adaptations to the swig interface are required.
It is not clear to me why the MappedFileIOReader
is needed rather than the regular FileIOReader
. Is it to maintain the ownership of the reader within the MaybeOwnedVector
and close the file when deallocated?
impl/pq4_fast_scan.cpp | ||
impl/pq4_fast_scan_search_1.cpp | ||
impl/pq4_fast_scan_search_qbs.cpp | ||
impl/residual_quantizer_encode_steps.cpp | ||
impl/io.cpp | ||
impl/lattice_Zn.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do these two file disappear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicates
faiss/impl/index_read.cpp
Outdated
READVECTOR(target); | ||
} | ||
|
||
template <typename VectorT> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible to factorize with previous function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a good suggestion, let me see...
faiss/impl/index_read.cpp
Outdated
} | ||
|
||
template <typename VectorT> | ||
void read_vector(VectorT& target, IOReader* f) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to enable this via an IO flag ? The IVF memory mapped files work on regular FileIOReader
https://github.com/facebookresearch/faiss/blob/main/faiss/invlists/OnDiskInvertedLists.cpp#L771
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> OnDiskInvertedLists is unsupported on Windows.
my code allows mmap functionality on Windows as well. Let me take a look
@mdouze has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This patch should fix the python test: |
Signed-off-by: Alexandr Guzhva <[email protected]>
in order to have a correct support for Windows |
Signed-off-by: Alexandr Guzhva <[email protected]>
Signed-off-by: Alexandr Guzhva <[email protected]>
@mdouze has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Signed-off-by: Alexandr Guzhva <[email protected]>
@mdouze I'm going to add IVFs and maybe Binary indices today as well, so please don't merge it yet :) |
Signed-off-by: Alexandr Guzhva <[email protected]>
Signed-off-by: Alexandr Guzhva <[email protected]>
Signed-off-by: Alexandr Guzhva <[email protected]>
Signed-off-by: Alexandr Guzhva <[email protected]>
Signed-off-by: Alexandr Guzhva <[email protected]>
Signed-off-by: Alexandr Guzhva <[email protected]>
@mdouze please let me know which other unit tests are useful to have |
the PR is ready to be reviewed |
@mdouze has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This PR introduces a backport of a combination of zilliztech/knowhere#996 and zilliztech/knowhere#1032 that allow to have memory-mapped and zerocopy indces.
The root underlying idea is that we replace certain
std::vector<>
containers with a customfaiss::MaybeOwnedVector<>
container, which may behave either asstd::vector<>
, or as a view of a certain pointer / descriptor. We don't replace all the instances ofstd::vector<>
, but the largest ones.This change affects
IndexFlatCodes
-based andIndexHNSW
CPU indices.(done) alter IVF lists as well.
(done) alter binary indices as well.
Memory-mapped index works like this:
In theory, it should be ready to be used from Python. All the descriptor management should be working.
Zero-copy index works like this:
All the pointer management for
faiss::ZeroCopyIOReader
should be handled manually.I'm not sure how to plug this into Python yet, maybe, some ref-counting is required.
(done) some refactoring