Skip to content

[MMAP][CPPRuntime] Extend map_to_memory API to include output read_bytes param#341

Open
rfsaliev wants to merge 1 commit into
mainfrom
rfsaliev/cpp-extend-memmap
Open

[MMAP][CPPRuntime] Extend map_to_memory API to include output read_bytes param#341
rfsaliev wants to merge 1 commit into
mainfrom
rfsaliev/cpp-extend-memmap

Conversation

@rfsaliev
Copy link
Copy Markdown
Member

This pull request enhances the in-memory mapping functionality for both FlatIndex and VamanaIndex by allowing the number of bytes read during deserialization to be reported. It also adds comprehensive tests to verify the correct behavior of this feature across different index types and storage kinds. The changes improve robustness and observability of index loading from memory buffers.

API Enhancements

  • The map_to_memory methods for both FlatIndex and VamanaIndex now accept an optional size_t* read_bytes parameter, allowing callers to determine how many bytes were consumed during mapping. [1] [2] [3] [4]

  • Implementation of get_mapped_stream() accessor in both FlatIndexImpl and VamanaIndexImpl to expose the stream position for calculating read_bytes. [1] [2]

Testing Improvements

  • Added a generic test helper, write_and_map_index_from_memory, to build, serialize, and map indices from in-memory buffers, checking that read_bytes is set correctly and that the loaded index is functional.

  • Introduced new test cases for mapping from memory for all major index/storage kind combinations:

    • FlatIndexWriteAndMapToMemory
    • WriteAndMapToMemoryStaticIndexSVS
    • WriteAndMapToMemoryStaticIndexSVSLVQ4x4
    • WriteAndMapToMemoryStaticIndexSVSLeanVec4x4

Robustness

  • All new tests check both the case when read_bytes is provided and when it is nullptr, ensuring backward compatibility and correct handling in both scenarios.

These changes together make the index mapping API more informative and thoroughly tested for different use cases.

Copy link
Copy Markdown
Member

@ethanglaser ethanglaser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants