Skip to content

Conversation

@mgoldenberg
Copy link
Contributor

Background

This pull request provides an initial implementation of the EventCacheStore and is mostly taken from the work completed in #4617, with a few additions and modifications to accommodate more recent updates to the trait.

Given that #4617 was a rather large pull request, this is part of a series of pull requests which will break that work into smaller, more manageable changes (see #4996). Notably, to minimize changes, this pull request omits the implementation of EventCacheStoreMedia and simply uses a nested MemoryStore for media queries for the time being.

Changes

This change is focused on integrating the implementation of EventCacheStore from #4617, while using a nested MemoryStore for media queries. Additionally, since the original implementation was written before the latest version of the EventCacheStore, I have added implementations of new functions.

There are virtually no changes to existing code, except for the addition of a new feature - event-cache-store - to the matrix-sdk-indexeddb crate. Most other changes occur in entirely new files.

Caveats

Given that the original implementation from #4617 was written before some of the more recent additions to the EventCacheStore trait, I had to retrofit implementations for a number of functions which are rather inefficient. I have left them inefficient, however, in order to keep this pull request small, as the changes are a bit involved.

I plan to improve them in a follow-up pull request.

Finding the Last Chunk

Loading the last chunk in a list - i.e., EventCacheStore::load_last_chunk - currently reads in and deserializes all chunks in a room and looks for any that have no next value. This is a rather inefficient process and could be greatly improved by adding another index on the LINKED_CHUNKS object store.

Querying Events by ID

The following functions read and deserialize all events in a room in order to perform their operations.

  • EventCacheStore::filter_duplicated_events
  • EventCacheStore::find_event
  • EventCacheStore::find_event_relations
  • EventCacheStore::save_event

These operations could also be greatly improved by the addition of a few new indices on the EVENTS object store.

Future Work

  • Add an index to the LINKED_CHUNKS object store in order to improve chunk-related queries.
  • Add indices to the EVENTS object store in order to improve event-related queries.
  • Add an EventCacheStoreMedia implementation in order to have persistent media storage.

  • Public API changes documented in changelogs (optional)

Signed-off-by: Michael Goldenberg [email protected]

mgoldenberg and others added 29 commits May 23, 2025 14:43
…ogic

Signed-off-by: Michael Goldenberg <[email protected]>
Co-authored-by: Oscar Franco <[email protected]>
Signed-off-by: Michael Goldenberg <[email protected]>
Co-authored-by: Oscar Franco <[email protected]>
…s in event cache store backend

Signed-off-by: Michael Goldenberg <[email protected]>
Co-authored-by: Oscar Franco <[email protected]>
…ized values have an `id` field

Signed-off-by: Michael Goldenberg <[email protected]>
Co-authored-by: Oscar Franco <[email protected]>
…_chunk_updates

Signed-off-by: Michael Goldenberg <[email protected]>
Co-authored-by: Oscar Franco <[email protected]>
…m_chunks

Signed-off-by: Michael Goldenberg <[email protected]>
Co-authored-by: Oscar Franco <[email protected]>
…ed_lock

Signed-off-by: Michael Goldenberg <[email protected]>
Co-authored-by: Oscar Franco <[email protected]>
…heStore::find_event

Signed-off-by: Michael Goldenberg <[email protected]>
@mgoldenberg mgoldenberg requested a review from a team as a code owner May 23, 2025 20:15
@mgoldenberg mgoldenberg marked this pull request as ready for review May 24, 2025 04:27
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.

Oh wow, it looks great! Thanks for the hardwork.

First off, is it possible to split this PR in smaller ones? It would radically simplify the review process, both for you and for me? Feel free to assign me for all the PR.

@mgoldenberg
Copy link
Contributor Author

First off, is it possible to split this PR in smaller ones?

I could certainly try, but it’d be helpful to hear any suggestions as to what would make things easier for you! For context, it was hard to keep things small as the trait is quite large 😬 but if you’re okay with partial implementations of the trait, failing tests, etc I could probably split it up further to add one function at a time.

Let me know what you think!

@mgoldenberg
Copy link
Contributor Author

Also, I did my best to add one function per commit, so that may be useful to you in review?

@Hywan
Copy link
Member

Hywan commented May 27, 2025

I think we can split the PR as follows:

  • creating the database, trait has unimplemented!()
  • one PR per method in the trait, removing unimplemented!() one PR at a time, adding tests gradually for each new method
  • one PR for the final bits (I don't know which ones exactly)

What do you think?

@mgoldenberg
Copy link
Contributor Author

@Hywan, I think that would mostly work okay, except that the integration tests cannot be added incrementally as far as I can tell. I suppose I could copy relevant tests over one-by-one, and then when we have all the functions implemented, I could remove the copies and make a call to the macro.

Does that sound reasonable? Or is there a better way of going about that?

@mgoldenberg
Copy link
Contributor Author

I might also be able to delegate to a nested MemoryStore instead of using unimplemented!(), that way the integration tests all pass right away. But, that might get tricky as we modify individual functions.

@mgoldenberg
Copy link
Contributor Author

mgoldenberg commented May 30, 2025

@Hywan, in keeping with your suggestions re: splitting the work into smaller pieces, I have created a pull request that deals solely with the database types and migrations (see #5138). That should hopefully be a good start!

@Hywan
Copy link
Member

Hywan commented Jun 2, 2025

Does that sound reasonable? Or is there a better way of going about that?

It sounds great to me!

@mgoldenberg
Copy link
Contributor Author

Closing this, as work is continuing piecemeal in #5138.

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