Add deferred flusher to Gridstore by timvisee · Pull Request #7446 · qdrant/qdrant

timvisee · 2025-10-23T15:24:17Z

Add a deferred flusher to Gridstore, using which we flush asynchronously through a closure.

This makes the flushing behavior of Gridstore consistent with the other storage types we have.

Specifically this:

takes a copy of pending updates in the tracker
deferred, in a closure:
a. write the copied updates to disk (ignoring any new updates)
b. bump the list of pending updates to remove the now persisted entries

This is a building block I need for #7443.

As a bonus this removes all Arc<RwLock<_>> wrappers around Gridstore, making the world a nicer place 🍀. It now also respects &self/&mut self.

Crasher run: https://github.com/qdrant/crasher/actions/runs/18755857443 🟢

All Submissions:

Contributions should target the dev branch. Did you create your branch from dev?
Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

lib/gridstore/src/gridstore.rs

timvisee · 2025-10-23T16:51:33Z

lib/gridstore/src/gridstore.rs

+
+    /// Create flusher that durably persists all pending changes when invoked
+    pub fn flusher(&self) -> Flusher {
+        let pending_updates = self.tracker.read().pending_updates.clone();


Key change: here we now take a snapshot of all pending changes. Only these are flushed. This is consistent with our other flushers.

timvisee · 2025-10-23T16:52:56Z

lib/gridstore/src/tracker.rs

+            old_pointers.extend(updates.to_outdated_pointers());
+
+            // Bump the pending updates for this point offset, drop entry if fully persisted
+            if let Some(pending_updates) = self.pending_updates.get_mut(&point_offset)
+                && pending_updates.drain_persisted_and_drop(&updates)
+            {
+                self.pending_updates.remove(&point_offset);
+            }


This function now takes the snapshot of pending changes.

Here we bump the latest pending changes, so that we drain all changes that we now persist.

Once this function is complete, the list of pending changes will only contain changes that still need to be persisted.

timvisee · 2025-10-24T10:44:00Z

This now has a bunch of tests added to ensure correct behavior. The tests are actually the majority of this PR.

agourlay · 2025-10-24T15:10:22Z

lib/gridstore/benches/flush_bench.rs

                    let instant = Instant::now();

-                    storage.flush().unwrap();
+                    storage.flusher()().unwrap();


Could you please run the flush_bench for a quick sanity check that the performance is similar to dev 🤞

It is worse than I expected. It may just be because now there's extra work. Not sure yet if we have to be very concerned about it.

I have done some basic profiling on the benchmark and I cannot find a clear root cause for the degradation.

The previous code was sub-millisecond fast already, maybe the additional locking makes things slower 🤷

I think this is fine.

10 μs even for 100 updates. Likely measuring locks.

Agreed 👍

bustle_bench does not show any noticeable degradation 👍

timvisee · 2025-10-31T13:36:13Z

Let's postpone merging this till after the 1.16 release. Then we can test it on chaos testing for some time without the risk of affecting the release.

* Wrap the tracker and bitmask in Arc * Implement deferred flusher for gridstore, defer tracker writes * Remove clone * Dynamically adjust list of pending updates to drain what is flushed * Return proper flusher, defer premature Gridstore flushing errors * Use deferred Gridstore flusher across storages * Remove all Arc<RwLock<_>> wrappers around Gridstore in storages * Flush pages inside closure, also wrap them in a lock * Remove direct flush function from Gridstore * Add test to assert behavior of deferred flushing in Gridstore * Add much more extensive test, including concurrent flushes and deletes * Add test for draining value pointer, make sure to drop when flushed * Feature gate RocksDB components

github-actions bot mentioned this pull request Oct 23, 2025

Flaky test tests::snapshot_test::test_snapshot_collection_listener #5218

Open

timvisee force-pushed the gridstore-flush-closure branch 2 times, most recently from c953108 to e98073c Compare October 23, 2025 16:47

timvisee marked this pull request as ready for review October 23, 2025 16:47

timvisee commented Oct 23, 2025

View reviewed changes

lib/gridstore/src/gridstore.rs Outdated Show resolved Hide resolved

timvisee force-pushed the gridstore-flush-closure branch from e98073c to 8fb001f Compare October 23, 2025 16:50

timvisee commented Oct 23, 2025

View reviewed changes

timvisee requested review from agourlay, coszio and generall October 23, 2025 16:53

qdrant deleted a comment from coderabbitai bot Oct 23, 2025

This comment was marked as resolved.

Sign in to view

qdrant deleted a comment from coderabbitai bot Oct 23, 2025

This comment was marked as resolved.

Sign in to view

timvisee force-pushed the gridstore-flush-closure branch from bda5571 to 2181b95 Compare October 24, 2025 10:43

qdrant deleted a comment from coderabbitai bot Oct 24, 2025

timvisee force-pushed the gridstore-flush-closure branch from 2181b95 to 4738476 Compare October 24, 2025 10:49

This comment was marked as resolved.

Sign in to view

github-actions bot mentioned this pull request Oct 24, 2025

Flaky test save_on_disk::tests::test_wait_for_condition_change #7114

Open

qdrant deleted a comment from coderabbitai bot Oct 24, 2025

agourlay reviewed Oct 24, 2025

View reviewed changes

coszio mentioned this pull request Oct 24, 2025

[#7446] Refactor draining pointer updates #7452

Closed

This comment was marked as resolved.

Sign in to view

qdrant deleted a comment from coderabbitai bot Oct 27, 2025

timvisee added the do not merge Pull requests blocked on an external event label Oct 31, 2025

Wrap the tracker and bitmask in Arc

b3850f5

timvisee added 12 commits November 18, 2025 13:25

Implement deferred flusher for gridstore, defer tracker writes

27fc7c4

Remove clone

5bfe939

Dynamically adjust list of pending updates to drain what is flushed

9e3e6c0

Return proper flusher, defer premature Gridstore flushing errors

c03f5d3

Use deferred Gridstore flusher across storages

48c9fc4

Remove all Arc<RwLock<_>> wrappers around Gridstore in storages

62eab59

Flush pages inside closure, also wrap them in a lock

fa60b5d

Remove direct flush function from Gridstore

5f1024b

Add test to assert behavior of deferred flushing in Gridstore

3979534

Add much more extensive test, including concurrent flushes and deletes

e6c6926

Add test for draining value pointer, make sure to drop when flushed

9a86b2a

Feature gate RocksDB components

d482fa7

timvisee force-pushed the gridstore-flush-closure branch from 575ec80 to d482fa7 Compare November 18, 2025 12:30

This comment was marked as resolved.

Sign in to view

qdrant deleted a comment from coderabbitai bot Nov 18, 2025

timvisee removed the do not merge Pull requests blocked on an external event label Nov 18, 2025

timvisee requested a review from agourlay November 18, 2025 13:11

agourlay approved these changes Nov 18, 2025

View reviewed changes

timvisee merged commit 8bbeda5 into dev Nov 18, 2025
21 of 22 checks passed

timvisee deleted the gridstore-flush-closure branch November 18, 2025 13:21

timvisee mentioned this pull request Nov 25, 2025

Bump version to 1.16.1 #7603

Merged

Conversation

timvisee commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes to Core Features:

Uh oh!

Uh oh!

timvisee Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timvisee Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

timvisee commented Oct 24, 2025

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

agourlay Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

timvisee Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

agourlay Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

timvisee Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

coszio Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

timvisee commented Oct 31, 2025

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timvisee commented Oct 23, 2025 •

edited

Loading

timvisee Oct 23, 2025 •

edited

Loading