Skip to content

feat(event cache): connect the room event cache and persistent storage#4347

Merged
bnjbvr merged 4 commits intomainfrom
bnjbvr/write-into-event-cache
Dec 2, 2024
Merged

feat(event cache): connect the room event cache and persistent storage#4347
bnjbvr merged 4 commits intomainfrom
bnjbvr/write-into-event-cache

Conversation

@bnjbvr
Copy link
Copy Markdown
Contributor

@bnjbvr bnjbvr commented Nov 28, 2024

This implements both directions for interacting between the event cache and the storage backend. I think this is fine design… as long as we don't have any other process writing into the cache, in which case we'll need to reset at different places.

writing

The main idea in this PR is to use RoomEventCacheState::with_events_mut(func: impl FnMut(&mut RoomEvents)) to perform any write to the underlying RoomEvent data structure. The function takes care of maintaining the storage, after updates have been performed.

Additionally: this behavior isn't enabled by default; callers have to call EventCache::enable_storage() when they start using the event cache.

I've put the RoomEventCacheState data structure in a private module, so that even the same module can't access its private fields, and we don't make mistakes by accidentally misusing events.

reading

Reading happens by reloading the chunks initially, and prefilling the deduplicator with the reloaded events information. It's mostly boring changes at this point.


Part of #3280. On top of #4362. Split and reworked from #4308.

@bnjbvr bnjbvr requested a review from a team as a code owner November 28, 2024 11:55
@bnjbvr bnjbvr requested review from Hywan and stefanceriu and removed request for a team and stefanceriu November 28, 2024 11:55
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 88.17204% with 11 lines in your changes missing coverage. Please review.

Project coverage is 85.15%. Comparing base (5da36d1) to head (fca4131).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/event_cache/deduplicator.rs 63.63% 4 Missing ⚠️
crates/matrix-sdk/src/event_cache/pagination.rs 82.60% 4 Missing ⚠️
crates/matrix-sdk/src/event_cache/mod.rs 78.57% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4347      +/-   ##
==========================================
+ Coverage   85.13%   85.15%   +0.02%     
==========================================
  Files         280      280              
  Lines       30666    30716      +50     
==========================================
+ Hits        26107    26157      +50     
  Misses       4559     4559              

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

@bnjbvr bnjbvr marked this pull request as draft December 2, 2024 09:29
@bnjbvr
Copy link
Copy Markdown
Contributor Author

bnjbvr commented Dec 2, 2024

Getting back to a draft for this; I suspect a bit more work is required because the back-pagination tokens ought to belong in this state too…

@bnjbvr bnjbvr force-pushed the bnjbvr/write-into-event-cache branch 2 times, most recently from c9eb2db to d6afbcc Compare December 2, 2024 13:28
@bnjbvr bnjbvr marked this pull request as ready for review December 2, 2024 13:28
@bnjbvr
Copy link
Copy Markdown
Contributor Author

bnjbvr commented Dec 2, 2024

Calling this ready for review, after a rebase from main. I think the backpagination issue I've thought about, should be resolved independently.

@Hywan Whenever the time is right for you, PTAL ☺️

@bnjbvr bnjbvr changed the title feat(event cache): write updates from the event cache to storage feat(event cache): read and write chunks from the event cache to storage Dec 2, 2024
@bnjbvr bnjbvr changed the title feat(event cache): read and write chunks from the event cache to storage feat(event cache): connect the room event cache and persistent storage Dec 2, 2024
Copy link
Copy Markdown
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good. I like the new API to mutate the state. Thanks for putting everything together.

Comment thread crates/matrix-sdk/src/event_cache/room/events.rs Outdated
Comment thread crates/matrix-sdk/src/event_cache/room/mod.rs Outdated
Comment thread crates/matrix-sdk/src/event_cache/deduplicator.rs
Comment thread crates/matrix-sdk/src/event_cache/room/events.rs
Comment thread crates/matrix-sdk/src/event_cache/room/mod.rs
.await
.unwrap();

// The stream doesn't report these changes *yet*. Use the items vector given
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's part of the PR I'm preparing I suppose, with VectorDiff being emitted by EventCache?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. With your PR, we'd get a remove(0) and a push_back (or insert(1) ^^). Right now, these are not sent via the updates, and that's why we can't use this by default yet 👀

@bnjbvr bnjbvr force-pushed the bnjbvr/write-into-event-cache branch from d6afbcc to fca4131 Compare December 2, 2024 14:18
@bnjbvr bnjbvr enabled auto-merge (rebase) December 2, 2024 14:18
@bnjbvr bnjbvr merged commit 2f9866c into main Dec 2, 2024
@bnjbvr bnjbvr deleted the bnjbvr/write-into-event-cache branch December 2, 2024 15:27
@bnjbvr bnjbvr mentioned this pull request Dec 3, 2024
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants