Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Message cache #595

Merged
merged 12 commits into from
May 26, 2024
Merged

Message cache #595

merged 12 commits into from
May 26, 2024

Conversation

Th3-M4jor
Copy link
Contributor

@Th3-M4jor Th3-M4jor commented May 13, 2024

This is still a work in progress, but is the beginnings of a message cache for Nostrum.

Still needs to be hooked into the event handler and I need to test it.

Adds a new Mnesia backed message cache, but defaults to a no-op cache for most users simply because it would use a lot more memory and most users don't need a message cache.

Also improves the way pluggable caching configuration works, such that users can provide a configuration to each cache, such as configuring a maximum size or how many messages get evicted when the cache reaches its maximum size

@jchristgit
Copy link
Collaborator

Thanks, this looks solid already!
There is one problem with QLC that I wonder if we can contribute a smart fix for upstream: qlc:keysort will always sort the entire table (as a list) in memory, so it's not exactly efficient on big tables. For large message caches this would mean a temporary bloating of memory on every cull. For the other "sorted by age" queries I think it's fine.
Should we maybe change the cull of old messages somehow differently... I'm thinking.. maybe we can memorize the oldest message somehow, but then we have to somehow update it - how would that work - or we need to, hm, I wonder

@Th3-M4jor Th3-M4jor force-pushed the add-message-cache branch from 34fea67 to 58f1dbc Compare May 18, 2024 21:07
@Th3-M4jor Th3-M4jor marked this pull request as ready for review May 18, 2024 21:07
@Th3-M4jor
Copy link
Contributor Author

Still need to do some real testing of it myself but other than that, this should be ready.

Do note that there are breaking changes that come with this

@Th3-M4jor Th3-M4jor force-pushed the add-message-cache branch from aa64801 to e9e6b2b Compare May 18, 2024 22:20
@jchristgit jchristgit requested a review from jb3 May 19, 2024 07:00
Copy link
Collaborator

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

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

Using an eviction count instead of always deleting the last message is pretty smart, nice!

I have a question, there is a lot of sorting going on, should we perhaps use an ordered_set table instead to just have it sorted by the primary key out of the box?

lib/nostrum/cache/message_cache.ex Outdated Show resolved Hide resolved
lib/nostrum/cache/message_cache.ex Outdated Show resolved Hide resolved
lib/nostrum/cache/message_cache.ex Outdated Show resolved Hide resolved
lib/nostrum/cache/message_cache/mnesia.ex Outdated Show resolved Hide resolved
lib/nostrum/cache/message_cache/mnesia.ex Outdated Show resolved Hide resolved
src/nostrum_message_cache_qlc.erl Outdated Show resolved Hide resolved
src/nostrum_message_cache_qlc.erl Outdated Show resolved Hide resolved
@Th3-M4jor Th3-M4jor force-pushed the add-message-cache branch from 2d838d6 to 24becdc Compare May 20, 2024 02:42
Copy link
Collaborator

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

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

Solid work!

I think I forgot to bring this up before: what do you think about using ordered_set for the cache, such that the messages are sorted by message ID by default, and we do not need to sort results everywhere? This should also speed up culling old messages if I'm not wrong.

guides/advanced/pluggable_caching.md Outdated Show resolved Hide resolved
lib/nostrum/cache/message_cache.ex Outdated Show resolved Hide resolved
lib/nostrum/cache/message_cache/mnesia.ex Outdated Show resolved Hide resolved
lib/nostrum/cache/message_cache/mnesia.ex Outdated Show resolved Hide resolved
lib/nostrum/cache/message_cache/mnesia.ex Show resolved Hide resolved
lib/nostrum/cache/message_cache.ex Outdated Show resolved Hide resolved
@Th3-M4jor Th3-M4jor force-pushed the add-message-cache branch from 010fcab to f6bedde Compare May 26, 2024 02:56
Copy link
Collaborator

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

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

This is really nice, thank you!!!!

@jchristgit jchristgit merged commit 1a695c0 into Kraigie:master May 26, 2024
9 checks passed
@jchristgit
Copy link
Collaborator

Thanks!

@Th3-M4jor Th3-M4jor deleted the add-message-cache branch May 27, 2024 04:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants