Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Dec 2, 2024

Conversation

bnjbvr
Copy link
Member

@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 stefanceriu and Hywan and removed request for a team and stefanceriu November 28, 2024 11:55
Copy link

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
Member 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
Member 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
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.

crates/matrix-sdk/src/event_cache/room/events.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/room/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/room/mod.rs Show resolved Hide resolved
.await
.unwrap();

// The stream doesn't report these changes *yet*. Use the items vector given
Copy link
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
Member 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
41 checks passed
@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