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

Reduce calls to Mattermost server by caching replies #513

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

hloeung
Copy link
Collaborator

@hloeung hloeung commented Nov 22, 2022

Replaces #508. For each message that's a reply to a thread, we make an extra API call to get the parent message to use as reply text. This caches that to reduce requests to the MM server.

@hloeung hloeung force-pushed the cache-reply-msgs branch 2 times, most recently from b7f3c7c to 5c7d483 Compare November 22, 2022 03:12
@hloeung hloeung requested a review from 42wim November 22, 2022 03:24
@hloeung hloeung marked this pull request as ready for review November 22, 2022 08:21
@hloeung
Copy link
Collaborator Author

hloeung commented Nov 23, 2022

Memory usage is pretty good and remains quite constant. This is with ShortenRepliesTo = 20 but shows that there isn't any memory leaks:

$ while echo -n "$(date): "; do ps aux | grep './m\atterircd'; sleep 1800; done
Tue Nov 22 10:29:56 PM UTC 2022: ubuntu   52779  0.1  0.4 724940 32732 pts/2    Sl+  02:46   2:18 ./matterircd
Tue Nov 22 10:59:56 PM UTC 2022: ubuntu   52779  0.1  0.4 724940 32608 pts/2    Sl+  02:46   2:21 ./matterircd
Tue Nov 22 11:29:56 PM UTC 2022: ubuntu   52779  0.1  0.4 724940 32608 pts/2    Sl+  02:46   2:24 ./matterircd
Tue Nov 22 11:59:56 PM UTC 2022: ubuntu   52779  0.1  0.4 724940 32608 pts/2    Sl+  02:46   2:28 ./matterircd
Wed Nov 23 12:29:56 AM UTC 2022: ubuntu   52779  0.1  0.4 724940 32680 pts/2    Sl+  Nov22   2:31 ./matterircd
Wed Nov 23 12:59:56 AM UTC 2022: ubuntu   52779  0.1  0.4 724940 32764 pts/2    Sl+  Nov22   2:34 ./matterircd
Wed Nov 23 01:29:56 AM UTC 2022: ubuntu   52779  0.1  0.4 724940 32764 pts/2    Sl+  Nov22   2:37 ./matterircd
Wed Nov 23 01:59:56 AM UTC 2022: ubuntu   52779  0.1  0.4 724940 32764 pts/2    Sl+  Nov22   2:40 ./matterircd
Wed Nov 23 02:29:56 AM UTC 2022: ubuntu   52779  0.1  0.4 724940 32764 pts/2    Sl+  Nov22   2:43 ./matterircd
Wed Nov 23 02:59:56 AM UTC 2022: ubuntu   52779  0.1  0.4 724940 32764 pts/2    Sl+  Nov22   2:47 ./matterircd
Wed Nov 23 03:29:56 AM UTC 2022: ubuntu   52779  0.1  0.4 724940 32720 pts/2    Sl+  Nov22   2:50 ./matterircd
Wed Nov 23 03:59:56 AM UTC 2022: ubuntu   52779  0.1  0.4 724940 32720 pts/2    Sl+  Nov22   2:53 ./matterircd
Wed Nov 23 04:29:56 AM UTC 2022: ubuntu   52779  0.1  0.4 724940 32752 pts/2    Sl+  Nov22   2:56 ./matterircd

Copy link
Owner

@42wim 42wim left a comment

Choose a reason for hiding this comment

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

Maybe it's better to use the https://github.com/hashicorp/golang-lru which handles the cleanup and locking for you.
You also don't need the maps then.

cache := lru.New(30)
var (
    replyMessage string
    ok bool
)

if replyMessage,ok = cache.Get(parentID); !ok {
   do the GetPost stuff
   ...
}

This removes all the complexity of maps/counters/locking and lets the 3rd party library deal with it.
The keys should be unique enough to just put them in the main cache instead of mapping it per channel.

WDYT ?

@hloeung hloeung force-pushed the cache-reply-msgs branch 4 times, most recently from ff5bf93 to 032f0b6 Compare December 3, 2022 22:41
@hloeung
Copy link
Collaborator Author

hloeung commented Dec 3, 2022

This removes all the complexity of maps/counters/locking and lets the 3rd party library deal with it.
The keys should be unique enough to just put them in the main cache instead of mapping it per channel.

WDYT ?

Good call/advice, I've updated the PR switching to using the 3rd party HashiCorp LRU library.

I think the advantage of using per-channel caching is that a channel that sees less activity would not have cached entries pushed out of the cache because of having channels with more activity. But yeah, this simplifies the code (less code, less bugs and all that), we can further tune the cache size if needed.

@hloeung hloeung requested a review from 42wim December 3, 2022 22:49
Copy link
Owner

@42wim 42wim left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Haven't tested it yet though. But fine to merge if you battletested it, otherwise I'll test tomorrow

@42wim 42wim added this to the v0.27.0 milestone Dec 3, 2022
@hloeung
Copy link
Collaborator Author

hloeung commented Dec 4, 2022

Thanks, I'm running with it now but not in a rush to have it landed so happy to wait for additional testing 👍

@42wim 42wim merged commit f253562 into 42wim:master Dec 14, 2022
@hloeung hloeung deleted the cache-reply-msgs branch December 15, 2022 08:39
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.

2 participants