feat: DatabaseCommit::commit_iter#3197
Conversation
CodSpeed Performance ReportMerging #3197 will not alter performanceComparing Summary
|
| /// Implementors of [`DatabaseCommit`] should override this method when possible for efficiency. | ||
| /// | ||
| /// Callers should prefer using [`DatabaseCommit::commit`] when they already have a [`HashMap`]. | ||
| fn commit_iter(&mut self, changes: impl IntoIterator<Item = (Address, Account)>) { |
There was a problem hiding this comment.
This makes a lot of sense, even for it to become the main required fn in future.
There was a problem hiding this comment.
Yeah I made it this way to avoid breaking changes, but we could absolutely invert the dependency between commit and commit_iter.
There was a problem hiding this comment.
Database/DatabaseCommit are one of oldest traits in repo, it is good that we are not breaking it
rakita
left a comment
rakita
left a comment
There was a problem hiding this comment.
I can't see why test would fail here
crates/database/src/states/cache.rs
Outdated
| pub fn apply_evm_state<'a>( | ||
| &'a mut self, | ||
| evm_state: impl IntoIterator<Item = (Address, Account)> + 'a, | ||
| ) -> impl Iterator<Item = (Address, TransitionAccount)> + 'a { |
There was a problem hiding this comment.
Hm is this maybe a problem. If we don't collect/iterate over returned Iterator, would the changes still be applied?
There was a problem hiding this comment.
lol, it is: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=30b3aaa62884a42cf12eaf5f99262a5c
Lets return vec here, it will get optimized by compiler, and returning a iterator is a footgun.
Would additionally add #[inline] as a hint.
There was a problem hiding this comment.
I don't think the collection into Vec will be optimized by the compiler, but this also isn't really a huge deal.
If we don't collect/iterate over returned Iterator, would the changes still be applied?
Ah you're right. This could be a foot gun, although I do think the caller would get a clippy lint for the unused iterator. I'll revert this though, thanks for the feedback!
| impl<DB: Database> DatabaseCommit for State<DB> { | ||
| fn commit(&mut self, evm_state: HashMap<Address, Account>) { | ||
| let transitions = self.cache.apply_evm_state(evm_state); | ||
| self.apply_transition(transitions); | ||
| fn commit(&mut self, changes: HashMap<Address, Account>) { | ||
| let transitions = self.cache.apply_evm_state(changes); | ||
| if let Some(s) = self.transition_state.as_mut() { | ||
| s.add_transitions(transitions) | ||
| } | ||
| } | ||
|
|
||
| fn commit_iter(&mut self, changes: impl IntoIterator<Item = (Address, Account)>) { | ||
| let transitions = self.cache.apply_evm_state(changes); | ||
| if let Some(s) = self.transition_state.as_mut() { | ||
| s.add_transitions(transitions) | ||
| } |
There was a problem hiding this comment.
Since HashMap implements IntoIterator, commit can just delegate to commit_iter..I believe
fn commit(&mut self, changes: HashMap<Address, Account>) {
self.commit_iter(changes)
}this removes redundancy and avoids future updates in multiple locations if if the transition logic ever changes ofc
There was a problem hiding this comment.
Well either commit depends on commit_iter or vice versa haha. We can't have both I chose my way because it doesn't have breaking changes.
3 participants
closes #3196
is overly restrictive and forces additional computation by constructing a
HashMap. I suggest adding an additional overridable trait method for use cases that don't require aHashMap.This PR provides a way around reconstructing a HashMap. Additionally I've overridden the implementation for State which should give a bit of added efficiency.
I have included a breaking change by returning an
Iteratorinapply_evm_state. This allows passing through an iterator rather than unnecessarily collecting into a Vec which should be more efficient. If we don't want a breaking change then I'm happy to revert just that part.