refactor: `RoomEvents::reset` really clears the linked chunk #4321

Hywan · 2024-11-25T16:32:51Z

This PR is twofold:

It fixes a bug in AsVector: when an Update::Clear was received, the internal state of AsVector wasn't cleared,
It updates RoomEvents::reset to use LinkedChunk::clear so that events are correctly cleared and not dropped silently, i.e. the Updates and VectorDiffs are correctly reflecting that. That's important to clear the stores (implementations of EventCacheStore, like MemoryStore or SqliteStore). A new RoomEvents::updates_as_vector_diffs() method is added, it's going to be used later and it's useful to test the new behaviour now.

Address [meta] EventCache storage #3280
Partly address linked chunk: the initial chunk isn't forwarded as part of the updates #4314

bnjbvr · 2024-11-26T09:52:18Z

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

+                    // An `Update::Clear` happens when `LinkedChunk::clear` is called, which removes
+                    // all chunks, and re-adds one `Chunk` with `FIRST_IDENTIFIER` as its chunk
+                    // identifier.
+                    self.chunks.push_back((ChunkIdentifierGenerator::FIRST_IDENTIFIER, 0));


I think it's the wrong layer to do that, the linked chunk should save that update and propagate it instead; see also #4314 which describes the same issue at construction.

Oh, sure! Good idea. I'll update this PR to address that.

Okay, this PR has the correct approach now, but another PR is necessary to fully fix #4314. I'm already on it, but I think it's nice to keep them separate, otherwise the diff will be a bit confusing.

bnjbvr · 2024-11-26T09:55:12Z

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

+    /// [`Update`]: matrix_sdk_base::linked_chunk::Update
+    #[allow(unused)] // gonna be useful very soon! but we need it now for test purposes
+    pub fn updates_as_vector_diffs(&mut self) -> Vec<VectorDiff<Event>> {
+        self.chunks_updates_as_vectordiffs.take()


Why do we have to store a chunks_updates_as_vectordiffs field, and why is not sufficient to expose the underlying method? Now that the underlying linked chunk is properly reset and not re-created, the stream won't close after a reset, right?

This is not a Stream :-). If we recreate the AsVector everytime, it will lose all its states, and the VectorDiff would be pretty messy.

Would we have to re-create an AsVector? I'm not sure about that.

Imagine we had:

pub fn as_vector(&self) -> ... { self.chunks.as_vector() }

The resulting AsVector could live as long as the caller wants, and it would always be sync'd with the underlying linked chunk, as long as it lived. In that case, we wouldn't have to store anything new in the linked chunk. Is there any reason we cannot do this?

codecov · 2024-11-26T10:03:05Z

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.06%. Comparing base (c61f707) to head (149ca7d).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-common/src/linked_chunk/mod.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4321      +/-   ##
==========================================
- Coverage   85.07%   85.06%   -0.01%     
==========================================
  Files         275      275              
  Lines       30314    30325      +11     
==========================================
+ Hits        25789    25797       +8     
- Misses       4525     4528       +3

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

This patch fixes a bug in `AsVector`: when an `Update::Clear` value is received, `AsVector`'s internal state must be cleared too, i.e. the `UpdateToVectorDiff::chunks` field should be reset to an initial value! This patch adds a test to ensure this works as expected.

This patch updates `RoomEvents::reset` to not drop the `LinkedChunk` to clear it.

bnjbvr

Still an open question before I approve, but this looks like this is on the right track!

bnjbvr · 2024-11-26T10:49:32Z

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

+    /// All events, all gaps, everything is dropped, move into the void, into
+    /// the ether, forever.


Having a blast, are you? :D

You have no idea!

bnjbvr · 2024-11-26T10:49:42Z

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

+            // SAFETY: The `LinkedChunk` has been built with `new_with_update_history`, so
+            // `as_vector` must return `Some(…)`.
+            .expect("`LinkedChunk` must have been constructor with `new_with_update_history`");


To avoid this expect, would it make sense that new_with_update_history() return the AsVector data structure too?

Hmm, new_with_update_history() could return (LinkedChunk, Update), but I don't see why it should return an AsVector directly. Problem is: to compute an AsVector, it needs an &mut Update, so the constructor would have to return (LinkedChunk, &mut Update), where the &mut is a mutable reference to LinkedChunk itself. Er, that's not possible.

Ok, too bad!

bnjbvr · 2024-11-26T10:51:54Z

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

+    /// [`Update`]: matrix_sdk_base::linked_chunk::Update
+    #[allow(unused)] // gonna be useful very soon! but we need it now for test purposes
+    pub fn updates_as_vector_diffs(&mut self) -> Vec<VectorDiff<Event>> {
+        self.chunks_updates_as_vectordiffs.take()


Would we have to re-create an AsVector? I'm not sure about that.

Imagine we had:

pub fn as_vector(&self) -> ... { self.chunks.as_vector() }

The resulting AsVector could live as long as the caller wants, and it would always be sync'd with the underlying linked chunk, as long as it lived. In that case, we wouldn't have to store anything new in the linked chunk. Is there any reason we cannot do this?

Hywan · 2024-11-26T13:54:23Z

Would we have to re-create an AsVector?

First off, we create AsVector right after LinkedChunk because AsVector drains all its updates when constructed. What it means is: updates that are past the creation of AsVector are ignored. And that's mandatory because AsVector computes its initial states by looking at all chunks, that's much more efficient. AsVector will map Updates to VectorDiffs starting from the state of the LinkedChunk at the creation time of AsVector.

If we re-create an AsVector every time, I mean, it's possible and it would be correct, but that's clearly a waste of time and resources. AsVector would have to re-calculate its initial states by looping over all chunks. That's not huge, but still.

Also, this AsVector will be used by RoomEvents to broadcast the VectorDiff to the Timeline via RoomEventCacheInner. So the caller is technically RoomEventCacherInner, which holds a RoomEvents, but I think it's much simpler for RoomEvents to hold AsVector rather than RoomEventCacheInner to fetch a new AsVector every time a RoomEventCacheUpdate must be sent.
Either way, AsVector has to be stored somewhere: either by RoomEventCacheInner or by RoomEvents. I thought it was easier on RoomEvents because it's more confined and easier to test (one less layer).

I've always seen RoomEvents as a bridge between EventCache and LinkedChunk. Within this frame, it makes sense to have AsVector inside RoomEvents. RoomEventCacheInner and RoomEvents have the same lifetime since the former holds the latter.

bnjbvr

Also, this AsVector will be used by RoomEvents to broadcast the VectorDiff to the Timeline via RoomEventCacheInner. So the caller is technically RoomEventCacherInner, which holds a RoomEvents, but I think it's much simpler for RoomEvents to hold AsVector rather than RoomEventCacheInner to fetch a new AsVector every time a RoomEventCacheUpdate must be sent.
Either way, AsVector has to be stored somewhere: either by RoomEventCacheInner or by RoomEvents. I thought it was easier on RoomEvents because it's more confined and easier to test (one less layer).

Thanks, I'm convinced by this 👍

bnjbvr · 2024-11-26T14:12:14Z

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

+            // SAFETY: The `LinkedChunk` has been built with `new_with_update_history`, so
+            // `as_vector` must return `Some(…)`.
+            .expect("`LinkedChunk` must have been constructor with `new_with_update_history`");


Ok, too bad!

Hywan changed the title ~~efactor: RoomEvents::reset really clear the linked chunk~~ refactor: RoomEvents::reset really clear the linked chunk Nov 25, 2024

Hywan changed the title ~~refactor: RoomEvents::reset really clear the linked chunk~~ refactor: RoomEvents::reset really clear the linked chunk Nov 25, 2024

Hywan changed the title ~~refactor: RoomEvents::reset really clear the linked chunk~~ refactor: RoomEvents::reset really clears the linked chunk Nov 25, 2024

Hywan force-pushed the feat-sdk-event-cache-room-events-clear branch 2 times, most recently from 20b61fe to 64e8114 Compare November 26, 2024 09:42

Hywan marked this pull request as ready for review November 26, 2024 09:45

Hywan requested a review from a team as a code owner November 26, 2024 09:45

Hywan requested review from poljar, stefanceriu and a team and removed request for a team and poljar November 26, 2024 09:45

Hywan force-pushed the feat-sdk-event-cache-room-events-clear branch from 64e8114 to 19a86e9 Compare November 26, 2024 09:48

bnjbvr reviewed Nov 26, 2024

View reviewed changes

Hywan force-pushed the feat-sdk-event-cache-room-events-clear branch from 19a86e9 to c6e6705 Compare November 26, 2024 10:06

Hywan added 2 commits November 26, 2024 11:18

refactor: RoomEvents::reset really clear the linked chunk.

149ca7d

This patch updates `RoomEvents::reset` to not drop the `LinkedChunk` to clear it.

Hywan force-pushed the feat-sdk-event-cache-room-events-clear branch from c6e6705 to 149ca7d Compare November 26, 2024 10:25

Hywan requested a review from bnjbvr November 26, 2024 10:27

bnjbvr reviewed Nov 26, 2024

View reviewed changes

Hywan requested a review from bnjbvr November 26, 2024 14:11

bnjbvr approved these changes Nov 26, 2024

View reviewed changes

Hywan merged commit cc8bc05 into matrix-org:main Nov 26, 2024
40 checks passed

Hywan mentioned this pull request Nov 28, 2024

[meta] EventCache storage #3280

Open

37 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: `RoomEvents::reset` really clears the linked chunk #4321

refactor: `RoomEvents::reset` really clears the linked chunk #4321

Hywan commented Nov 25, 2024 •

edited

Loading

bnjbvr Nov 26, 2024

Hywan Nov 26, 2024

Hywan Nov 26, 2024

bnjbvr Nov 26, 2024

Hywan Nov 26, 2024

bnjbvr Nov 26, 2024

codecov bot commented Nov 26, 2024 •

edited

Loading

bnjbvr left a comment

bnjbvr Nov 26, 2024

Hywan Nov 26, 2024

bnjbvr Nov 26, 2024

Hywan Nov 26, 2024

bnjbvr Nov 26, 2024

bnjbvr Nov 26, 2024

Hywan commented Nov 26, 2024

bnjbvr left a comment

bnjbvr Nov 26, 2024

		/// All events, all gaps, everything is dropped, move into the void, into
		/// the ether, forever.

refactor: RoomEvents::reset really clears the linked chunk #4321

refactor: RoomEvents::reset really clears the linked chunk #4321

Conversation

Hywan commented Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hywan commented Nov 26, 2024

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor: `RoomEvents::reset` really clears the linked chunk #4321

refactor: `RoomEvents::reset` really clears the linked chunk #4321

Hywan commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading