[ENH]: Added functionality to use an in-memory intermediary to load #35
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tanujnay112
force-pushed
the
add_memory_bindings
branch
2 times, most recently
from
da6fa1c to
736bfd1
Compare
tanujnay112
changed the title
tanujnay112
force-pushed
the
add_memory_bindings
branch
2 times, most recently
from
77bc087 to
06f82f7
Compare
HammadB
reviewed
Aug 13, 2025
HammadB
reviewed
Aug 20, 2025
src/hnsw.rs
Outdated
| #[repr(C)] | ||
| pub struct HnswDataViewFFI { | ||
| _data: [u8; 0], | ||
| _marker: core::marker::PhantomData<(*mut u8, core::marker::PhantomPinned)>, |
HammadB
reviewed
Aug 20, 2025
src/hnsw.rs
Outdated
| // Create HnswData FFI structure and set the buffers | ||
| let ffi_ptr = unsafe { create_hnsw_data_view(header_buffer.as_ptr(), header_buffer.len(), data_level0_buffer.as_ptr(), data_level0_buffer.len(), length_buffer.as_ptr(), length_buffer.len(), link_list_buffer.as_ptr(), link_list_buffer.len()) }; | ||
| if ffi_ptr.is_null() { | ||
| return Err(HnswError::FFIException("Failed to create HnswData FFI structure".to_string())); |
There was a problem hiding this comment.
I think we've generally isolated the FFI exception to exceptions thrown in C++, that makes it clear to debug where the error is. Could we make this a custom error?
HammadB
reviewed
Aug 20, 2025
src/hnsw.rs
Outdated
|
|
||
| // Opaque struct for memory buffers | ||
| #[repr(C)] | ||
| pub struct HnswDataFFI { |
There was a problem hiding this comment.
naming nit - DataFFI vs HnswDataFFI, I know its HNSW
HammadB
reviewed
Aug 20, 2025
HammadB
reviewed
Aug 20, 2025
HammadB
reviewed
Aug 20, 2025
src/bindings.cpp
Outdated
| // Memory buffer management functions | ||
| hnswlib::HnswDataMut* create_mutable_hnsw_data(char* header_buffer, size_t header_size, char* data_level0_buffer, size_t data_level0_size, char* length_buffer, size_t length_size, char* link_list_buffer, size_t link_list_size) | ||
| { | ||
| return new hnswlib::HnswDataMut(header_buffer, header_size, data_level0_buffer, data_level0_size, length_buffer, length_size, link_list_buffer, link_list_size); |
There was a problem hiding this comment.
should we error handle here just defensively?
HammadB
reviewed
Aug 20, 2025
src/bindings.cpp
Outdated
|
|
||
| const hnswlib::HnswDataView* hnsw_data_to_view(hnswlib::HnswDataMut* hnsw_data) | ||
| { | ||
| return hnsw_data->getView(); |
There was a problem hiding this comment.
should we error handle here just defensively?
HammadB
reviewed
Aug 20, 2025
src/bindings.cpp
Outdated
|
|
||
| hnswlib::HnswDataView* create_hnsw_data_view(char* header_buffer, size_t header_size, char* data_level0_buffer, size_t data_level0_size, char* length_buffer, size_t length_size, char* link_list_buffer, size_t link_list_size) | ||
| { | ||
| return new hnswlib::HnswDataView(header_buffer, header_size, data_level0_buffer, data_level0_size, length_buffer, length_size, link_list_buffer, link_list_size); |
There was a problem hiding this comment.
should we error handle here just defensively?
HammadB
reviewed
Aug 20, 2025
HammadB
left a comment
HammadB
left a comment
There was a problem hiding this comment.
- Some of the size calcs seem descrepant (void * vs unsigned int)
- Reallocs need to happen the same way as the malloc changes
- The link list malloc is probably not OK we should alloc all memory on one side of the FFI boundary, ideally rust
tanujnay112
force-pushed
the
add_memory_bindings
branch
from
06f82f7 to
586ac7f
Compare
tanujnay112
commented
Aug 22, 2025
| mutable std::atomic<size_t> cur_element_count{0}; // current number of elements | ||
| size_t size_data_per_element_{0}; | ||
| size_t size_links_per_element_{0}; | ||
| unsigned int size_links_per_element_{0}; |
Author
There was a problem hiding this comment.
We've been serializing and reading this as unsigned int so just making it consistent here.
HammadB
reviewed
Aug 25, 2025
src/hnsw.rs
Outdated
| pub dimensionality: i32, | ||
| pub persist_path: PathBuf, | ||
| pub ef_search: usize, | ||
| pub hnsw_data: HnswData, |
There was a problem hiding this comment.
I think this shouldn't be in the config, its not really config
tanujnay112
force-pushed
the
add_memory_bindings
branch
from
586ac7f to
834a0ee
Compare
HammadB
approved these changes
Aug 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.