Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Don't pull out the full state of the room when handling new events. #12684

Closed
3 tasks
erikjohnston opened this issue May 10, 2022 · 4 comments
Closed
3 tasks

Don't pull out the full state of the room when handling new events. #12684

erikjohnston opened this issue May 10, 2022 · 4 comments
Assignees
@erikjohnston
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Milestone
Server Density

Comments

@erikjohnston
Copy link
Member

Currently when we calculate the state group for a new event we always fetch the full state from the DB, even when no state resolution happens. This is then stored in the EventContext, which is access via context.get_prev_state_ids() and get_current_state_ids().

Historically, we needed to store the state in EventContext as we may not have persisted it yet. However, nowadays we always store the state group for the new event before we create EventContext, so we can rely on being able to fetch the state from the DB.

I suggest we:

  • Remove the cached state from EventContext and instead fetch state from the DB each time (relying on the caching to minimise performance problems)
  • Ensure the state handler doesn't fetch the full state from the DB when there are no conflicts
  • Change the EventContext methods to accept a StateFilter and use those everywhere.
@erikjohnston erikjohnston added the T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. label May 10, 2022
@erikjohnston erikjohnston self-assigned this May 10, 2022
@erikjohnston erikjohnston mentioned this issue May 10, 2022
Merged
@erikjohnston
Copy link
Member Author

erikjohnston commented May 10, 2022 •
edited by H-Shay
Loading

#12689 refactors the EventContext so that it doesn't cache the full state.

Next steps:

@erikjohnston erikjohnston removed their assignment May 10, 2022
erikjohnston added a commit that referenced this issue May 10, 2022
@erikjohnston
Refactor EventContext (#12689)
c72d26c 
Refactor how the `EventContext` class works, with the intention of reducing the amount of state we fetch from the DB during event processing.

The idea here is to get rid of the cached `current_state_ids` and `prev_state_ids` that live in the `EventContext`, and instead defer straight to the database (and its caching). 

One change that may have a noticeable effect is that we now no longer prefill the `get_current_state_ids` cache on a state change. However, that query is relatively light, since its just a case of reading a table from the DB (unlike fetching state at an event which is more heavyweight). For deployments with workers this cache isn't even used.


Part of #12684
@H-Shay H-Shay mentioned this issue May 18, 2022
Merged
@erikjohnston erikjohnston assigned erikjohnston and unassigned H-Shay Jun 6, 2022
@erikjohnston
Copy link
Member Author

All of the work other than #12653 has been done, so closing in favour of #12653

1 similar comment
@erikjohnston
Copy link
Member Author

All of the work other than #12653 has been done, so closing in favour of #12653

@erikjohnston
Copy link
Member Author

All of the work other than #12653 has been done, so closing in favour of #12653

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@erikjohnston erikjohnston

Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Milestone
Server Density
Development

No branches or pull requests
2 participants
@erikjohnston @H-Shay