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

OF-2530: Optimize MUC Message History cache usage #2134

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

guusdk
Copy link
Member

@guusdk guusdk commented Oct 19, 2022

When reading or writing cached MUC messages, the entire collection of messages is serialized between cluster nodes. This adds an unacceptable amount of overhead, for every message that is added.

In this commit, the singular cached entity (a list of messages), is separated into two parts:

  • A cached entity that represents a list of message references per chatroom
  • messages, by reference

The purpose of this is to optimize the scenario of adding a message (which is expected to happen more frequently than reading the history). By having to update only a list of references, instead of a list of actual objects, the amount of data that is to be operated on is reduced significantly.

When reading or writing cached MUC messages, the entire collection of messages is serialized between cluster nodes. This adds an unacceptable amount of overhead, for every message that is added.

In this commit, the singular cached entity (a list of messages), is separated into two parts:
- A cached entity that represents a list of message references per chatroom
- messages, by reference

The purpose of this is to optimize the scenario of  adding a message (which is expected to happen more frequently than reading the history). By having to update only a list of references, instead of a list of actual objects, the amount of data that is to be operated on is reduced significantly.
@guusdk guusdk added this to the 4.7.4 milestone Oct 19, 2022
@guusdk
Copy link
Member Author

guusdk commented Oct 19, 2022

I have already applied this change to the 4.7 branch, which helped me generate a distributable artifact that I needed for testing. If this PR is to be changed or rejected, the same needs to be applied to the commit already in the 4.7 branch.

@guusdk guusdk modified the milestones: 4.7.4, 4.8.0 Oct 21, 2022
@guusdk
Copy link
Member Author

guusdk commented Oct 21, 2022

I have reverted this change on the 4.7 branch. The issue at hand was resolved by changing the 'message history' from 'all' to a finite amount. With this change in place, an unexpected (possibly unrelated) message delivery delay got introduced.

As this change also introduces a second cache, which depends on faultless operation to avoid leaking orphaned entries (I failed to include a 'cleanup' routine), it might be best to keep this out of the 4.7 branch, and re-evaluate it for 4.8.

@akrherz
Copy link
Member

akrherz commented Jun 27, 2023

@guusdk What's the status of this PR?

@guusdk
Copy link
Member Author

guusdk commented Jun 27, 2023

I've not looked at it since my last comment. It's something best picked up when there's renewed activity around MUC changes.

@Fishbowler
Copy link
Member

Kick back to draft?

@guusdk guusdk marked this pull request as draft November 15, 2023 13:25
@akrherz akrherz removed this from the 4.8.0 milestone Nov 16, 2023
@akrherz
Copy link
Member

akrherz commented Nov 16, 2023

Removed from 4.8.0 milestone as beta is imminent and this would need to be in first.

@Fishbowler
Copy link
Member

Is this compatible with the new MUC changes? At a guess, yes. But we'd need to solve those outstanding problems. Feels like a good change for vNext.

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.

3 participants