twilight
twilight copied to clipboard
in-memory cache: take owned values instead of borrowing
Taking an owned value instead of a borrowing means the cache doesn't have to clone data to construct it's cached models.
The advantage of this is no forced cloning of data but simply moving around owned data to avoid additional overhead. While minor at small scale, this can does add up with the amount of events per second that need to be processed goes up.
The disadvantage would be the caller has no ownership anymore so would be forced to clone it if they want to do anything with the event after the cache has been updated
This change follows C-CALLER-CONTROL
I'm going to close #1430 and post a more concise version of my opinion here. #1430 can certainly be reused in the future, but is incomplete in its current intent.
The PR updates the signature from a borrowed event to an owned event, but the issue is that this will unnecessarily clone events that the cache does not want (see InMemoryCache::wants). This is, in fact, one reason why it takes references today. This problem is further compounded by the API being introduced in #1350 (one day).
Before creating another PR, we need to figure out what kind of API would work here that would allow conditionally passing in owned values. To do this, I've thought of two problems to solve:
- How do we guard against the user unnecessarily cloning events (i.e. only events the cache wants)?
- If it does want an event, is it possible to only clone a portion of it? Consider a massive event - such as
GuildCreate- when the cache wants only a subset of the event, such as emojis or voice states.
Towards 1, consider a sample pseudo-API like so:
impl InMemoryCache {
pub fn update(&self, event: &Event) -> Option<Updater<'_>> {
if self.wants(event) {
Updater::new(self)
} else {
None
}
}
}
impl Updater {
pub fn commit(&self, event: Event) {
// process event
}
}
// used like
if let Some(updater) = cache.update(&event) {
updater.commit(event);
}
What are the pitfalls of this API? In what ways will it solve our problems? Why? Is it interoperable with 2 in any way, and can we even solve 2?
couldn’t there just be two methods, one taking by reference and only cloning what needs to be cached inside, and another taking ownership