Comments (3)
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.
from moka.
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
fromK
in all caches. It was added by mistake. - Keep
Clone
forV
andS
in all caches exceptunsync::Cache
. (no change)- Removing
Clone
fromV
is not possible for these concurrent caches. - Removing
Clone
fromS
is possible in some of the caches. But will not do it for performance reason.
- Removing
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 usesstd::collection::HashMap
internally, which takesS
.dash::Cache
— usesdashmap::HashMap
internally, which takesS: 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.
from moka.
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 impl
s 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.
from moka.
Related Issues (20)
- CI: Enable Miri tests on `moka::cht::*` modules
- Memory corruption observed when using Moka v0.9.6 HOT 15
- unbounded capacity? HOT 2
- Possibility of using async runtime tasks instead of thread pools HOT 10
- oom caused after use #234's statistics record code HOT 4
- How can i add something to the cache inside the `eviction_listener`? HOT 5
- Moka loses cache with curl HOT 9
- Provide an easy way to implement per-entry TTL and TTI HOT 6
- Enable `clippy::arc_with_non_send_sync` lint for active branches
- Provide a way to iterate entries with their metadata
- Provide a way to get a read-only snapshot of the `FrequencySketch` of `Cache`
- Provide a way to restore a `Cache` from entries with metadata and a `FrequencySketch` snapshot HOT 2
- CI: Temporary disable CirrusCI HOT 3
- wasm compatibility - change Expiry to avoid needing std::time::Instant::now() HOT 4
- An internal `do_insert_with_hash` method gets the current `Instant` too early when eviction listener is enabled HOT 1
- Tracking issue for restoring cache state from backed up entries and a snapshot of the LFU filter
- Reason for `Arc<Error>` HOT 2
- Memory leak in moka 0.12 HOT 13
- With Rust 1.73.0, some unit tests started to fail for `mips-unknown-linux-musl` target HOT 7
- not support `armv5te-unknown-linux-musleabi` HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from moka.