Use BitvecFlags in vector storages (#6994)

* put all flags impls in a single module

* use in appendable_dense_vector_storage

* use in appendable_mmap_multi_dense_storage

* use in mmap_sparse_vector_storage

* clippy

* Add comment we cannot rely on length of deleted vectors bitvec

---------

Co-authored-by: timvisee <tim@visee.me>

lib/segment/benches/dynamic_mmap_flags.rs

@@ -5,8 +5,8 @@ use std::sync::atomic::AtomicBool;
 use criterion::{Criterion, criterion_group, criterion_main};
 use rand::rngs::StdRng;
 use rand::{Rng, SeedableRng};
+use segment::common::flags::dynamic_mmap_flags::DynamicMmapFlags;
 use segment::common::operation_error::check_process_stopped;
-use segment::vector_storage::dense::dynamic_mmap_flags::DynamicMmapFlags;
 use tempfile::tempdir;
 
 const FLAG_COUNT: usize = 50_000_000;

lib/segment/src/common/flags/bitvec_flags.rs

@@ -4,10 +4,10 @@ use bitvec::slice::BitSlice;
 use bitvec::vec::BitVec;
 use common::types::PointOffsetType;
 
+use super::buffered_dynamic_flags::BufferedDynamicFlags;
+use super::dynamic_mmap_flags::DynamicMmapFlags;
 use crate::common::Flusher;
-use crate::common::buffered_dynamic_flags::BufferedDynamicFlags;
 use crate::common::operation_error::OperationResult;
-use crate::vector_storage::dense::dynamic_mmap_flags::DynamicMmapFlags;
 
 /// A buffered, growable, and persistent bitslice with a separate in-memory bitvec.
 ///
@@ -16,6 +16,7 @@ use crate::vector_storage::dense::dynamic_mmap_flags::DynamicMmapFlags;
 /// Changes are buffered until explicitly flushed.
 ///
 /// [1]: super::roaring_flags::RoaringFlags
+#[derive(Debug)]
 pub struct BitvecFlags {
     /// Buffered persisted flags.
     storage: BufferedDynamicFlags,
@@ -71,17 +72,19 @@ impl BitvecFlags {
             .map(|index| index as PointOffsetType)
     }
 
+    #[inline]
     pub fn count_trues(&self) -> usize {
         self.bitvec.count_ones()
     }
 
+    #[inline]
     pub fn count_falses(&self) -> usize {
         self.bitvec.count_zeros()
     }
 
-    /// Set the value of a flag at the given index.
+    /// Set the value of a flag at the given index, grows the bitvec if needed.
     /// Returns the previous value of the flag.
-    pub fn set(&mut self, index: PointOffsetType, value: bool) {
+    pub fn set(&mut self, index: PointOffsetType, value: bool) -> bool {
         // queue write in buffer
         self.storage.buffer_set(index, value);
 
@@ -93,7 +96,7 @@ impl BitvecFlags {
         }
 
         // update bitmap
-        self.bitvec.set(index_usize, value)
+        self.bitvec.replace(index_usize, value)
     }
 
     pub fn clear_cache(&self) -> OperationResult<()> {
@@ -114,8 +117,8 @@ impl BitvecFlags {
 mod tests {
     use common::types::PointOffsetType;
 
-    use crate::common::bitvec_flags::BitvecFlags;
-    use crate::vector_storage::dense::dynamic_mmap_flags::DynamicMmapFlags;
+    use crate::common::flags::bitvec_flags::BitvecFlags;
+    use crate::common::flags::dynamic_mmap_flags::DynamicMmapFlags;
 
     #[test]
     fn test_roaring_flags_consistency_after_persistence() {

lib/segment/src/common/flags/buffered_dynamic_flags.rs

@@ -5,13 +5,14 @@ use ahash::AHashMap;
 use common::types::PointOffsetType;
 use parking_lot::{Mutex, RwLock};
 
+use super::dynamic_mmap_flags::DynamicMmapFlags;
 use crate::common::Flusher;
 use crate::common::operation_error::OperationResult;
-use crate::vector_storage::dense::dynamic_mmap_flags::DynamicMmapFlags;
 
 /// A buffered wrapper around DynamicMmapFlags that provides manual flushing, without interface for reading.
 ///
 /// Changes are buffered until explicitly flushed.
+#[derive(Debug)]
 pub(crate) struct BufferedDynamicFlags {
     /// Persisted flags.
     storage: Arc<Mutex<DynamicMmapFlags>>,
@@ -83,8 +84,8 @@ mod tests {
     use rand::rngs::StdRng;
     use rand::{Rng, SeedableRng};
 
-    use crate::common::buffered_dynamic_flags::BufferedDynamicFlags;
-    use crate::vector_storage::dense::dynamic_mmap_flags::DynamicMmapFlags;
+    use crate::common::flags::buffered_dynamic_flags::BufferedDynamicFlags;
+    use crate::common::flags::dynamic_mmap_flags::DynamicMmapFlags;
 
     #[test]
     fn test_buffered_flags_growth_persistence() {

lib/segment/src/common/flags/mod.rs

@@ -0,0 +1,12 @@
+//! Different flavors of flags structures, akin to a Vec<bool>, but persistent and efficient.
+//!
+//! Here's a brief overview of the different flavors of flags structures:
+//! - `dynamic_mmap_flags`: Base implementation of storage in mmapped files.
+//! - `buffered_dynamic_flags`: Builds on top of `dynamic_mmap_flags` to provide buffered writes.
+//! - `bitvec_flags`: `buffered_dynamic_flags` with in-memory bitvec for reads.
+//! - `roaring_flags`: `buffered_dynamic_flags` with in-memory roaring bitmap for reads.
+
+pub mod bitvec_flags;
+mod buffered_dynamic_flags;
+pub mod dynamic_mmap_flags;
+pub mod roaring_flags;

lib/segment/src/common/flags/roaring_flags.rs

@@ -3,10 +3,10 @@ use std::path::PathBuf;
 use common::types::PointOffsetType;
 use roaring::RoaringBitmap;
 
+use super::buffered_dynamic_flags::BufferedDynamicFlags;
+use super::dynamic_mmap_flags::DynamicMmapFlags;
 use crate::common::Flusher;
-use crate::common::buffered_dynamic_flags::BufferedDynamicFlags;
 use crate::common::operation_error::OperationResult;
-use crate::vector_storage::dense::dynamic_mmap_flags::DynamicMmapFlags;
 
 /// A buffered, growable, and persistent bitslice with fast in-memory roaring bitmap.
 ///
@@ -117,8 +117,8 @@ impl RoaringFlags {
 mod tests {
     use common::types::PointOffsetType;
 
-    use crate::common::roaring_flags::RoaringFlags;
-    use crate::vector_storage::dense::dynamic_mmap_flags::DynamicMmapFlags;
+    use crate::common::flags::dynamic_mmap_flags::DynamicMmapFlags;
+    use crate::common::flags::roaring_flags::RoaringFlags;
 
     #[test]
     fn test_roaring_flags_consistency_after_persistence() {

lib/segment/src/common/mod.rs

@@ -1,14 +1,12 @@
 pub mod anonymize;
-pub mod bitvec_flags;
-pub mod buffered_dynamic_flags;
 pub mod error_logging;
+pub mod flags;
 pub mod macros;
 pub mod mmap_bitslice_buffered_update_wrapper;
 pub mod mmap_slice_buffered_update_wrapper;
 pub mod operation_error;
 pub mod operation_time_statistics;
 pub mod reciprocal_rank_fusion;
-pub mod roaring_flags;
 #[cfg(feature = "rocksdb")]
 pub mod rocksdb_buffered_delete_wrapper;
 #[cfg(feature = "rocksdb")]

lib/segment/src/index/field_index/bool_index/mutable_bool_index.rs

@@ -7,8 +7,9 @@ use itertools::Either;
 use roaring::RoaringBitmap;
 
 use super::BoolIndex;
+use crate::common::flags::dynamic_mmap_flags::DynamicMmapFlags;
+use crate::common::flags::roaring_flags::RoaringFlags;
 use crate::common::operation_error::{OperationError, OperationResult};
-use crate::common::roaring_flags::RoaringFlags;
 use crate::index::field_index::map_index::IdIter;
 use crate::index::field_index::{
     CardinalityEstimation, FieldIndexBuilderTrait, PayloadBlockCondition, PayloadFieldIndex,
@@ -16,7 +17,6 @@ use crate::index::field_index::{
 };
 use crate::telemetry::PayloadIndexTelemetry;
 use crate::types::{FieldCondition, Match, MatchValue, PayloadKeyType, ValueVariants};
-use crate::vector_storage::dense::dynamic_mmap_flags::DynamicMmapFlags;
 
 const TRUES_DIRNAME: &str = "trues";
 const FALSES_DIRNAME: &str = "falses";

lib/segment/src/index/field_index/null_index/mutable_null_index.rs

@@ -5,8 +5,9 @@ use common::types::PointOffsetType;
 use serde_json::Value;
 
 use crate::common::Flusher;
+use crate::common::flags::dynamic_mmap_flags::DynamicMmapFlags;
+use crate::common::flags::roaring_flags::RoaringFlags;
 use crate::common::operation_error::{OperationError, OperationResult};
-use crate::common::roaring_flags::RoaringFlags;
 use crate::index::field_index::{
     CardinalityEstimation, FieldIndexBuilderTrait, PayloadBlockCondition, PayloadFieldIndex,
     PrimaryCondition,
@@ -14,7 +15,6 @@ use crate::index::field_index::{
 use crate::index::payload_config::{IndexMutability, StorageType};
 use crate::telemetry::PayloadIndexTelemetry;
 use crate::types::{FieldCondition, PayloadKeyType};
-use crate::vector_storage::dense::dynamic_mmap_flags::DynamicMmapFlags;
 
 const HAS_VALUES_DIRNAME: &str = "has_values";
 const IS_NULL_DIRNAME: &str = "is_null";

lib/segment/src/vector_storage/dense/appendable_dense_vector_storage.rs

@@ -12,6 +12,8 @@ use common::types::PointOffsetType;
 use memory::madvise::AdviceSetting;
 
 use crate::common::Flusher;
+use crate::common::flags::bitvec_flags::BitvecFlags;
+use crate::common::flags::dynamic_mmap_flags::DynamicMmapFlags;
 use crate::common::operation_error::{OperationResult, check_process_stopped};
 use crate::data_types::named_vectors::CowVector;
 use crate::data_types::primitive::PrimitiveVectorElement;
@@ -20,7 +22,6 @@ use crate::types::{Distance, VectorStorageDatatype};
 use crate::vector_storage::chunked_mmap_vectors::ChunkedMmapVectors;
 use crate::vector_storage::chunked_vector_storage::{ChunkedVectorStorage, VectorOffsetType};
 use crate::vector_storage::common::VECTOR_READ_BATCH_SIZE;
-use crate::vector_storage::dense::dynamic_mmap_flags::DynamicMmapFlags;
 use crate::vector_storage::in_ram_persisted_vectors::InRamPersistedVectors;
 use crate::vector_storage::{DenseVectorStorage, VectorStorage, VectorStorageEnum};
 
@@ -30,7 +31,11 @@ const DELETED_DIR_PATH: &str = "deleted";
 #[derive(Debug)]
 pub struct AppendableMmapDenseVectorStorage<T: PrimitiveVectorElement, S: ChunkedVectorStorage<T>> {
     vectors: S,
-    deleted: DynamicMmapFlags,
+    /// Flags marking deleted vectors
+    ///
+    /// Structure grows dynamically, but may be smaller than actual number of vectors. Must not
+    /// depend on its length.
+    deleted: BitvecFlags,
     distance: Distance,
     deleted_count: usize,
     _phantom: std::marker::PhantomData<T>,
@@ -39,27 +44,28 @@ pub struct AppendableMmapDenseVectorStorage<T: PrimitiveVectorElement, S: Chunke
 impl<T: PrimitiveVectorElement, S: ChunkedVectorStorage<T>> AppendableMmapDenseVectorStorage<T, S> {
     /// Set deleted flag for given key. Returns previous deleted state.
     #[inline]
-    fn set_deleted(&mut self, key: PointOffsetType, deleted: bool) -> OperationResult<bool> {
+    fn set_deleted(&mut self, key: PointOffsetType, deleted: bool) -> bool {
         if !deleted && self.vectors.len() <= key as usize {
-            return Ok(false);
+            return false;
         }
 
-        if self.deleted.len() <= key as usize {
-            self.deleted.set_len(key as usize + 1)?;
-        }
+        // mark deletion
         let previous = self.deleted.set(key, deleted);
+
+        // update counter
         if !previous && deleted {
             self.deleted_count += 1;
         } else if previous && !deleted {
             self.deleted_count -= 1;
         }
-        Ok(previous)
+
+        previous
     }
 
     /// Populate all pages in the mmap.
     /// Block until all pages are populated.
     pub fn populate(&self) -> OperationResult<()> {
-        self.deleted.populate()?;
+        // deleted bitvec is already loaded
         self.vectors.populate()?;
         Ok(())
     }
@@ -152,7 +158,7 @@ impl<T: PrimitiveVectorElement, S: ChunkedVectorStorage<T>> VectorStorage
         let vector = T::slice_from_float_cow(Cow::from(vector));
         self.vectors
             .insert(key as VectorOffsetType, vector.as_ref(), hw_counter)?;
-        self.set_deleted(key, false)?;
+        self.set_deleted(key, false);
         Ok(())
     }
 
@@ -168,7 +174,7 @@ impl<T: PrimitiveVectorElement, S: ChunkedVectorStorage<T>> VectorStorage
             // Do not perform preprocessing - vectors should be already processed
             let other_vector = T::slice_from_float_cow(Cow::try_from(other_vector)?);
             let new_id = self.vectors.push(other_vector.as_ref(), &disposed_hw)?;
-            self.set_deleted(new_id as PointOffsetType, other_deleted)?;
+            self.set_deleted(new_id as PointOffsetType, other_deleted);
         }
         let end_index = self.vectors.len() as PointOffsetType;
         Ok(start_index..end_index)
@@ -197,7 +203,7 @@ impl<T: PrimitiveVectorElement, S: ChunkedVectorStorage<T>> VectorStorage
     }
 
     fn delete_vector(&mut self, key: PointOffsetType) -> OperationResult<bool> {
-        self.set_deleted(key, true)
+        Ok(self.set_deleted(key, true))
     }
 
     fn is_deleted_vector(&self, key: PointOffsetType) -> bool {
@@ -268,8 +274,8 @@ pub fn open_appendable_memmap_vector_storage_impl<T: PrimitiveVectorElement>(
         Some(populate),
     )?;
 
-    let deleted: DynamicMmapFlags = DynamicMmapFlags::open(&deleted_path, populate)?;
-    let deleted_count = deleted.count_flags();
+    let deleted = BitvecFlags::new(DynamicMmapFlags::open(&deleted_path, populate)?);
+    let deleted_count = deleted.count_trues();
 
     Ok(AppendableMmapDenseVectorStorage {
         vectors,
@@ -347,8 +353,8 @@ pub fn open_appendable_in_ram_vector_storage_impl<T: PrimitiveVectorElement>(
     let vectors = InRamPersistedVectors::<T>::open(&vectors_path, dim)?;
 
     let populate = true;
-    let deleted: DynamicMmapFlags = DynamicMmapFlags::open(&deleted_path, populate)?;
-    let deleted_count = deleted.count_flags();
+    let deleted = BitvecFlags::new(DynamicMmapFlags::open(&deleted_path, populate)?);
+    let deleted_count = deleted.count_trues();
 
     Ok(AppendableMmapDenseVectorStorage {
         vectors,

lib/segment/src/vector_storage/dense/mod.rs

@@ -1,5 +1,4 @@
 pub mod appendable_dense_vector_storage;
-pub mod dynamic_mmap_flags;
 pub mod memmap_dense_vector_storage;
 pub mod mmap_dense_vectors;
 #[cfg(feature = "rocksdb")]

lib/segment/src/vector_storage/multi_dense/appendable_mmap_multi_dense_vector_storage.rs

@@ -9,6 +9,8 @@ use common::types::PointOffsetType;
 use memory::madvise::AdviceSetting;
 
 use crate::common::Flusher;
+use crate::common::flags::bitvec_flags::BitvecFlags;
+use crate::common::flags::dynamic_mmap_flags::DynamicMmapFlags;
 use crate::common::operation_error::{OperationError, OperationResult, check_process_stopped};
 use crate::data_types::named_vectors::{CowMultiVector, CowVector};
 use crate::data_types::primitive::PrimitiveVectorElement;
@@ -16,7 +18,6 @@ use crate::data_types::vectors::{TypedMultiDenseVectorRef, VectorElementType, Ve
 use crate::types::{Distance, MultiVectorConfig, VectorStorageDatatype};
 use crate::vector_storage::chunked_mmap_vectors::ChunkedMmapVectors;
 use crate::vector_storage::chunked_vector_storage::{ChunkedVectorStorage, VectorOffsetType};
-use crate::vector_storage::dense::dynamic_mmap_flags::DynamicMmapFlags;
 use crate::vector_storage::in_ram_persisted_vectors::InRamPersistedVectors;
 use crate::vector_storage::{MultiVectorStorage, VectorStorage, VectorStorageEnum};
 
@@ -39,7 +40,11 @@ pub struct AppendableMmapMultiDenseVectorStorage<
 > {
     vectors: S,
     offsets: O,
-    deleted: DynamicMmapFlags,
+    /// Flags marking deleted vectors
+    ///
+    /// Structure grows dynamically, but may be smaller than actual number of vectors. Must not
+    /// depend on its length.
+    deleted: BitvecFlags,
     distance: Distance,
     multi_vector_config: MultiVectorConfig,
     deleted_count: usize,
@@ -54,26 +59,28 @@ impl<
 {
     /// Set deleted flag for given key. Returns previous deleted state.
     #[inline]
-    fn set_deleted(&mut self, key: PointOffsetType, deleted: bool) -> OperationResult<bool> {
+    fn set_deleted(&mut self, key: PointOffsetType, deleted: bool) -> bool {
         if !deleted && self.vectors.len() <= key as usize {
-            return Ok(false);
+            return false;
         }
 
-        if self.deleted.len() <= key as usize {
-            self.deleted.set_len(key as usize + 1)?;
-        }
+        // set value
         let previous = self.deleted.set(key, deleted);
+
+        // update counter
         if !previous && deleted {
             self.deleted_count += 1;
         } else if previous && !deleted {
             self.deleted_count -= 1;
         }
-        Ok(previous)
+
+        previous
     }
 
     /// Populate all pages in the mmap.
     /// Block until all pages are populated.
     pub fn populate(&self) -> OperationResult<()> {
+        // deleted bitvec is already loaded
         self.vectors.populate()?;
         self.offsets.populate()?;
         Ok(())
@@ -265,7 +272,7 @@ impl<
         )?;
         self.offsets
             .insert(key as VectorOffsetType, &[offset], hw_counter)?;
-        self.set_deleted(key, false)?;
+        self.set_deleted(key, false);
 
         Ok(())
     }
@@ -283,7 +290,7 @@ impl<
             let other_vector: VectorRef = other_vector.as_vec_ref();
             let new_id = self.offsets.len() as PointOffsetType;
             self.insert_vector(new_id, other_vector, &disposed_hw_counter)?;
-            self.set_deleted(new_id, other_deleted)?;
+            self.set_deleted(new_id, other_deleted);
         }
         let end_index = self.offsets.len() as PointOffsetType;
         Ok(start_index..end_index)
@@ -317,7 +324,7 @@ impl<
     }
 
     fn delete_vector(&mut self, key: PointOffsetType) -> OperationResult<bool> {
-        self.set_deleted(key, true)
+        Ok(self.set_deleted(key, true))
     }
 
     fn is_deleted_vector(&self, key: PointOffsetType) -> bool {
@@ -443,8 +450,8 @@ pub fn open_appendable_memmap_multi_vector_storage_impl<T: PrimitiveVectorElemen
         Some(populate),
     )?;
 
-    let deleted: DynamicMmapFlags = DynamicMmapFlags::open(&deleted_path, populate)?;
-    let deleted_count = deleted.count_flags();
+    let deleted = BitvecFlags::new(DynamicMmapFlags::open(&deleted_path, populate)?);
+    let deleted_count = deleted.count_trues();
 
     Ok(AppendableMmapMultiDenseVectorStorage {
         vectors,
@@ -555,8 +562,8 @@ pub fn open_appendable_in_ram_multi_vector_storage_impl<T: PrimitiveVectorElemen
     let vectors = InRamPersistedVectors::open(&vectors_path, dim)?;
     let offsets = InRamPersistedVectors::open(&offsets_path, 1)?;
 
-    let deleted: DynamicMmapFlags = DynamicMmapFlags::open(&deleted_path, populate)?;
-    let deleted_count = deleted.count_flags();
+    let deleted = BitvecFlags::new(DynamicMmapFlags::open(&deleted_path, populate)?);
+    let deleted_count = deleted.count_trues();
 
     Ok(AppendableMmapMultiDenseVectorStorage {
         vectors,

lib/segment/src/vector_storage/sparse/mmap_sparse_vector_storage.rs

@@ -12,26 +12,27 @@ use gridstore::config::{Compression, StorageOptions};
 use parking_lot::RwLock;
 use sparse::common::sparse_vector::SparseVector;
 
+use crate::common::flags::bitvec_flags::BitvecFlags;
+use crate::common::flags::dynamic_mmap_flags::DynamicMmapFlags;
 use crate::common::operation_error::{OperationError, OperationResult, check_process_stopped};
 use crate::data_types::named_vectors::CowVector;
 use crate::data_types::vectors::VectorRef;
 use crate::types::VectorStorageDatatype;
-use crate::vector_storage::dense::dynamic_mmap_flags::DynamicMmapFlags;
 use crate::vector_storage::sparse::stored_sparse_vectors::StoredSparseVector;
 use crate::vector_storage::{SparseVectorStorage, VectorStorage};
 
 const DELETED_DIRNAME: &str = "deleted";
 const STORAGE_DIRNAME: &str = "store";
 
-/// When resizing bitslice, grow by this extra amount.
-const BITSLICE_GROWTH_SLACK: usize = 1024;
-
 /// Memory-mapped mutable sparse vector storage.
 #[derive(Debug)]
 pub struct MmapSparseVectorStorage {
     storage: Arc<RwLock<Gridstore<StoredSparseVector>>>,
-    /// BitSlice for deleted flags. Grows dynamically upto last set flag.
-    deleted: DynamicMmapFlags, // TODO currently eagerly flushed outside of the flushing sequence
+    /// Flags marking deleted vectors
+    ///
+    /// Structure grows dynamically, but may be smaller than actual number of vectors. Must not
+    /// depend on its length.
+    deleted: BitvecFlags,
     /// Current number of deleted vectors.
     deleted_count: usize,
     /// Maximum point offset in the storage + 1. This also means the total amount of point offsets
@@ -64,9 +65,9 @@ impl MmapSparseVectorStorage {
 
         // Deleted flags
         let deleted_path = path.join(DELETED_DIRNAME);
-        let deleted = DynamicMmapFlags::open(&deleted_path, populate)?;
+        let deleted = BitvecFlags::new(DynamicMmapFlags::open(&deleted_path, populate)?);
 
-        let deleted_count = deleted.count_flags();
+        let deleted_count = deleted.count_trues();
         let next_point_offset = deleted
             .get_bitslice()
             .last_one()
@@ -105,7 +106,7 @@ impl MmapSparseVectorStorage {
 
         // Deleted flags
         let deleted_path = path.join(DELETED_DIRNAME);
-        let deleted = DynamicMmapFlags::open(&deleted_path, populate)?;
+        let deleted = BitvecFlags::new(DynamicMmapFlags::open(&deleted_path, populate)?);
 
         Ok(Self {
             storage: Arc::new(RwLock::new(storage)),
@@ -115,27 +116,14 @@ impl MmapSparseVectorStorage {
         })
     }
 
-    fn set_deleted_flag(&mut self, key: PointOffsetType, deleted: bool) -> OperationResult<bool> {
-        if (key as usize) < self.deleted.len() {
-            return Ok(self.deleted.set(key, deleted));
-        }
-
-        // Bitslice is too small; grow and set the deletion flag, but only if we need to set it to true.
-        if deleted {
-            self.deleted.set_len(key as usize + BITSLICE_GROWTH_SLACK)?;
-            return Ok(self.deleted.set(key, true));
-        }
-
-        Ok(false)
-    }
-
     #[inline]
-    fn set_deleted(&mut self, key: PointOffsetType, deleted: bool) -> OperationResult<bool> {
+    fn set_deleted(&mut self, key: PointOffsetType, deleted: bool) -> bool {
         if !deleted && key as usize >= self.next_point_offset {
-            return Ok(false);
+            return false;
         }
+
         // set deleted flag
-        let previous_value = self.set_deleted_flag(key, deleted)?;
+        let previous_value = self.deleted.set(key, deleted);
 
         // update deleted_count if it changed
         match (previous_value, deleted) {
@@ -143,7 +131,7 @@ impl MmapSparseVectorStorage {
             (true, false) => self.deleted_count = self.deleted_count.saturating_sub(1),
             _ => {}
         }
-        Ok(previous_value)
+        previous_value
     }
 
     fn update_stored(
@@ -175,7 +163,7 @@ impl MmapSparseVectorStorage {
     /// Populate all pages in the mmap.
     /// Block until all pages are populated.
     pub fn populate(&self) -> OperationResult<()> {
-        self.deleted.populate()?;
+        // deleted bitvec is already in-memory
         self.storage.read().populate()?;
         Ok(())
     }
@@ -258,7 +246,7 @@ impl VectorStorage for MmapSparseVectorStorage {
     ) -> OperationResult<()> {
         let vector = <&SparseVector>::try_from(vector)?;
         debug_assert!(vector.is_sorted(), "Vector is not sorted {vector:?}");
-        self.set_deleted(key, false)?;
+        self.set_deleted(key, false);
         self.update_stored(key, Some(vector), hw_counter)?;
         Ok(())
     }
@@ -277,7 +265,7 @@ impl VectorStorage for MmapSparseVectorStorage {
             let other_vector = other_vector.as_vec_ref().try_into()?;
             let new_id = self.next_point_offset as PointOffsetType;
             self.next_point_offset += 1;
-            self.set_deleted(new_id, other_deleted)?;
+            self.set_deleted(new_id, other_deleted);
 
             let vector = (!other_deleted).then_some(other_vector);
             self.update_stored(new_id, vector, &hw_counter)?;
@@ -317,7 +305,7 @@ impl VectorStorage for MmapSparseVectorStorage {
         &mut self,
         key: common::types::PointOffsetType,
     ) -> crate::common::operation_error::OperationResult<bool> {
-        let was_deleted = !self.set_deleted(key, true)?;
+        let was_deleted = !self.set_deleted(key, true);
 
         let hw_counter = HardwareCounterCell::disposable(); // Deletions not measured
         self.update_stored(key, None, &hw_counter)?;
@@ -326,7 +314,7 @@ impl VectorStorage for MmapSparseVectorStorage {
     }
 
     fn is_deleted_vector(&self, key: common::types::PointOffsetType) -> bool {
-        self.deleted.get(key as usize)
+        self.deleted.get(key)
     }
 
     fn deleted_vector_count(&self) -> usize {