Repository: qdrant/qdrant
File: lib/segment/src/id_tracker/mutable_id_tracker.rs (Rust)

Code before:
        let mappings_path = mappings_path(&self.segment_path);

        let changes = self.pending_mappings.lock().clone();

        if changes.is_empty() {
            return Box::new(|| Ok(()));
        }

        let is_alive_handle = self.is_alive_lock.handle();
        let pending_mappings_weak = Arc::downgrade(&self.pending_mappings);

        Box::new(move || {
            let (Some(is_alive_guard), Some(pending_mappings_arc)) = (
                is_alive_handle.lock_if_alive(),
                pending_mappings_weak.upgrade(),
            ) else {
                return Err(OperationError::cancelled(
                    "Aborted flushing on a dropped MutableIdTracker instance",
                ));
            };

            let pending_mappings_guard = pending_mappings_arc.lock();

            store_mapping_changes(&mappings_path, &changes)?;

            reconcile_persisted_mapping_changes(pending_mappings_guard, &changes);

            drop(is_alive_guard);

            Ok(())
        })
    }

    /// Creates a flusher function, that persists the removed points in the version database
    /// and flushes the version database to disk.
    /// This function should be called _after_ flushing the mapping database.
    fn versions_flusher(&self) -> Flusher {
        let versions_path = versions_path(&self.segment_path);
        let changes = self.pending_versions.lock().clone();

        if changes.is_empty() {
            return Box::new(|| Ok(()));
        }

        let pending_versions_weak = Arc::downgrade(&self.pending_versions);
        let is_alive_handle = self.is_alive_lock.handle();

        Box::new(move || {
            let (Some(is_alive_guard), Some(pending_versions_arc)) = (
                is_alive_handle.lock_if_alive(),
                pending_versions_weak.upgrade(),

Review comment (performance):
We should lock after storing.

Storing might take some time (on slow hardware). And we want to hold this lock for the shortest amount possible, since it affects all reads.

Same applies to version flusher below.

Code after:
        let mappings_path = mappings_path(&self.segment_path);

        let changes = {
            let changes_guard = self.pending_mappings.lock();
            if changes_guard.is_empty() {
                return Box::new(|| Ok(()));
            }
            changes_guard.clone()
        };

        let is_alive_handle = self.is_alive_lock.handle();
        let pending_mappings_weak = Arc::downgrade(&self.pending_mappings);

        Box::new(move || {
            let (Some(is_alive_guard), Some(pending_mappings_arc)) = (
                is_alive_handle.lock_if_alive(),
                pending_mappings_weak.upgrade(),
            ) else {
                return Ok(());
            };

            store_mapping_changes(&mappings_path, &changes)?;

            reconcile_persisted_mapping_changes(&pending_mappings_arc, &changes);

            drop(is_alive_guard);

            Ok(())
        })
    }

    /// Creates a flusher function, that persists the removed points in the version database
    /// and flushes the version database to disk.
    /// This function should be called _after_ flushing the mapping database.
    fn versions_flusher(&self) -> Flusher {
        let changes = {
            let changes_guard = self.pending_versions.lock();
            if changes_guard.is_empty() {
                return Box::new(|| Ok(()));
            }
            changes_guard.clone()
        };

        let versions_path = versions_path(&self.segment_path);

        let pending_versions_weak = Arc::downgrade(&self.pending_versions);
        let is_alive_handle = self.is_alive_lock.handle();

        Box::new(move || {
            let (Some(is_alive_guard), Some(pending_versions_arc)) = (
                is_alive_handle.lock_if_alive(),