ref(lru-cache): Change lru cache crate to avoid vulnerable dependency #478
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some quick and dirty benchmarks for the caches and lru crates for the LRU cache.
This makes use the the facilities to compare two functions and also uses iter_batches to handle setup and dropping correctly. The results are widely different! The differences between both implementations are much muchness really, not much to go either way.
The performance is pretty similar and we don't use any features of the bigger crate.
This makes sure it no longer shows up in cargo-audit.
b5
reviewed
Nov 9, 2022
iroh-p2p/benches/lru_cache.rs
Outdated
Comment on lines
22
to
23
| //! The bench functions for the `caches` crate are commented out in order not to require a | ||
| //! dependency on the crate. |
There was a problem hiding this comment.
Let's remove these entirely, and reposition this PR as swapping out the caches crate for lru, but also comes with the upside of having a benchmark
b5
reviewed
Nov 9, 2022
Comment on lines
+6
to
+18
| //! # Running the benchmarks | ||
| //! | ||
| //! Install `cargo-criterion`: | ||
| //! | ||
| //! ```shell | ||
| //! cargo install cargo-criterion | ||
| //! ``` | ||
| //! | ||
| //! Run the benchmarks: | ||
| //! | ||
| //! ```shell | ||
| //! cargo criterion -p iroh-p2p | ||
| //! ``` |
There was a problem hiding this comment.
ran these locally, works! If we add more benchmarks that use this pattern we can define an xtask to run them. I'd skip it for this one, as we're mainly hanging on to this bench for future use
There was a problem hiding this comment.
I am confused, you can just run the benchmarks with cargo bench if the config is setup correctly: https://github.com/bheisler/criterion.rs#quickstart
There was a problem hiding this comment.
true, i forgot that worked by the time i was finished: #483
rklaehn
previously approved these changes
Nov 9, 2022
rklaehn
approved these changes
Nov 9, 2022
| use criterion::{criterion_group, criterion_main, BatchSize, Criterion}; | ||
| use libp2p::PeerId; | ||
|
|
||
| // The size of the cache to make. Taken from behaviour::peer_manager::DEFAULT_BAD_PEER_CAP. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change the LRU cache from the caches crate to the lru crate. This crate is much simpler and does not depend on a vulnerable version of the time crate, resolving our cargo-audit issues.
A benchmark is included showing our use of the LRU cache to have no notable performance difference between both crates.
Closes n0-computer/beetle#131