Less cloning when importing blocks#11531
Conversation
| /// | ||
| /// If valid, it will be executed, and archived together with the receipt. | ||
| pub fn push_transaction(&mut self, t: SignedTransaction) -> Result<&Receipt, Error> { | ||
| pub fn push_transaction(&mut self, t: &SignedTransaction) -> Result<&Receipt, Error> { |
There was a problem hiding this comment.
The rationale behind this change?
The transaction is still cloned if the else branch is taken, are you doing this because of saving an allocation if the if branch is taken?
Not sure how this plays along w.r.t. to speculative execution on modern CPUs
There was a problem hiding this comment.
Wait, which else-branch do you mean?
There was a problem hiding this comment.
I meant the implicit else branch, i.e if https://github.com/OpenEthereum/open-ethereum/pull/11531/files#diff-e2e171033e005c3b329818aaff213767R172-R174 is missed
There was a problem hiding this comment.
I see, well there's that also, but mainly passing the tx by ref is fallout from a borrow from higher up in the call chain, in enact_verified(). (I was also unsure wth the into() was doing).
There was a problem hiding this comment.
The into() was not doing anything
There was a problem hiding this comment.
I'm also not sure why we need this change, if anything, it makes more allocations.
There was a problem hiding this comment.
So in import_verified_blocks() we call check_and_lock_block() passing in an owned PreverifiedBlock, so to make the rest of the code work there we do some cloning that didn't seem necessary and that's where this PR started.
When we call enact_verified() we move the block but what is actually needed for enact() is actually the header, transactions and the (max) 2 uncles.
What I'd really would like to do is have a way to split up the PreverifiedBlock into (owned) components and be able to move them independently. I don't know of a way to do that, and that's why I ended up with this: it avoids cloning the block bytes in import_verified_blocks() and a few Header structs, and passes references around instead. Maybe this mean more clones, as each transaction that is added to the block is cloned, but they are also smaller (I hope).
There was a problem hiding this comment.
I guess you could use some mem::take tricks, for example, replace Bytes with an empty Vec.
But, why can't you take PreverifiedBlock by value instead in enact? As I did here: https://github.com/OpenEthereum/open-ethereum/compare/na-ethcore-blockerror#diff-e2e171033e005c3b329818aaff213767R411-R564
I'm not saying that it is elegant but it worked for me at least :)
There was a problem hiding this comment.
This whole thing reminds me of something :P
The real problem is that programmers have spent far too much time worrying about efficiency in the wrong places and at the wrong times; premature optimization is the root of all evil (or at least most of it) in programming.”
Seriously though,
What I'd really would like to do is have a way to split up the PreverifiedBlock into (owned) components and be able to move them independently. I don't know of a way to do that
create a type for parts or pass them directly instead of PreferifiedBlock? or what Niklas suggested
There was a problem hiding this comment.
Ok, thank you for the input, much appreciated. At the end I went with mem::take()-ing the block RLP bytes and after some further shuffling things around I think I have something decent.
| let (imported_blocks, import_results, invalid_blocks, imported, duration, has_more_blocks_to_import) = { | ||
| let mut imported_blocks = Vec::with_capacity(max_blocks_to_import); | ||
| let mut invalid_blocks = HashSet::new(); | ||
| let proposed_blocks = Vec::with_capacity(max_blocks_to_import); |
There was a problem hiding this comment.
this was actually not ever used? Nice catch 👍
dvdplm
changed the title
Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Co-Authored-By: Andronik Ordian <write@reusable.software>
| continue; | ||
| } | ||
|
|
||
| let block_bytes = std::mem::take(&mut block.bytes); |
There was a problem hiding this comment.
Taking the RLP here, so I can move the block in the next line.
|
|
||
| let enact_result = enact_verified( | ||
| block, | ||
| let enact_result = enact( |
There was a problem hiding this comment.
Calling enact() directly here lets me avoid splitting up the PreverifiedBlock before making the call (and pass the Header by ref).
There was a problem hiding this comment.
I guess enact_verified added a bit of type safety to enact. Another approach would be to keep that function, but change the return type to return unused but consumed fields of PreverifiedBlock, something like EthcoreResult<(LockedBlock, Option<_>, Bytes)> or (EthcoreResult<_>, Bytes).
There was a problem hiding this comment.
Yeah I tried that, and Niklas did too I think. It gets pretty nasty and I'd rather not. It feels like a dirty trick to hand in data and ask for it back.
There was a problem hiding this comment.
Yeah, it gets messy because then we need to return Bytes in both the Ok and Err case because of this damn BadBlocks::report
There was a problem hiding this comment.
Ok, we can merge the PR as is, I can take a look if it could be cleaned up.
| } | ||
|
|
||
| let block_bytes = std::mem::take(&mut block.bytes); | ||
| match self.check_and_lock_block(block, client) { |
There was a problem hiding this comment.
Hmm, this will call check_and_lock_block with empty block bytes.
Then we must ensure that it and all the functions it calls don't use block bytes, have you done it?
Well, I propose to create another type PreverifiedBlockWithoutBytes or something.
If that's not possible add a big fat note in fn check_and_lock_block which explains that block bytes is empty.
There was a problem hiding this comment.
Then we must ensure that it and all the functions it calls don't use
block bytes, have you done it?
I have yes. It is not used lower down in the call graph. I'll add a big comment about it.
Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>
niklasad1
added
A8-looksgood 🦄
* master: Code cleanup in the sync module (#11552) initial cleanup (#11542) Warn if genesis constructor revert (#11550) ethcore: cleanup after #11531 (#11546) license update (#11543) Less cloning when importing blocks (#11531) Github Actions (#11528) Fix Alpine Dockerfile (#11538) Remove AuxiliaryData/AuxiliaryRequest (#11533) [journaldb]: cleanup (#11534) Remove references to parity-ethereum (#11525) Drop IPFS support (#11532) chain-supplier: fix warning reporting for GetNodeData request (#11530) Faster kill_garbage (#11514) [EngineSigner]: don't sign message with only zeroes (#11524)
4 participants
Try to reduce the number of clones in general and of the block raw bytes in particular. Block RLP is typically 15 - 40kb in size so I think it makes sense to keep the number of clones we make to a minimum. Another aspect of unnecessary cloning is that it obfuscates the intention of the code making it harder to read and follow; to address this this PR tries to move the clones that are necessary to the end of the processing flow.
During the work I noiticed that the
AuxiliaryDataandAuxiliaryRequesttypes were not necessary. I extracted their removal to #11533. It is probably best to review that PR first.UPDATE I used the
alloc_countercrate to instrument importing the exact same 60 blocks with and without the changes in this PR. The good news that yes, with this PR we do allocate less often. The bad news is that the change is minscule: 2 177 227 (master) vs 2 176 794 (branch); something somewhere is allocating a lot.Master:
This branch: