Make a loop instead of using filter #2071
Conversation
|
Thanks to @ilya-bobyr for suggesting this refactoring. |
| .collect_vec(); | ||
| pubkeys_by_stake | ||
| .into_iter() | ||
| .filter(|&pubkey| { | ||
| if !continue_forwarding { | ||
| return false; | ||
| } | ||
| if let Some(lock) = self.get_entry(pubkey) { | ||
| let mut vote = lock.write().unwrap(); | ||
| if !vote.is_vote_taken() && !vote.is_forwarded() { | ||
| let deserialized_vote_packet = vote.vote.as_ref().unwrap().clone(); | ||
| if let Some(sanitized_vote_transaction) = deserialized_vote_packet | ||
| .build_sanitized_transaction( | ||
| &bank.feature_set, | ||
| bank.vote_only_bank(), | ||
| bank.as_ref(), | ||
| ) | ||
| { | ||
| if forward_packet_batches_by_accounts.try_add_packet( | ||
| &sanitized_vote_transaction, | ||
| deserialized_vote_packet, | ||
| &bank.feature_set, | ||
| ) { | ||
| vote.forwarded = true; | ||
| } else { | ||
| // To match behavior of regular transactions we stop | ||
| // forwarding votes as soon as one fails | ||
| continue_forwarding = false; | ||
| } | ||
| return true; | ||
| let mut count: usize = 0; | ||
| for pubkey in pubkeys_by_stake { |
There was a problem hiding this comment.
for statement should handle an iterator just fine.
Removing a need for a vector to be constructed.
- .collect_vec();
let mut count: usize = 0;
for pubkey in pubkeys_by_stake {There was a problem hiding this comment.
There was a lifetime issue with the pubkeys_by_stake. Hoisting self.latest_votes_per_pubkey.read().unwrap(); out of the call in 93c3e75 fixed it.
There was a problem hiding this comment.
Yeah, I figured there might be an issue with lifetimes there.
But, I think, it is because the function type is not correctly inferred.
The returned iterator does not hold any references to the input data, so there should be no lifetimes it is capturing.
Try specifying explicitly that the returned object does not capture any lifetimes:
pub(crate) fn weighted_random_order_by_stake<'a>(
bank: &Bank,
pubkeys: impl Iterator<Item = &'a Pubkey>,
) -> impl Iterator<Item = Pubkey> + 'static {I think 'a gets added to the returned type for some reason, as if you said
... -> impl Iterator<Item = Pubkey> + 'a {There was a problem hiding this comment.
At the same time, I think, it might be possible to reduce the number of copies, if it would actually return Item = &'a Pubkey.
In which case you would need to put binding on the stack and keep is there for the whole duration of the algorithm.
As the returned iterator would reference binding.
Depending on the lock contention, one or the other might be faster.
| // To match behavior of regular transactions we stop | ||
| // forwarding votes as soon as one fails. We are assuming that failure (try_add_packet) means no more space available. |
There was a problem hiding this comment.
Convention is to wrap on 100 characters for comments.
| // To match behavior of regular transactions we stop | |
| // forwarding votes as soon as one fails. We are assuming that failure (try_add_packet) means no more space available. | |
| // To match behavior of regular transactions we stop forwarding votes as | |
| // soon as one fails. We are assuming that failure (try_add_packet) | |
| // means no more space available. |
| let binding = self.latest_votes_per_pubkey.read().unwrap(); | ||
| let pubkeys_by_stake = weighted_random_order_by_stake(&bank, binding.keys()); |
There was a problem hiding this comment.
I think this is locking the latest_votes_per_pubkey for longer than necessary.
If the following compiles, it is a slightly better way to do it, as it releases the lock earlier:
| let binding = self.latest_votes_per_pubkey.read().unwrap(); | |
| let pubkeys_by_stake = weighted_random_order_by_stake(&bank, binding.keys()); | |
| let pubkeys_by_stake = { | |
| let binding = self.latest_votes_per_pubkey.read().unwrap(); | |
| weighted_random_order_by_stake(&bank, binding.keys()) | |
| }; |
There was a problem hiding this comment.
doesn't compile because error: binding does not live long enough
There was a problem hiding this comment.
Thanks! I'll abandon this patch then.
Simplifies the code by avoiding redundant iteration, and collection.
|
Abandoning it in favor of #2091 |
Simplifies the code by avoiding redundant iteration, and collection.