deps: replace lru with schnellru
#1217
Conversation
ordian
added
R0-silent
| @@ -112,118 +109,118 @@ impl RequestResultCache { | |||
| &mut self, | |||
| relay_parent: &Hash, | |||
| ) -> Option<&Vec<AuthorityDiscoveryId>> { | |||
| self.authorities.get(relay_parent) | |||
| self.authorities.peek(relay_parent) | |||
There was a problem hiding this comment.
there's a small change in logic, as peek won't have an effect on order, but I think it's fine in our case
There was a problem hiding this comment.
Could you explain (for someone like me who isn't super familiar with this code and might not understand all of the consequences) why did you change this? Is it just unnecessary?
Maybe it could be interesting to empirically see whether this affects the cache hit rate in any measurable way? (The trie cache has some code which tracks the hit rate, so it could be transplanted/stolen as permanent debug logs here. I think it's always good to have some real data for stuff like this; if nothing else it can be used to pick an appropriate size for the caches. Although for 26 different LRU caches it might not really be practical, so maybe it'd be nice to add something like that to schnellru directly, hmm....)
Also, if you're doing this then you can also change &mut self to &self. (:
(Same for other places, as I can see you nuked every .get here.)
There was a problem hiding this comment.
It was the easiest way to satisfy the compiler since the get method in schnellru returns Option<&mut V> instead of Option<&V> :)
But what I meant by "I think it's fine for our use-case" is that it shouldn't really affect the hit rate given that the insertion order should roughly match the read order.
I've now reverted this change to keep the previous behavior.
| let val = self | ||
| .cache | ||
| .get_or_insert(*slot, || 0) | ||
| .expect("insertion with ByLength limiter always succeeds; qed"); |
There was a problem hiding this comment.
This is correct, as long as the limit is not zero. (:
| @@ -112,118 +109,118 @@ impl RequestResultCache { | |||
| &mut self, | |||
| relay_parent: &Hash, | |||
| ) -> Option<&Vec<AuthorityDiscoveryId>> { | |||
| self.authorities.get(relay_parent) | |||
| self.authorities.peek(relay_parent) | |||
There was a problem hiding this comment.
Could you explain (for someone like me who isn't super familiar with this code and might not understand all of the consequences) why did you change this? Is it just unnecessary?
Maybe it could be interesting to empirically see whether this affects the cache hit rate in any measurable way? (The trie cache has some code which tracks the hit rate, so it could be transplanted/stolen as permanent debug logs here. I think it's always good to have some real data for stuff like this; if nothing else it can be used to pick an appropriate size for the caches. Although for 26 different LRU caches it might not really be practical, so maybe it'd be nice to add something like that to schnellru directly, hmm....)
Also, if you're doing this then you can also change &mut self to &self. (:
(Same for other places, as I can see you nuked every .get here.)
Similar to substrate PR: paritytech/substrate#13802, this PR replaces usages of
lruwithschnellruin polkadot and cumulus.Reasons: