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

Avoid re-calculating bundled aggregations for cached results #11585

Closed
clokep opened this issue Dec 15, 2021 · 3 comments · Fixed by #11612 or #11659
Closed

Avoid re-calculating bundled aggregations for cached results #11585

clokep opened this issue Dec 15, 2021 · 3 comments · Fixed by #11612 or #11659
Assignees
Labels
A-Threads Threaded messages T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Comments

@clokep
Copy link
Member

clokep commented Dec 15, 2021

Related to #11583 -- currently when we generate a sync response we cache it for a period of time (as clients sometimes timeout and retry). Unfortunately the bundled aggregations are added during the serialization step, which occurs after caching, and is re-done even when using a cached response.

Ideally we should be calculating the bundled aggregations earlier in a different manner and the serialization methods should be synchronous.

See https://matrix.to/#/!XaqDhxuTIlvldquJaV:matrix.org/$1E_K64Dd5p6iFMS4agak62bn5JZl3Ry-6XiTXYzOmhU?via=matrix.org&via=vector.modular.im&via=envs.net for a brief conversation around this.

@clokep clokep added T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. A-Threads Threaded messages labels Dec 15, 2021
@clokep clokep self-assigned this Dec 15, 2021
@clokep
Copy link
Member Author

clokep commented Dec 15, 2021

It might make more sense to:

  • Explicitly calculate the bundled aggregations at some point by calling a method on a list of events.
  • The bundled aggregations could be store on _EventInternalMetadata (similar to how redacted is handled).
  • The event serialization code would then have everything necessary to go from EventBase -> JSON.

It would also be slightly less magical, but might be a bit more verbose.

@clokep
Copy link
Member Author

clokep commented Dec 15, 2021

My hope with the above is that we can avoid this problem for other APIs too, if it exists. Note that we need to be careful though since events get cached and modifying them will modify a shared copy.

@clokep
Copy link
Member Author

clokep commented Jan 7, 2022

Oops, this isn't "done" until #11659 lands.

@clokep clokep reopened this Jan 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Threads Threaded messages T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
1 participant