replace lru cache with schnellru lru map #2736
Conversation
anujmax
requested review from
shamil-gadelshin,
nazar-pc and
rg3l3dr
as code owners
| @@ -17,7 +17,7 @@ async-oneshot = "0.5.9" | |||
| futures = "0.3.29" | |||
| futures-timer = "3.0.3" | |||
| jsonrpsee = { version = "0.22.5", features = ["server", "macros"] } | |||
| lru = "0.12.3" | |||
| schnellru = "0.2.1" | |||
There was a problem hiding this comment.
Please sort dependencies alphabetically
| let changes: HashMap<_, _> = self | ||
| .gossip_cache | ||
| .iter() | ||
| .map(|(sender, proofs)| { | ||
| let mut retained_proofs = VecDeque::new(); | ||
| for proof in proofs { | ||
| if proof.slot > next_slot_input.slot { | ||
| retained_proofs.push_back(*proof); | ||
| } else { | ||
| old_proofs.entry(*proof).or_default().push(*sender); | ||
| } | ||
| } | ||
| }); | ||
| (*sender, retained_proofs) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
I see there is no .retain() API, but you could just iterate once to collect old proofs and then iterate over old proofs to delete them from cache explicitly.
Extra allocations and re-insertion is awkward and just wastes both CPU and RAM. Previous code was trying to minimize memory usage by proactively removing outdated records, what you're doing with re-insertion without wiping is basically doing useless work and achieves nothing in return.
There was a problem hiding this comment.
actually there is .retain() API on VecDeque<GossipProof> but there is no iter_mut in LruMap, so I cannot mutate the proofs while iterating(if my understanding is correct) . I was trying to avoid this :
for (sender, proofs) in self.gossip_cache.iter() {
for proof in proofs {
if proof.slot <= next_slot_input.slot {
old_proofs.entry(*proof).or_default().push(*sender);
}
}
}
// this is n³
for (proof, senders) in old_proofs.iter() {
for sender in senders {
if let Some(proofs) = self.gossip_cache.get(sender) {
proofs.retain(|p| p != proof);
if proofs.is_empty() {
self.gossip_cache.remove(sender);
}
}
}
}
I am also okay to implement iter_mut on the LruMap if you think that is the better approach.
There was a problem hiding this comment.
The absolute numbers should be small there so it is not a lot of iterations. Suggested code is what I expected, but if you can implement mutable iteration that would be even better for sure. Basically old code would not need to be changed.
There was a problem hiding this comment.
created PR:
koute/schnellru#5
I also tested gossip.rs by pointing to this in my local. Will push the changes once released.
| cache: Arc<Mutex<PotVerifierLruMap>>, | ||
| } | ||
|
|
||
| struct PotVerifierLruMap(LruMap<CacheKey, CacheValue>); |
There was a problem hiding this comment.
You should open PR (or at very least issue) upstream implementing Debug trait for LruMap and leaving a TODO here to remove this wrapper (or ideally switching to version that has Debug implemented from the very beginning).
Also if you have to implement something custom here I'd rather have manual Debug on PotVerifier explicitly an have just non-exhaustive implementation fo LruMap struct without actual entries. The cache can be quite large, no need to dump the whole thing in debug output. Either way TODO is required with link to upstream issue/PR.
There was a problem hiding this comment.
I have created the PR:
koute/schnellru#4
If this is not urgent, I will wait for the my upstream PR to merged and released and then I will change the versions here.
There was a problem hiding this comment.
Not urgent and I expect them to merge it quickly as well.
| @@ -44,7 +55,7 @@ impl PotVerifier { | |||
| slot_iterations: NonZeroU32, | |||
| checkpoints: PotCheckpoints, | |||
| ) { | |||
| self.cache.lock().push( | |||
| self.cache.lock().0.insert( | |||
There was a problem hiding this comment.
It is better to implement Deref and DerefMut on such wrappers so you don't need to use .0.
| @@ -58,19 +58,27 @@ impl TemporaryBan { | |||
| } | |||
| } | |||
|
|
|||
| struct TemporaryBannedPeersLruMap(LruMap<PeerId, TemporaryBan>); | |||
There was a problem hiding this comment.
Same comment about wrapper as in the other file
rust-toolchain.toml
Outdated
| @@ -1,5 +1,5 @@ | |||
| [toolchain] | |||
| channel = "nightly-2024-04-22" | |||
| components = ["rust-src"] | |||
| targets = ["wasm32-unknown-unknown"] | |||
| targets = ["x86_64-apple-darwin"] | |||
There was a problem hiding this comment.
Why did you modify this completely unrelated properly?
There was a problem hiding this comment.
my mistake, sorry 🙏🏽. I will remove it in the squashed commit. Also, I will check that I do not have any irrelevant changes in the commit.
|
It'd be great if you can squash and rebase everything on top of latest |
anujmax
force-pushed
the
replace-lru-with-schnellru
branch
from
857095a
to
601c92c
Compare
|
hi @nazar-pc, could you please check now. |
| self.engine | ||
| .lock() | ||
| .report(sender, rep::GOSSIP_TOO_MANY_PROOFS); | ||
| if let Some(proofs) = self.gossip_cache.get_or_insert(sender, Default::default) { |
There was a problem hiding this comment.
Not sure why it is necessary, asked a question about this API upstream: koute/schnellru#7
|
BTW there is a button to re-request review when necessary in reviewers section. GitHub changed something recently and there are no email notifications about force-pushes anymore 😕 |
this PR replaces usages of lru with schnellru for following crates:
sc-consensus-subspace-rpc
sc-consensus-subspace
sc-proof-of-time
subspace-farmer
subspace-networking
Closes #2598
Code contributor checklist: