Skip to content

Added event cache #445

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

alanpoon
Copy link
Contributor

@alanpoon alanpoon commented Mar 28, 2025

Fixes #442, #435
Allows displaying of offline cached messages and room list.

Tested:

  • Starting Robrix in offline mode and reconnecting to internet. But the performance is bad, because all the rooms are synced together, causing the UI to freeze
  • UI to clear room cache

@alanpoon alanpoon added the waiting-on-review This issue is waiting to be reviewed label Mar 28, 2025
@alanpoon alanpoon self-assigned this Mar 28, 2025
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

What are the performance problems you're referring to? Can you elaborate on that?

Also, I'm confused, because I thought the SDK did not fully support saving/restoring the event cache to/from disk. I asked about this previously but I think you might have missed it. Last I heard, the SDK implementation of event caching was not complete; is it fully working yet?

Finally, Robrix currently assumes that there will not be any existing timeline content when syncing and displaying rooms. What happens now that we restore old timeline content to each room? At a minimum, we'll encounter the problem of needing to support forward pagination, because there will very likely be a gap between the old events from the event cache and the newest events fetched via sliding sync. We don't have a way to handle that yet.

.sqlite_store(client_session.db_path, Some(&client_session.passphrase))
.sliding_sync_version_builder(VersionBuilder::DiscoverNative)
Copy link
Member

Choose a reason for hiding this comment

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

we also need this, otherwise the client won't attempt to use sliding sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, Saved sliding_sync_version into persistent session. Added "client.set_sliding_sync_version" in restoring_session.

@kevinaboos kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Mar 31, 2025
@alanpoon
Copy link
Contributor Author

alanpoon commented Apr 1, 2025

What are the performance problems you're referring to? Can you elaborate on that?

Also, I'm confused, because I thought the SDK did not fully support saving/restoring the event cache to/from disk. I asked about this previously but I think you might have missed it. Last I heard, the SDK implementation of event caching was not complete; is it fully working yet?

Finally, Robrix currently assumes that there will not be any existing timeline content when syncing and displaying rooms. What happens now that we restore old timeline content to each room? At a minimum, we'll encounter the problem of needing to support forward pagination, because there will very likely be a gap between the old events from the event cache and the newest events fetched via sliding sync. We don't have a way to handle that yet.

Added Forward pagination after internet reconnection.

@alanpoon alanpoon added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Apr 1, 2025
@kevinaboos
Copy link
Member

Finally, Robrix currently assumes that there will not be any existing timeline content when syncing and displaying rooms. What happens now that we restore old timeline content to each room? At a minimum, we'll encounter the problem of needing to support forward pagination, because there will very likely be a gap between the old events from the event cache and the newest events fetched via sliding sync. We don't have a way to handle that yet.

Added Forward pagination after internet reconnection.

I appreciate that, but .... it's not that simple. First, the code you added will unconditionally run forward pagination on all rooms in the entire client. We never want to do that, as a user might have thousands of rooms, the vast majority of which will not be opened by them. Doing something like that would be incredibly wasteful of CPU, memory, disk, and network resources on both the client and server.

Second, and most importantly, we need significant infrastructure across multiple parts of Robrix in order to be able to handle forward pagination correctly. All of the pagination-related code we have thus far assumes only backwards pagination, so running a forward pagination request will not work as well as you expect. For example, we don't efficiently handle the case of merging in new items from a recent forward-pagination request into the existing timeline. We also don't have a way to represent that a timeline may have a gap of missing events at any point within it, rather than just before it's current starting point. There are many other cases that we also have to consider beyond these.

This requires serious modifications/improvements to Robrix's code. I would suggest that we pause work on enabling the event cache until those tasks are complete.

@kevinaboos kevinaboos added blocked Blocked on another issue or missing feature waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Apr 3, 2025
@kevinaboos kevinaboos removed the blocked Blocked on another issue or missing feature label May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author This issue is waiting on the original author for a response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants