feat(event cache): introduce an absolute local event ordering #5225

bnjbvr · 2025-06-12T16:16:01Z

This PR introduces a local absolute ordering for items of a linked chunk, or equivalently, for events within a room's timeline. The idea is to reuse the same underlying mechanism we had for AsVector, but restricting it to only counting the number of items in a chunk; given an item's Position, we can then compute its absolute order as the total number of items before its containing chunk + its index within the chunk.

This will help us order edits that would apply to a thread event, for instance; this is deferred to a future PR, to not make this one too heavyweight.

Attention to reviewers: sorry, this is a bulky PR (mostly because of tests), but I think it's important to see how the OrderTracker methods are used in 280bd32, to make sense of their raison d'être.

Part of #4869 / #5123.

codecov · 2025-06-12T16:29:45Z

Codecov Report

Attention: Patch coverage is 93.29389% with 68 lines in your changes missing coverage. Please review.

Project coverage is 90.15%. Comparing base (1d47507) to head (68d7da4).
Report is 104 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
crates/matrix-sdk/src/event_cache/room/mod.rs	82.71%	37 Missing and 5 partials ⚠️
...s/matrix-sdk-common/src/linked_chunk/relational.rs	86.56%	8 Missing and 1 partial ⚠️
crates/matrix-sdk-sqlite/src/event_cache_store.rs	85.18%	0 Missing and 8 partials ⚠️
...atrix-sdk-common/src/linked_chunk/order_tracker.rs	99.07%	3 Missing and 1 partial ⚠️
crates/matrix-sdk-common/src/linked_chunk/mod.rs	92.59%	0 Missing and 2 partials ⚠️
crates/matrix-sdk/src/event_cache/room/events.rs	97.61%	0 Missing and 2 partials ⚠️
...rix-sdk-base/src/event_cache/store/memory_store.rs	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5225      +/-   ##
==========================================
+ Coverage   90.13%   90.15%   +0.02%     
==========================================
  Files         334      335       +1     
  Lines      104717   105687     +970     
  Branches   104717   105687     +970     
==========================================
+ Hits        94387    95286     +899     
- Misses       6277     6329      +52     
- Partials     4053     4072      +19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ferent accumulators In the next patch, we're going to introduce another user of `UpdatesToVectorDiff` which doesn't require accumulating the `VectorDiff` updates; so as to make it optional, let's generalize the algorithm with a trait, that carries the same semantics. No changes in functionality.

…gs are doing

…ordering of the current items This is a new data structure that will help figuring out a local, absolute ordering for events in the current linked chunk. It's designed to work even if the linked chunk is being lazily loaded, and it provides a few high-level primitives that make it possible to work nicely with the event cache.

…by the event cache The one hardship is that lazy-loading updates must NOT affect the order tracker, otherwise its internal state will be incorrect (disynchronized from the store) and thus return incorrect values upon shrink/lazy-load. In this specific case, some updates must be ignored, the same way we do it for the store using `let _ = store_updates().take()` in a few places. The author considered that a right place where to flush the pending updates was at the same time we flushed the updates-as-vector-diffs, since they would be observable at the same time.

…block

Hywan

Thanks for the code, it's clear and well explained.

I'm wondering how the event order will be used. I'm actually intrigued by the complexity induced by a lazy-loaded vs. a fully-loaded. I've a feeling that having a negative or positive positions would work and could simplify stuff. I'm not super comfortable with the idea of loading all the chunks (even without the events), it defeats the purpose of having such a light structure.

Are you loading all the chunks to always return the position of an event? It means you want the position of an event from the store, not from the in-memory room event cache? If this is the case, I think the strategy should entirely move onto the EventCacheStore store trait. If that's not the case, I don't understand why we need to load all the chunks. Can you explain to me please?

Hywan · 2025-06-25T08:38:44Z

crates/matrix-sdk-common/src/linked_chunk/as_vector.rs

+pub(super) trait UpdatesAccumulator<Item> {
+    /// Create a new accumulator with a rough estimation of the number of
+    /// updates this accumulator is going to receive.
+    fn new(num_updates_hint: usize) -> Self;
+
+    /// Fold the accumulator with the given new updates.
+    fn fold(&mut self, updates: impl IntoIterator<Item = VectorDiff<Item>>);
+}


Couldn't we use std::default::Default and std::iter::Extend here? It would avoid re-inventing the wheel probably. Just a personal taste.

I should roughly look like:

pub(super) trait UpdatesAccumulator<Item>: Default + Extend<Item> {}

(a trait might even not be necessary actually)

We loose the num_updates_hint though, don't know how impactful it is.

Hywan · 2025-06-25T08:42:18Z

crates/matrix-sdk-common/src/linked_chunk/as_vector.rs

+        Vec::with_capacity(num_updates_hint)
+    }
+
+    fn fold(&mut self, updates: impl IntoIterator<Item = VectorDiff<Item>>) {


If you prefer to not use Default and Extend, please rename fold to extend:

fold produces a single value by combining a previous value and a new one,

extend takes a mutable reference to a collection and pushes items at its end.

The meanings are too different.

Hywan · 2025-06-25T08:55:16Z

crates/matrix-sdk-common/src/linked_chunk/mod.rs

+    pub fn order_tracker(
+        &mut self,
+        all_chunks_iterator: Option<Iter<'_, CAP, Item, Gap>>,
+    ) -> Option<OrderTracker<Item, Gap>>


Idea 💡: If the order is defined as i64 (which can represent negative indexes), you may not need to get an overview of all the chunks or items, thus supporting lazy-loaded as fully-loaded seamlessly.

Hywan · 2025-06-25T08:56:05Z

crates/matrix-sdk-common/src/linked_chunk/order_tracker.rs

+use crate::linked_chunk::UpdateToVectorDiff;
+
+#[derive(Debug)]
+pub struct OrderTracker<Item, Gap> {


Can we have a documentation for this type please 🥹?

Hywan · 2025-06-25T08:58:09Z

crates/matrix-sdk-common/src/linked_chunk/order_tracker.rs

+impl<Item> super::UpdatesAccumulator<Item> for NullAccumulator<Item> {
+    fn new(_num_updates_hint: usize) -> Self {
+        Self { _phantom: std::marker::PhantomData }
+    }
+
+    fn fold(&mut self, _updates: impl IntoIterator<Item = eyeball_im::VectorDiff<Item>>) {
+        // Nothing to do, this is a no-op accumulator.
+    }
+}


If we use Default + Extend as suggested earlier, NullAccumulator can be () (because, yeah, Extend is implemented for ()).

Also, removing the need for the special trait UpdatesAccumulator, mechanically remove the need for the PhantomData (for the Item generic).

Hywan · 2025-06-25T09:13:02Z

crates/matrix-sdk-common/src/linked_chunk/updates.rs

@@ -294,6 +294,12 @@ impl<Item, Gap> UpdatesInner<Item, Gap> {
        slice
    }

+    /// Has the given reader, identified by its [`ReaderToken`], some pending
+    /// updates, or has it consumed all the pending updates?
+    pub(super) fn is_reader_up_to_date(&self, token: ReaderToken) -> bool {


Can we use debug_assertions here please?

#[cfg(debug_assertions)]

So that it exists only for non-release build. That's what debug_assert! uses internally.

Hywan · 2025-06-25T09:16:05Z

crates/matrix-sdk-common/src/linked_chunk/order_tracker.rs

+    /// If `inhibit` is `true`, the updates are ignored; otherwise, they are
+    /// consumed normally.


When do we need to use inhibit = true?

Hywan · 2025-06-25T09:21:11Z

crates/matrix-sdk/src/event_cache/room/events.rs

    ) -> Self {
        let mut linked_chunk = linked_chunk.unwrap_or_else(LinkedChunk::new_with_update_history);

        let chunks_updates_as_vectordiffs = linked_chunk
            .as_vector()
-            // SAFETY: The `LinkedChunk` has been built with `new_with_update_history`, so


Why removing the SAFETY comment?

Because the expect() says the same thing one line below, so there's no real value in repeating it in a code comment IMO.

Hywan · 2025-06-25T09:24:51Z

crates/matrix-sdk/src/event_cache/room/events.rs

@@ -231,6 +255,35 @@ impl RoomEvents {
        self.chunks.items()
    }

+    /// Return the order of an event in the room (main) linked chunk.


You can remove “(main)” I think:

it can only create confusion,

to me, it's clear that a room has a linked chunk, and that each thread will have its own linked chunk,

you are saying “in the room”, so we know it's not for the thread.

The comment is already crystal clear, you don't need “(main)”.

You're right! I suppose I've confused myself with respect to the "main conversation thread", but the room is so much more than that…

Hywan · 2025-06-25T09:41:51Z

crates/matrix-sdk/src/event_cache/room/mod.rs

+            // If loading the full linked chunk failed, we'll clear the event cache, as it
+            // indicates that at some point, there's some malformed data.
+            let fully_loaded_linked_chunk =
+                match Self::load_full_linked_chunk(&*store_lock, linked_chunk_id).await {


It seems we are loading all chunks with all their items. It seems to be quite annoying and quite a blocker to me. It will impact performance too much. At best we must only load the chunks without their items.

bnjbvr · 2025-06-25T10:44:09Z

Thanks for the review! These are very valid questions.

I'm wondering how the event order will be used.

The requirement is that we want to be able to compare the relative position of two events, be they loaded in the in-memory linked chunk or not. So, this must work, independently of the actual position of the event.

Some alternative ways to build this:

as you suggest, the most obvious optimization is to not load all chunks with their events, but only some "metadata" (prev/next/number of items in it).
we could also move the entire computation inside the store trait, and make it stateless; in this case, comparing the relative positions of two events would be equivalent to finding their owning chunks, i.e. find the relative ordering of the chunks (they belong to). But we have a linked list of chunks, here, so we can't immediately answer that query without going through the next/prev link of one of the two chunks. I suspect the optimal algorithm would be a "ping-pong" strategy: start with the next; if it's not this one, continue to the previous; if it's not this one, continue with the next's next; if if it's not this one, continue with the previous' previous, and so on. But worst case scenario, we now have to iterate over all the chunks every time we want to order two events.
- Could be mitigated by keeping a local cache for the ordering of chunks, that's blown up as soon as we add / remove a chunk, maybe… but at this point, we might as well keep the current solution, that constructs the full ordering, then maintains it over time.
maybe there's a way to have a mixed, incremental solution where we only get the initial chunk in the total ordering; every time we don't find the chunk owning an event in the local state, we'd lazily load the previous chunk in the order tracker (and maybe we could use negative indices as you suggest). Worst case, we'd get to the initial chunk (but then we'd have a fully-loaded order tracker, which will be faster every time it's called subsequently). Now, this might be tricky because we don't want to lazy-load the room's linked chunk just so as to maintain the order tracking up to date; but if the room's linked chunk does lazy-load, we want the order tracker to benefit from this. Grmpf.

It sounds like the first and last solutions might be preferable in the short term, and would both require loading only the metadata of a chunk. So I could start with this.

Then, I find the concern around performance totally legit. The best way to resolve it would be benchmarking or using some real-world measures, so I may look into this and get back to you here.

Are you loading all the chunks to always return the position of an event? It means you want the position of an event from the store, not from the in-memory room event cache? If this is the case, I think the strategy should entirely move onto the EventCacheStore store trait. If that's not the case, I don't understand why we need to load all the chunks. Can you explain to me please?

As said above, a solution only based on the EventCacheStore trait would likely be slow, as in the worst case you go through the entire prev/next chain of nodes in the linked list (in sqlite, that's probably one query per "deref").

Loading all the chunks at start seemed like a reasonable solution to build the initial state, and then maintain it cleanly over time. But yeah, at the very least we'd need to only load the minimal amount of data for the order tracker to work correctly.

… `OrderTracker`

bnjbvr · 2025-06-25T16:16:07Z

We've discussed this, and lazy loading the order tracker brings many other complications, so we're going to roll with the first suggestion only (load a limited set of metadata about each chunk of a linked chunk, and use that instead of the fully loaded linked chunk), and see what comes out of that in terms of performance. We can load the entire metadata in a single SQL query, so that ought to be rather efficient.

bnjbvr force-pushed the bnjbvr/linked-chunk-ordering branch 2 times, most recently from 2acde34 to fc99597 Compare June 18, 2025 18:06

bnjbvr marked this pull request as ready for review June 18, 2025 18:07

bnjbvr requested a review from a team as a code owner June 18, 2025 18:07

bnjbvr requested review from andybalaam and Hywan and removed request for a team and andybalaam June 18, 2025 18:07

bnjbvr marked this pull request as draft June 18, 2025 18:10

bnjbvr requested review from Hywan and removed request for Hywan June 18, 2025 18:10

bnjbvr marked this pull request as ready for review June 18, 2025 18:12

bnjbvr force-pushed the bnjbvr/linked-chunk-ordering branch from fc99597 to 7673e19 Compare June 19, 2025 08:26

bnjbvr added 5 commits June 19, 2025 15:51

doc(linked chunk): explain briefly what the reattaching/detaching fla…

802a2ec

…gs are doing

refactor(event cache): regroup lazy-loading methods in the same impl …

b218b10

…block

bnjbvr force-pushed the bnjbvr/linked-chunk-ordering branch from 7673e19 to b218b10 Compare June 19, 2025 13:52

bnjbvr mentioned this pull request Jun 19, 2025

feat(event cache): return relations in their local ordering #5258

Draft

poljar mentioned this pull request Jun 19, 2025

chore: Switch to latest rustfmt style edition #5255

Open

4 tasks

Hywan requested changes Jun 25, 2025

View reviewed changes

bnjbvr added 2 commits June 25, 2025 17:12

feat(event cache store): add a method to return the chunks' metadata

eb72e2b

refactor(event cache): use the linked chunk metadata to construct the…

68d7da4

… `OrderTracker`

		/// If `inhibit` is `true`, the updates are ignored; otherwise, they are
		/// consumed normally.

feat(event cache): introduce an absolute local event ordering #5225

Are you sure you want to change the base?

feat(event cache): introduce an absolute local event ordering #5225

Conversation

bnjbvr commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Hywan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnjbvr commented Jun 25, 2025

Uh oh!

bnjbvr commented Jun 25, 2025

Uh oh!

Uh oh!

bnjbvr commented Jun 12, 2025 •

edited

Loading

codecov bot commented Jun 12, 2025 •

edited

Loading