-
Notifications
You must be signed in to change notification settings - Fork 2.2k
perf(engine): parellelize multiproof_targets_from_state #20669
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
perf(engine): parellelize multiproof_targets_from_state #20669
Conversation
mattsse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, smol suggestions
| .collect(); | ||
|
|
||
| let mut targets = MultiProofTargets::default(); | ||
| let mut storage_targets = 0usize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this equivalent to smth like MultiProofTargets::storage_targets_count?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we don't have that available but we can add it
| // do nothing if unchanged | ||
| if !slot.is_changed() { | ||
| continue | ||
| let results: Vec<_> = state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could collect this directly into B256Map? then we don't need to collect into vec then insert into map?
|
|
||
| /// Returns a set of [`MultiProofTargets`] and the total amount of storage targets, based on the | ||
| /// given state. | ||
| fn multiproof_targets_from_state(state: EvmState) -> (MultiProofTargets, usize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can change this to just return MultiProofTargets and then add a helper fn for counting the targets?
mattsse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smol nit
| impl FromParallelIterator<(B256, B256Set)> for MultiProofTargets { | ||
| fn from_par_iter<I>(par_iter: I) -> Self | ||
| where | ||
| I: IntoParallelIterator<Item = (B256, B256Set)>, | ||
| { | ||
| Self(par_iter.into_par_iter().collect()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great
| // if the account was not touched, or if the account was selfdestructed, do not | ||
| // fetch proofs for it | ||
| // | ||
| // Since selfdestruct can only happen in the same transaction, we can skip | ||
| // prefetching proofs for selfdestructed accounts | ||
| // | ||
| // See: https://eips.ethereum.org/EIPS/eip-6780 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we keep these docs?
…)" This reverts commit 6384226.
same reasoning as this #20635