Prefer ordered collections (BTreeMap / BTreeSet) over hash based collections #33
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
rklaehn
force-pushed
the
rkl/ordered-collections
branch
2 times, most recently
from
f2e7dd0 to
1834c5b
Compare
rklaehn
commented
Jul 24, 2020
protocols/gossipsub/src/behaviour.rs
Outdated
| for (peer, controls) in self.control_pool.drain().collect::<Vec<_>>() { | ||
| let mut old = BTreeMap::new(); | ||
| std::mem::swap(&mut self.control_pool, &mut old); | ||
| for (peer, controls) in old.into_iter() { |
There was a problem hiding this comment.
This is necessary because BTreeMap does not have drain(), so basically manual drain.
rklaehn
commented
Jul 24, 2020
protocols/gossipsub/src/behaviour.rs
Outdated
| fanout.remove(&topic_hash); | ||
| return false; | ||
| let now = Instant::now(); | ||
| let expired_topics = self.fanout_last_pub.iter().filter_map( |
There was a problem hiding this comment.
This two stage approach is necessary because BTreeMap does not yet have retain. It is in the works, but for now we first have to figure out what to remove, and then actually remove it.
For the probably quite common case where nothing is removed, this does not allocate anything.
|
Not sure why ci is failing. Ran tests and doc locally without issue. Might be a fluke. |
rklaehn
force-pushed
the
rkl/ordered-collections
branch
from
fe76e95 to
f9cfc8d
Compare
rklaehn
force-pushed
the
rkl/ordered-collections
branch
from
0de44d7 to
7b76c8f
Compare
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.
This is a matter of preference/taste. So take it or leave it :-)
see e.g. https://users.rust-lang.org/t/hashmap-vs-btreemap/13804
for peer ids, the Ord instance is very fast since they are essentially random and therefore will differ in some way in the first few bytes
to get best performance out of rust hash based collections, you need to use a quick, deterministic hasher like https://docs.rs/fnv/1.0.7/fnv/struct.FnvHasher.html . But then you are open to hash collision attacks again. Using ordered collections completely avoids the problem and is IMHO much simpler.