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(base) Implement RelationalLinkedChunk #4298

Merged
merged 9 commits into from
Nov 25, 2024

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Nov 20, 2024

A flask of poison

A book floating in space in front of a black hole.


A RelationalLinkedChunk is like a LinkedChunk but with a relational
layout, similar to what we would have in a database.

This is used by memory stores. The idea is to have a data layout that
is similar for memory stores and for relational database stores, to
represent a LinkedChunk.

This type is also designed to receive Update. Applying Updates
directly on a LinkedChunk is not ideal and particularly not trivial
as the Updates do not match the internal data layout of the
LinkedChunk, they have been designed for storages, like a relational
database for example.

This type is not as performant as LinkedChunk (in terms of memory
layout, CPU caches etc.). It is only designed to be used in memory
stores, which are mostly used for test purposes or light usages of the
SDK.


@Hywan Hywan mentioned this pull request Nov 20, 2024
37 tasks
@Hywan Hywan changed the title feat: Implement event cache persistent storage (unplugged) feat(base) Implement RelationalLinkedChunk Nov 20, 2024
@Hywan Hywan force-pushed the feat-event-cache-persistent-storage branch 5 times, most recently from 0a23400 to 0c9f681 Compare November 20, 2024 15:27
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 85.08772% with 17 lines in your changes missing coverage. Please review.

Project coverage is 85.11%. Comparing base (e5ca44b) to head (c788221).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
...s/matrix-sdk-common/src/linked_chunk/relational.rs 87.35% 11 Missing ⚠️
...rix-sdk-base/src/event_cache/store/memory_store.rs 83.33% 3 Missing ⚠️
crates/matrix-sdk-common/src/linked_chunk/mod.rs 66.66% 2 Missing ⚠️
...es/matrix-sdk-base/src/event_cache/store/traits.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4298      +/-   ##
==========================================
+ Coverage   85.09%   85.11%   +0.01%     
==========================================
  Files         274      275       +1     
  Lines       30183    30280      +97     
==========================================
+ Hits        25684    25772      +88     
- Misses       4499     4508       +9     

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


🚨 Try these New Features:

A `RelationalLinkedChunk` is like a `LinkedChunk` but with a relational
layout, similar to what we would have in a database.

This is used by memory stores. The idea is to have a data layout that
is similar for memory stores and for relational database stores, to
represent a `LinkedChunk`.

This type is also designed to receive `Update`. Applying `Update`s
directly on a `LinkedChunk` is not ideal and particularly not trivial
as the `Update`s do _not_ match the internal data layout of the
`LinkedChunk`, they have been designed for storages, like a relational
database for example.

This type is not as performant as `LinkedChunk` (in terms of memory
layout, CPU caches etc.). It is only designed to be used in memory
stores, which are mostly used for test purposes or light usages of the
SDK.
@Hywan Hywan force-pushed the feat-event-cache-persistent-storage branch from 0c9f681 to f063296 Compare November 20, 2024 15:49
@Hywan Hywan marked this pull request as ready for review November 20, 2024 15:52
@Hywan Hywan requested a review from a team as a code owner November 20, 2024 15:52
@Hywan Hywan requested review from stefanceriu and removed request for a team November 20, 2024 15:52
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

This code is likely wrong, given that the store method should include a reference to a RoomId (sorry, should've caught that during review of the patch adding the trait method, but noticed the problem only while doing the implementation for the SQLite backend…).

@stefanceriu stefanceriu removed their request for review November 22, 2024 06:41
@Hywan Hywan requested a review from bnjbvr November 25, 2024 09:20
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

lgtm! minor comments so approving to avoid back-and-forth, and I've included some ideas for bigger improvements (a single inner lock for the memory store; tweak the signature of handle_linked_chunk_updates) that can be done here or as follow-up PRs. Thanks!

crates/matrix-sdk-common/src/linked_chunk/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-common/src/linked_chunk/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-common/src/linked_chunk/relational.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-common/src/linked_chunk/relational.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-common/src/linked_chunk/relational.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-common/src/linked_chunk/relational.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-common/src/linked_chunk/relational.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-common/src/linked_chunk/relational.rs Outdated Show resolved Hide resolved
@Hywan Hywan requested a review from a team as a code owner November 25, 2024 15:41
@Hywan Hywan requested review from BillCarsonFr and removed request for a team November 25, 2024 15:41
This patch adds constructors for `Position` and `ChunkIdentifier` so
that we keep their inner values private.
This patch creates a new `MemoryStoreInner` and moves all fields from
`MemoryStore` into this new type. All locks are removed, but a new lock
is added around `MemoryStoreInner`. That way we have a single lock.
@Hywan Hywan force-pushed the feat-event-cache-persistent-storage branch from a455751 to 0a6b093 Compare November 25, 2024 15:41
@Hywan Hywan removed the request for review from BillCarsonFr November 25, 2024 15:41
@Hywan Hywan enabled auto-merge (rebase) November 25, 2024 15:42
…<Update>`.

This patch updates `EventCacheStore::handle_linked_chunk_updates` to
take a `Vec<Update<Item, Gap>>` instead of `&[Update<Item, Gap>]`.
In fact, `linked_chunk::ObservableUpdates::take()` already returns a
`Vec<Update<Item, Gap>>`; we can simply forward this `Vec` up to here
without any further clones.
@Hywan Hywan force-pushed the feat-event-cache-persistent-storage branch from 0a6b093 to c788221 Compare November 25, 2024 15:53
@Hywan Hywan disabled auto-merge November 25, 2024 16:08
@Hywan Hywan merged commit b979b2e into matrix-org:main Nov 25, 2024
38 checks passed
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