Is it worth to modify the key of `get_with` as a `&K` rather than `K`?

[moka-rs]

Is it worth to modify the key of `get_with` as a `&K` rather than `K`?

[tatsuya6502]

Thanks. No need to rush. Please take your time.

If we decided to use entry_by_ref, I would keep it consistent and use get_with_by_ref/try_get_with_by_ref even though it is a bit long.

I started to play with the entry API and I am almost certain to use entry_by_ref there. So using get_with_by_ref / try_get_with_by_ref would make sense.

I have got the following code to compile.

https://gitlab.com/moka-labs/moka-entry-api/moka-entry-api-mock/-/blob/main/src/main.rs#L12-27

    let cache: Cache<String, String> = Cache::new(100);
    let key = "key".to_string();

    let entry = cache
        .entry_by_ref(key.as_str())
        .or_insert_with(|| "value".to_string());
    dbg!(entry.is_fresh()); // bool
    // dbg!(entry.key()); //   &K
    dbg!(entry.value()); //    &V
    let _value = entry.into_value(); // V
    // dbg!(entry.value()); // Error: Borrow of moved value.

    let _value = cache
        .entry(key)
        .or_insert_with(|| "value".to_string())
        .into_value(); // V

Note: I am not directly modifying Moka's source code yet, but created a thin wrapper called moka_entry_api_mock to see if all those trait and lifetime bounds would work.

    use moka_entry_api_mock as moka_mock;
    use moka_mock::sync::Cache;

Also, I realized the ToOwned bound in the previous example was wrong. The correct bound should be the following:

pub fn get_with_by_ref(&self, key: &Q, init: impl FnOnce() -> V) -> V
where
    K: Borrow<Q>,
    Q: ToOwned<Owned = K> + Hash + Eq + ?Sized,

Owned = K was needed to create K from &Q in my thin wrapper:

https://gitlab.com/moka-labs/moka-entry-api/moka-entry-api-mock/-/blob/main/src/sync.rs#L85

impl<'a, K, Q, V, S> RefKeyEntrySelector<'a, K, Q, V, S>
where
    K: Borrow<Q> + Hash + Eq + Send + Sync + 'static,
    Q: ToOwned<Owned = K> + Hash + Eq + ?Sized,
    V: Clone + Send + Sync + 'static,
    S: BuildHasher + Clone + Send + Sync + 'static,
{
    pub fn or_insert_with(self, init: impl FnOnce() -> V) -> Entry<K, V> {
        let owned_key: K = self.ref_key.to_owned();

[LMJW]

Push a PR, feel free to take a look when you have time @tatsuya6502 :)

[tatsuya6502]

Thank you for the suggestion. We can achieve this by cloning the key (at line 1054) in an internal method get_or_insert_with_hash_and_fun.

I would add them as new APIs rather than changing the current ones. The current ones will still have advantages in some use cases:

• New ones (to pass key by reference) will be better when:
  • You own the key, and you still need it after calling the API. (and the key type implements Clone)
  • Or, you do not own the key, but have an reference to it, and the key type implements Clone.
• Current ones (to pass key by value) will be better when:
  • You own the key, and you do not need it after calling the API.
    • If so, cloning the key in the API is redundant.
  • Or, your key type does not implement Clone.

use moka::sync::Cache;

let cache = Cache::new(100);
let key = "key".to_string();

// New API
let _ = cache.rget_with(&key, || 0);

// Current API
// We can move the key into the API because we do not use it after calling the API.
let _ = cache.get_with(key, || 0);

The reason I made the current APIs to pass key by value is that the entry API in std::collections::HashMap does so:

https://doc.rust-lang.org/stable/std/collections/struct.HashMap.html#method.entry

pub fn entry(&mut self, key: K) -> Entry<'_, K, V>

But I understand there are some use cases that the current APIs are not suitable for.

I am not sure for the naming of the new APIs. In the above example, I used rget_* where r stands for "reference" (key by reference). It is a silly name, but I thought we would prefer a short name? I am open to any suggestions.

The method signature will be like this:

pub fn rget_with(&self, key: &Q, init: impl FnOnce() -> V) -> V
where
    K: Borrow<Q>,
    Q: ToOwned + Hash + Eq + ?Sized,

We will use ToOwned rather than Clone, because it supports creating String from &str, in addition to from &String. Note that all Clone types automatically implement ToOwned.

[tatsuya6502]

FYI, I am planning to move some advanced or unpopular APIs to under a new API that mimics the entry API in the std HashMap. I have not finalized the design yet, but it will be something like this:

// Pass key by reference
let entry = cache.entry_by_ref(&key).or_insert_with(|| 0);

// `is_fresh` is a new API requested in https://github.com/moka-rs/moka/issues/136.
// => `true`:  the entry is just inserted.
//    `false`: the entry is already in the cache.
log::info!(entry.is_fresh());
// Consume the entry to get the owned value.
let value = entry.into_value();

// Pass key by value
let value = cache.entry(key.clone()).or_insert_with(|| 0).into_value();

// Advanced or unpopular APIs
// `compute` is a new API requested in https://github.com/moka-rs/moka/issues/179.
cache.entry_by_ref(&key)
    .compute(
        // If the entry is not in the cache, insert a new value.
        || 0,
        // If the entry is in the cache, update the value by returning `Some()`,
        // or invalidate the entry by returning `None`.
        |old_value| Some(old_value += 1));

// `get_with_if` is an existing API and we will move it to the `entry` API.
cache.entry_by_ref(&key)
    .get_or_insert_with_if(
        // If the entry is not in the cache, insert a new value.
        || 0,
        // Invalidate the entry if the closure returns `true`.
        |value| value >= 10;

So we will have entry_by_ref and entry to pass key by reference and by value, respectively.

And we will keep/add the following convenient APIs as shortcuts of above:

// Pass key by reference
cache.rget_with(&key, || 0);
let _ = cache.try_rget_with(&key, || Ok(0));
let _ = cache.optionally_rget_with(&key, || Some(0));

// Pass key by value
cache.get_with(key.clone(), || 0);
let _ = cache.try_get_with(key.clone(), || Ok(0));
let _ = cache.optionally_get_with(key.clone(), || Some(0));

[LMJW]

Hi @tatsuya6502 . I think rget_with/try_rget_with is fine. A potential alternative is get_with_ref/try_get_with_ref or even get_with_by_ref/try_get_with_by_ref to matching entry_by_ref style. I will spend sometime on this once the other PR is finalized.

[tatsuya6502]

Thanks for the method name ideas. I have no preference/it is hard to choose to me. All names are fine. If you have any preference, please pick one.

[LMJW]

I don't have a strong preference. If we decided to use entry_by_ref, I would keep it consistent and use get_with_by_ref/try_get_with_by_ref even though it is a bit long.

I will look into this issue a bit later this week. :)

[LMJW]

Thanks @tatsuya6502 . The new entry api looks pretty cool. I like it. Also, I will try out the trait bound when I implement the by_ref api. I think it should work. :)

One question I have about the entry api is, if the entry returns the reference to V, since this is a concurrent cache, if it is called from another thread/tokio task to write the same entry, will there be any potential contentions? I haven't dive into the code to see how this will work. But just curious about how to solve this problem.

[tatsuya6502]

One question I have about the entry api is, if the entry returns the reference to V, since this is a concurrent cache, if it is called from another thread/tokio task to write the same entry, will there be any potential contentions?

No, there is no contention. or_insert_with() method returns an Entry instance, and that entry owns a clone of value V. The &V returned from value() is referencing the value in the entry, instead of the one in the cache. So, there is no lock to guard the value in the cache and other threads are free to replace it.

Yeah, it might be a bit confusing.

[LMJW]

I got it. Thanks for the explaination.