refactor: reserve memory allocation for transaction outputs #30093
+121
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
paplorinc
changed the title
paplorinc
changed the title
theuni
reviewed
May 13, 2024
src/wallet/spend.cpp
Outdated
| @@ -1088,6 +1088,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( | |||
| coin_selection_params.tx_noinputs_size = 10 + GetSizeOfCompactSize(vecSend.size()); // bytes for output count | |||
|
|
|||
| // vouts to the payees | |||
| txNew.vout.reserve(txNew.vout.size() + vecSend.size()); | |||
There was a problem hiding this comment.
A few suggestions:
- Looks to me like
txNew.voutis always empty here, so I'm not sure why we'd bother to addtxNew.vout.size()unless I'm missing something ? - There's an additional likely insert for change. Since the reserved size here is exact, that could trigger a reallocation up to the next power-of-two. Makes sense to make this
vecSend.size()+1just in case. - I think the txouts could be
std::moved (in both places) instead to save a copy similar to rpc: move UniValue in blockToJSON #30094? - Any reason not
.reserve()for thetxNew.vinas well?
There was a problem hiding this comment.
Hey @theuni, excellent review, thanks!
- yes, it's always empty, leaving it out is indeed more readable - and if people modify it in the future, they should just pay attention to this part
- nice, haven't noticed it, added the +1 (added you as co-author)
- To be consistent with
txNew.vin, I've changed it toemplace_backinstead, the performance should be comparable - do you agree? - wanted to be focused only on the issue discovered during code review, but this is much better, thanks for the comment, pushed!
paplorinc
force-pushed
the
paplorinc/spend_preallocation
branch
from
aaed2db
to
a1c3d58
Compare
theuni
reviewed
May 13, 2024
src/wallet/spend.cpp
Outdated
| @@ -1098,7 +1099,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( | |||
| if (IsDust(txout, wallet.chain().relayDustFee())) { | |||
| return util::Error{_("Transaction amount too small")}; | |||
| } | |||
| txNew.vout.push_back(txout); | |||
| txNew.vout.emplace_back(txout); | |||
There was a problem hiding this comment.
This will just be forwarded to the copy ctor. push_back(std::move(txout)) would be more correct (and clear).
There was a problem hiding this comment.
I've added benchmarks to verify:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 2,089.91 | 478,489.15 | 0.1% | 1.10 | `BenchmarkAddOutputsEmplaceBackReserve`
| 1,257.81 | 795,031.71 | 0.1% | 1.10 | `BenchmarkAddOutputsInPlaceEmplaceBackReserve`
| 2,594.12 | 385,487.12 | 0.2% | 1.10 | `BenchmarkAddOutputsPushBack`
| 1,667.85 | 599,573.52 | 0.1% | 1.10 | `BenchmarkAddOutputsPushBackMoveReserve`
| 2,089.21 | 478,649.26 | 0.1% | 1.10 | `BenchmarkAddOutputsPushBackReserve`
ended up adding it directly to txNew.vout which is both faster and simpler - what do you think?
paplorinc
force-pushed
the
paplorinc/spend_preallocation
branch
3 times, most recently
from
794db03
to
1cdecda
Compare
Accommodating possible later insert as well Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
Benchmark results show that in place `emplace_back` outperforms constructing `CTxOut` and then `push_back` or `emplace_back` and even `push_back` with `move`. This simplifies and optimizes the code at the same time. > make && ./src/bench/bench_bitcoin --filter='BenchmarkAddOutputsPushBack|BenchmarkAddOutputsPushBackReserve|BenchmarkAddOutputsPushBackMoveReserve|BenchmarkAddOutputsEmplaceBackReserve|BenchmarkAddOutputsInPlaceEmplaceBackReserve' --min-time=1000 ``` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 2,089.91 | 478,489.15 | 0.1% | 1.10 | `BenchmarkAddOutputsEmplaceBackReserve` | 1,257.81 | 795,031.71 | 0.1% | 1.10 | `BenchmarkAddOutputsInPlaceEmplaceBackReserve` | 2,594.12 | 385,487.12 | 0.2% | 1.10 | `BenchmarkAddOutputsPushBack` | 1,667.85 | 599,573.52 | 0.1% | 1.10 | `BenchmarkAddOutputsPushBackMoveReserve` | 2,089.21 | 478,649.26 | 0.1% | 1.10 | `BenchmarkAddOutputsPushBackReserve` ``` Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
paplorinc
force-pushed
the
paplorinc/spend_preallocation
branch
from
1cdecda
to
d23be15
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reserved memory for the transaction inputs and outputs.
Split out of https://github.com/bitcoin/bitcoin/pull/30050/files#r1597631104