Cache result of LocalSite::read to avoid unnecessary db calls #4585
Conversation
Nutomic
requested review from
dessalines,
phiresky and
SleeplessOne1917
as code owners
| static CACHE: Lazy<Cache<(), LocalSite>> = Lazy::new(|| { | ||
| Cache::builder() | ||
| .max_capacity(1) | ||
| .time_to_live(Duration::from_secs(1)) | ||
| .build() | ||
| }); |
There was a problem hiding this comment.
Should the static be defined in the function? I thought stuff like this was usually handled at the module level.
There was a problem hiding this comment.
Its only used in this one function, so no need to make it available anywhere else.
There was a problem hiding this comment.
why not extend the time to live, and invalidate cache in the 'update' method? 1 sec is really short
There was a problem hiding this comment.
Because some Lemmy instances may run more than one backend for horizontal scaling. So the value can be updated through another backend and we wont know about it unless we read again from the db.
There was a problem hiding this comment.
I grepped the other time_to_live , and like the others it'd be good to extract this into a const somewhere. Ideally they should all live in utils or someplace common.
There was a problem hiding this comment.
improving expensive db queries is needed. removing 0.1ms db queries will not make a dent unless called thousands of times per second
There was a problem hiding this comment.
Does the time to live make it so the cache is invalidated every second?
There was a problem hiding this comment.
yes, as is this will make the number of queries for local site go to around 1 per second for an active instance. there's similar caching elsewhere. the number could potentially be higher but since this is a cheap query this is probably fine
There was a problem hiding this comment.
I'm surprised that 1 second is enough to improve performance. This must be called a lot of times per second.
There was a problem hiding this comment.
Lemmy.ml receives roughly 230 votes per minute, so it will be called at least that many times. Actually more for post creations and other actions. Still it probably wont make a noticable difference unless you aggressively cache api responses.
|
Changed it to use the same const for all cache durations (with shorter duration in debug mode). |
This function is called in a lot of different places, for example whenever a vote is made or a post is created, whether locally or over federation. This value changes very rarely, so we can avoid unnecessary db calls by caching it.