Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace lru cache with schnellru lru map #2736

Merged
merged 1 commit into from
May 16, 2024

Conversation

anujmax
Copy link

@anujmax anujmax commented May 3, 2024

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:

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contribution!

Left a few comments and please don't merge main proactively most of the time, especially before the first review, it just adds a bunch of unnecessary commits.

@@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sort dependencies alphabetically

Comment on lines 327 to 341
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@anujmax anujmax May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about wrapper as in the other file

@@ -1,5 +1,5 @@
[toolchain]
channel = "nightly-2024-04-22"
components = ["rust-src"]
targets = ["wasm32-unknown-unknown"]
targets = ["x86_64-apple-darwin"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you modify this completely unrelated properly?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nazar-pc
Copy link
Member

It'd be great if you can squash and rebase everything on top of latest main and review the changes to make sure no undesired changes are committed. I think it is close to being merged.

@anujmax anujmax force-pushed the replace-lru-with-schnellru branch from 857095a to 601c92c Compare May 14, 2024 18:08
@anujmax
Copy link
Author

anujmax commented May 16, 2024

hi @nazar-pc, could you please check now.

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

self.engine
.lock()
.report(sender, rep::GOSSIP_TOO_MANY_PROOFS);
if let Some(proofs) = self.gossip_cache.get_or_insert(sender, Default::default) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why it is necessary, asked a question about this API upstream: koute/schnellru#7

@nazar-pc nazar-pc enabled auto-merge May 16, 2024 12:27
@nazar-pc
Copy link
Member

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 😕

@nazar-pc nazar-pc added this pull request to the merge queue May 16, 2024
Merged via the queue into autonomys:main with commit fcef01b May 16, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace lru with schnellru
2 participants