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

in-memory cache: take owned values instead of borrowing #1417

Open
AEnterprise opened this issue Jan 1, 2022 · 3 comments
Open

in-memory cache: take owned values instead of borrowing #1417

AEnterprise opened this issue Jan 1, 2022 · 3 comments
Labels
c-cache Affects the cache crate m-breaking change Breaks the public API. t-refactor Refactors APIs or code. w-needs-more-docs Needs more documentation before being worked on.

Comments

@AEnterprise
Copy link
Member

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

@AEnterprise AEnterprise added c-cache Affects the cache crate m-breaking change Breaks the public API. t-enhancement labels Jan 1, 2022
@zeylahellyer zeylahellyer added the w-needs-more-docs Needs more documentation before being worked on. label Jan 1, 2022
@vilgotf
Copy link
Member

vilgotf commented Jan 1, 2022

This change follows C-CALLER-CONTROL

@zeylahellyer
Copy link
Member

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:

  1. How do we guard against the user unnecessarily cloning events (i.e. only events the cache wants)?
  2. 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?

@7596ff 7596ff added t-refactor Refactors APIs or code. and removed t-enhancement-DEPRECATED labels Jan 12, 2022
@laralove143
Copy link
Member

couldn’t there just be two methods, one taking by reference and only cloning what needs to be cached inside, and another taking ownership

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-cache Affects the cache crate m-breaking change Breaks the public API. t-refactor Refactors APIs or code. w-needs-more-docs Needs more documentation before being worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants