Fix bank coalescing #949
Fix bank coalescing #949
Conversation
There was a problem hiding this comment.
Thanks for doing this! This looks like a great fix and a nice optimization to not deserialize spam. Since it would be quite difficult to test the bug you fixed, let's make this code especially readable, and acknowledge those collect() TODOs while we're in the neighborhood. Comments inline.
src/banking_stage.rs
Outdated
| .collect(); //TODO: so many allocs | ||
| res | ||
| }) | ||
| .collect(); //TODO: so many allocs |
There was a problem hiding this comment.
This collect() doesn't look necessary. This module contains use itertools::Itertools;, so you should be able to call chunk() on the iterator. It's possible you'd have to into_iter(), in which case you can hoist the recycling, which wouldn't be so bad.
src/banking_stage.rs
Outdated
| if 0 == vers[i] { | ||
| None | ||
| } else { | ||
| //seems like we should only deserialize verified data |
There was a problem hiding this comment.
I agree, but instead of this comment, can you copy over the comment on the deserialize_transactions() function you deleted?
src/banking_stage.rs
Outdated
| deserialize(&x.data[0..x.meta.size]).ok() | ||
| } | ||
| }) | ||
| .collect(); //TODO: so many allocs |
There was a problem hiding this comment.
The flap_map() documentation suggests you don't need to collect() here. Looks like it'll fuse any type that can be converted into an iterator.
There was a problem hiding this comment.
no method named `chunks` found for type `std::iter::FlatMap<std::slice::Iter<'_, (std:
``
src/banking_stage.rs
Outdated
| debug!("process_transactions"); | ||
| let transactions: Vec<_> = txs.to_vec(); //TODO: so many allocs |
There was a problem hiding this comment.
Go ahead and make process_transactions() a shim that takes/returns a vector and calls a function that takes/returns an Iterator. See to_file_iter in this module for an example. Then call that new process_transactions_iter().
src/banking_stage.rs
Outdated
| debug!("done process_transactions"); | ||
|
|
||
| packet_recycler.recycle(msgs); | ||
| //once processed, results cannot be merged, output must be sent even if its smaller than a blob |
There was a problem hiding this comment.
Can you please update your comment to look like others in this module? They should be complete sentences and begin with a space and capital letter. Also it's "it's".
src/banking_stage.rs
Outdated
|
|
||
| packet_recycler.recycle(msgs); | ||
| //once processed, results cannot be merged, output must be sent even if its smaller than a blob | ||
| let output = results.into_iter().filter_map(|x| x.ok()).collect(); //TODO: so many allocs |
There was a problem hiding this comment.
This collect() cannot be removed. If you agree, can you remove that comment?
src/banking_stage.rs
Outdated
| .collect(); | ||
|
|
||
| let count = mms.iter().map(|x| x.1.len()).sum(); | ||
| let txs: Vec<Transaction> = mms |
There was a problem hiding this comment.
Can you move all this to a function called deserialize_verified_transactions()? It should do everything up to and not including process_transactions().
|
@garious there is a better way to do this thing. So in
|
aeyakovenko
force-pushed
the
fix_bank_coalescing
branch
from
b689c8c to
f0facd4
Compare
once transactions are processed, they can't be coalesced since that will break assumptions that every transaction in an entry can be spent independently.