<Cache as Clone> has overly-restrictive trait bounds

[lilyball]

The various Cache types derive Clone, which means they require where K: Clone, V: Clone, S: Clone. This is overly restrictive. Since the underlying BaseCache on each type has no trait bounds on their Clone impls, the Caches don't need them either.

Besides being annoying, this also means that the cache key needs to be cloneable in order to clone the cache, even though this is not otherwise a requirement for cache keys.

[tatsuya6502]

Thank you for reporting the issue.

As a short answer, I will remove Clone from K as it was added by mistake, but will keep Clone for V and S.

• Remove Clone from K in all caches. It was added by mistake.
• Keep Clone for V and S in all caches except unsync::Cache. (no change)
  • Removing Clone from V is not possible for these concurrent caches.
  • Removing Clone from S is possible in some of the caches. But will not do it for performance reason.

Here are longer answers:

K

As for K: Clone when Cache is Clone, I will remove this bound because it was added by mistake. (commit)

V

As for V: Clone (except unsync::Cache), this is intentional and I cannot remove it.

All concurrent caches (the caches under sync, future and dash) return a clone of V for get method to provide full concurrency. If V is not Clone, get method should return &V wrapped by a reader-writer-lock (something like RwLockGuard<'a, Option<&'a V>>), and this lock will spoil concurrency.

If you want to store V that is not Clone, please wrap it with std::sync::Arc. See here for more details.

S

As for S: Clone, this is intentional, and I want to keep it if there is no strong reason to remove.

An instance of cache can internally have two hash maps and each of them own an instance of S.

So, I cannot remove Clone from S in the following caches:

• unsync::Cache — This uses std::collection::HashMap internally, which takes S.
• dash::Cache — uses dashmap::HashMap internally, which takes S: Clone.

But I could remove Clone from S in the following caches:

• sync::Cache
• sync::SegmentedCache
• future::Cache

They use our own implementation of hash map, so I could change it to take Arc<S> instead of S.

However, I do not want to remove Clone due to performance reasons. Wrapping with Arc will prevent the Rust compiler to inline hashing functions. Some internal operations like rehash may get non-negligible performance degradation as rehash reallocates a bigger storage space in heap memory and copy existing entries to the new space by recalculating hash of every keys.

On the other hand, implementing Clone on S: BuildHasher will be easy, and you will rarely write your own BuildHasher anyway. I checked the following popular BuildHasher implementations and found they all implement Clone:

• std::collections::hash_map::RandomState (doc)
• ahash::RandomState (doc)
• fnv::FnvBuildHasher (doc)
• rustc_hash::FxHasher (doc

// This code fragment compiles.

use std::hash::BuildHasherDefault;

let bh = std::collections::hash_map::RandomState::default();
bh.build_hasher();
let _ = bh.clone();

let bh = ahash::RandomState::default();
bh.build_hasher();
let _ = bh.clone();

let bh = fnv::FnvBuildHasher::default();
bh.build_hasher();
let _ = bh.clone();

let bh = BuildHasherDefault::<rustc_hash::FxHasher>::default();
bh.build_hasher();
let _ = bh.clone();

I see dashmap made a design choice not to hold Arc<S> internally, but to require S: Clone. I believe it is a right choice and I would take the same choice.

[lilyball]

Thank you for the detailed explanation. It looks like you actually removed all the bounds from the Clone impls anyway so that's good.

More generally it would be nice to remove bounds from methods where they aren't necessary. For example, all of the moka::future::Cache methods require V: Clone even though AFAIK this is only actually necessary on the getters. Similarly methods like iter() still require S: BuildHasher even though AFAICT there's no need to hash anything for iteration. And everything requires S: Clone even though it looks like this may only be required for new().

Removing unnecessary bounds from traits (like Clone) is probably the most important as that prevents propagating unnecessary bounds across generic code involving that trait, but there are potential simplifications to generic code using the cache if the cache methods similarly have unnecessary bounds removed.

For an example of precedent, if you look at HashMap you'll see it has multiple inherent impls with different bounds based on what is necessary for the methods. So e.g. get and insert require S: BuildHasher but iter and len do not.

[tatsuya6502]

It looks like you actually removed all the bounds from the Clone impls anyway so that's good.

Yes, I did.

In my previous comment, I meant I cannot remove these bounds (e.g. V: Clone) from new function for example, so nobody will be able to create a cache when V is not Clone, thus nobody can clone it.

More generally it would be nice to remove bounds from methods where they aren't necessary.

Okay. I think I got your point. Thank you for explaining it.

I will create a separate issue for removing unnecessary bounds from some methods and do it. I think I have done for iter as a part of #134, but there are much more to do.