Chore: Replace Contains Key with Entry #4483
Conversation
…etter benchmark results
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #4483 +/- ##
==========================================
- Coverage 85.25% 83.41% -1.85%
==========================================
Files 471 471
Lines 337762 337762
Branches 317 317
==========================================
- Hits 287955 281728 -6227
- Misses 49799 56026 +6227
Partials 8 8
... and 65 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
- fallback to `contains_key()` in places where inserts aren't made - swap `get() ... insert()` on maps with `get_mut() += value` where available
|
I've run the benchmark again after the latest commit (a8597f1), compared to the previous one (885b6e5). The results show a decrease of Benchmark 1: ./stacks-inspect-entry-no-inserts replay-block ~/chainstate range 99990 100000
Time (mean ± σ): 6.694 s ± 0.011 s [User: 6.310 s, System: 0.350 s]
Range (min … max): 6.680 s … 6.711 s 10 runs
Benchmark 2: ./stacks-inspect-entry-inserts replay-block ~/chainstate range 99990 100000
Time (mean ± σ): 6.587 s ± 0.009 s [User: 6.227 s, System: 0.325 s]
Range (min … max): 6.573 s … 6.606 s 10 runs
Summary
./stacks-inspect-entry-inserts replay-block ~/chainstate range 99990 100000 ran
1.02 ± 0.00 times faster than ./stacks-inspect-entry-no-inserts replay-block ~/chainstate range 99990 100000In this case, |
There was a problem hiding this comment.
See earlier comment.
EDIT: I see my email comment didn't land on this PR for some reason. Here's what it meant to say:
I'm really not a fan of the entry API. It takes several (sometimes 10x) lines of code to express that which can be done in a single line with contains_key(), so code legibility and density are things that would be sacrificed by merging this PR. Moreover, to use this API, you needed to manually transform lots of code on consensus-critical code paths, which makes merging this PR super risky. Finally, the speed improvements are basically nonexistent (a questionable 1% speedup?), which means the wins from the PR are basically nonexistent.
As is, I'm a NACK.
This PR is currently at
It's a simple and straightforward transformation, similar to transforming let resp = self
.requests
.remove(&req)
.expect("BUG: had key but then didn't")
.expect("BUG: had response but then didn't");IMO this makes the entry API safer to use than indexing a The unit and integration tests are currently passing, but we can also do some extra testing here
The numbers in the description are incorrect, it's about a 2% speedup in block processing. See the second comment on this issue |
I'm really not seeing how this: match foo.get(&bar).entry() {
Entry::Occupied(value) => {
/* do something */
}
Entry::Vacant(e) => {
/* do something else */
}
}is a legibility improvement over this: if foo.contains_key(&bar) {
/* do something */
} else {
/* do something else */
}The former has two levels of indentation, whereas the latter has one. Also, the former has 6 lines of boilerplate, whereas the latter has 3. We've been trying to reduce gratuitous indentations for the past few years because it aids in comprehensibility, but this PR works against it. Do we know for sure that using the |
I don't know about that. Sure the second example is less text, but the first is more explicit because both cases are clearly labeled, rather than using an if !self.prune_inbound_counts.contains_key(prune) {
self.prune_inbound_counts.insert(prune.clone(), 1);
} else {
let c = self.prune_inbound_counts.get(prune).unwrap().to_owned();
self.prune_inbound_counts.insert(prune.clone(), c + 1);
}vs. match self.prune_inbound_counts.entry(prune.to_owned()) {
Entry::Occupied(mut e) => {
*e.get_mut() += 1;
}
Entry::Vacant(e) => {
e.insert(1);
}
}Line count went up, but the second is fewer characters and much more readable. In the first, you have to read it and think about what it's doing, whereas it's obvious as soon as you look at the second that you're incrementing a counter. Also, there's no
The The perf gains are probably entirely from the few changes in clarity. We should at least merge those. I'm not sure the other changes in stackslib are even used in the
You'd think, but during my testing I've been surprised by what the compiler doesn't optimize away at |
Yup, 100% onboard with this. Now that you mention it, it was surprising that these changes impacted code outside of
The example you're citing here could also be refactored as follows, with less boilerplate and the same number of clones: if let Some(c) = self.prune_inbound_counts.get_mut(prune) {
*c += 1;
} else {
self.prune_inbound_counts.insert(prune.clone(), 1);
}Granted, there's a lot of really old code in the |
|
Also, I think that using if !lowest_reward_cycle_with_missing_block.contains_key(nk) {
lowest_reward_cycle_with_missing_block.insert(nk.clone(), reward_cycle);
}vs. if let Entry::Vacant(e) = lowest_reward_cycle_with_missing_block.entry(nk) {
e.insert(reward_cycle);
}Aside from the Clarity perf gains, there are some clear readability improvements and |
|
I disagree -- I think it requires more cognitive overhead. Instead of reading "okay, insert this thing into the hash table if it doesn't have it", I'm reading "okay, get this opaque pointer-that-is-not-a-pointer thing ("entry") via a let-binding, and then call this deref-and-store-that-is-not-a-deref-and-store function on it ("insert") to put this thing into the hash table". This is why I avoid the That all said, I'm not really keen to pursue the bike-shedding further since I don't think we're going to ever come to agreement. Also, it's off-topic. The purpose of the workstream to which this PR belongs is to improve the speed of the Clarity VM. A few comments ago, you mentioned that the speedup you see does not appear to come from the introduction of the |
There were no previous merge conflicts, what I've seen was that the |
|
It looks like it's because I changed |
There was a problem hiding this comment.
I've requested (via Slack) that we try to retain the readability of contains_key via a new trait or method, since these changes make the code less obvious as to what it's doing. This is probably something that's done in several other places throughout stacks-core, so having such a util method would likely be beneficial.
Do you have a suggestion for how to do that? |
Yeah I gave a few to @ASuciuX on Slack -- but if it proves difficult I'm happy to take it. |
…ains-key Track functions using mutants from PR #4483
| match contract_ast.referenced_traits.entry(trait_name.to_owned()) { | ||
| Entry::Occupied(_) => { | ||
| return Err(ParseErrors::NameAlreadyUsed( | ||
| trait_name.to_string(), | ||
| ) | ||
| .into()); | ||
| } | ||
| Entry::Vacant(e) => { | ||
| // Traverse and probe for generics nested in the trait definition | ||
| self.probe_for_generics( | ||
| trait_definition.iter(), | ||
| &mut referenced_traits, | ||
| true, | ||
| )?; | ||
|
|
||
| let trait_id = TraitIdentifier { | ||
| name: trait_name.clone(), | ||
| contract_identifier: contract_ast | ||
| .contract_identifier | ||
| .clone(), | ||
| }; | ||
| e.insert(TraitDefinition::Defined(trait_id)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This could be replaced with a more idiomatic/cleaner approach which reduces nesting while keeping the use of Entry and maintaining readability :)
| match contract_ast.referenced_traits.entry(trait_name.to_owned()) { | |
| Entry::Occupied(_) => { | |
| return Err(ParseErrors::NameAlreadyUsed( | |
| trait_name.to_string(), | |
| ) | |
| .into()); | |
| } | |
| Entry::Vacant(e) => { | |
| // Traverse and probe for generics nested in the trait definition | |
| self.probe_for_generics( | |
| trait_definition.iter(), | |
| &mut referenced_traits, | |
| true, | |
| )?; | |
| let trait_id = TraitIdentifier { | |
| name: trait_name.clone(), | |
| contract_identifier: contract_ast | |
| .contract_identifier | |
| .clone(), | |
| }; | |
| e.insert(TraitDefinition::Defined(trait_id)); | |
| } | |
| } | |
| let entry = contract_ast | |
| .referenced_traits.entry(trait_name.to_owned()); | |
| // If the entry is `Occupied` then the name is already in use. | |
| if let Entry::Occupied(_) = entry { | |
| return Err(ParseErrors::NameAlreadyUsed(trait_name.to_string()).into()); | |
| } | |
| // Traverse and probe for generics nested in the trait definition | |
| self.probe_for_generics( | |
| trait_definition.iter(), | |
| &mut referenced_traits, | |
| true, | |
| )?; | |
| let trait_id = TraitIdentifier { | |
| name: trait_name.clone(), | |
| contract_identifier: contract_ast.contract_identifier.clone(), | |
| }; | |
| // Ensure a value is in the entry | |
| entry.or_insert_with(|| TraitDefinition::Defined(trait_id)); |
There was a problem hiding this comment.
And then if you want to get really fancy you could package all of this in something like the following for re-use (note: just wrote this in playground, so it's not thoroughly tested)
use std::collections::{HashMap, hash_map::{Entry, OccupiedEntry, VacantEntry}};
pub trait EntryExt<'a, K: 'a, V: 'a> where Self: 'a {
fn map_occupied(
&mut self,
f: impl FnOnce(&mut OccupiedEntry<'a, K, V>) -> Result<(), MyError>
) -> Result<&mut Self, MyError>;
fn map_vacant(
&mut self,
f: impl FnOnce(&mut VacantEntry<'a, K, V>) -> Result<(), MyError>
) -> Result<&mut Self, MyError>;
}
impl<'a, K: 'a, V: 'a> EntryExt<'a, K, V> for Entry<'a, K, V> where Self: 'a {
fn map_occupied(
&mut self,
f: impl FnOnce(&mut OccupiedEntry<'a, K, V>) -> Result<(), MyError>
) -> Result<&mut Self, MyError>{
if let Entry::Occupied(e) = self {
f(e)?;
}
Ok(self)
}
fn map_vacant(
&mut self,
f: impl FnOnce(&mut VacantEntry<'a, K, V>) -> Result<(), MyError>
) -> Result<&mut Self, MyError> {
if let Entry::Vacant(e) = self {
f(e)?;
}
Ok(self)
}
}
#[derive(Debug)]
pub enum MyError {
NameAlreadyUsed(String),
}
fn main() -> Result<(), MyError>{
let mut map = HashMap::<&str, &str>::new();
let exists = "asdf";
map.insert(exists, "");
map.entry(exists)
//.map_occupied(|_| Err(MyError::NameAlreadyUsed(exists.to_string())))?
.map_occupied(|_| { // But we'll use this one instead so we get to the next example :)
println!("We're in the occupied entry for 'asdf'");
Ok(())
})?
.map_vacant(|_| {
println!("We're in the vacant entry for 'asdf'");
Ok(())
})?;
let doesnt_exist = "qwerty";
map.entry(doesnt_exist)
.map_occupied(|_| Err(MyError::NameAlreadyUsed(exists.to_string())))?
.map_vacant(|_| {
println!("We're in the vacant entry for 'qwerty'");
Ok(())
})?;
Ok(())
}There was a problem hiding this comment.
One minor nitpick here: We should be consistent with Rust naming conventions and use
mapfor infallible conversionsand_thenfor fallible conversions which return aResult
Otherwise this looks cool and would be a nice addition to the StacksHashMap type
There was a problem hiding this comment.
I'll finish the stacking testing and look into this afterwards. Thank you for adding the trait @cylewitruk
There was a problem hiding this comment.
@ASuciuX here comes an update for the move issue you ran into:
Trait + Impl
pub trait EntryExt<'a, K: 'a, V: 'a, S: 'a>
where
S: BuildHasher,
Self: Sized,
{
fn occupied_and_then<T, F, E>(self, f: F) -> Result<Self, E>
where
F: FnOnce(&OccupiedEntry<'a, K, V, S>) -> Result<T, E>;
fn or_else_try_insert<F, E>(self, default: F) -> Result<&'a mut V, E>
where
F: FnOnce() -> Result<V, E>;
fn vacant_or<E>(self, err: E) -> Result<Self, E>;
}
impl<'a, K: 'a, V: 'a, S: 'a> EntryExt<'a, K, V, S> for Entry<'a, K, V, S>
where
S: BuildHasher,
K: Hash
{
fn occupied_and_then<T, F, E>(self, f: F) -> Result<Self, E>
where
F: FnOnce(&OccupiedEntry<'a, K, V, S>) -> Result<T, E>
{
match self {
Entry::Occupied(ref e) => {
f(e)?;
Ok(self)
},
Entry::Vacant(_) => Ok(self)
}
}
fn or_else_try_insert<F, E>(self, default: F) -> Result<&'a mut V, E>
where
F: FnOnce() -> Result<V, E>
{
match self {
Entry::Occupied(e) => Ok(e.into_mut()),
Entry::Vacant(e) => Ok(e.insert(default()?)),
}
}
fn vacant_or<E>(self, err: E) -> Result<Self, E> {
match self {
Entry::Occupied(_) => Err(err),
Entry::Vacant(_) => Ok(self)
}
}
}Usage
match (&args[0].pre_expr, &args[1].pre_expr) {
(Atom(trait_name), List(trait_definition)) => {
contract_ast.referenced_traits
.entry(trait_name.to_owned())
.vacant_or(ParseErrors::NameAlreadyUsed(trait_name.to_string()))?
.or_else_try_insert(|| {
self.probe_for_generics(
trait_definition.iter(),
&mut referenced_traits,
true,
).map_err(|e| e.err)?;
let trait_id = TraitIdentifier {
name: trait_name.clone(),
contract_identifier: contract_ast
.contract_identifier
.clone(),
};
// Note: I wonder why the compiler can't infer this from the `e.err` above .oO
Ok::<_, ParseErrors>(TraitDefinition::Defined(trait_id))
})?;
}
_ => return Err(ParseErrors::DefineTraitBadSignature.into()),
}There was a problem hiding this comment.
@jcnelson I invite you to give your input on the above as well as you had comments on the original impl.
| let contract_identifier = QualifiedContractIdentifier::new( | ||
| contract_ast.contract_identifier.issuer.clone(), | ||
| contract_name.clone(), | ||
| match contract_ast.referenced_traits.entry(trait_name.to_owned()) { |
- for HashMap - for BTreeMap
|
If there is a more appropriate place for the traits and implements please let me know and I will move them. |
If @cylewitruk can merge his |
I might actually suggest starting a new dir stacks_common/src/ext where all "extension" traits can be placed. This pattern can be applied to a lot of repetitive and/or verbose code.. If |
Benchmark Results
The benchmark above showcase the performance of data for the following:
The best benchmark is the 2nd case, with updated mutable occurrences and default immutable ones.
Applicable issues