optimization: reserve memory allocation for transaction inputs/outputs #30093

pull l0rinc wants to merge 2 commits into bitcoin:master from l0rinc:paplorinc/spend_preallocation changing 1 files +2 −0

l0rinc commented at 9:53 AM on May 13, 2024: contributor

Reserved memory for the transaction inputs and outputs.

Split out of https://github.com/bitcoin/bitcoin/pull/30050/files#r1597631104
DrahtBot commented at 9:54 AM on May 13, 2024: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK TheCharlatan, stickies-v, achow101

Concept ACK josibake

Stale ACK theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #24128 (wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs by maflcko)
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.
l0rinc renamed this:
~~Optimize memory allocation for transaction outputs~~
refactor: optimize memory allocation for transaction outputs
on May 13, 2024
DrahtBot added the label Refactoring on May 13, 2024
l0rinc renamed this:
~~refactor: optimize memory allocation for transaction outputs~~
refactor: reserve memory allocation for transaction outputs
on May 13, 2024
in src/wallet/spend.cpp:1091 in aaed2db4f4 outdated
```
1087 | @@ -1088,6 +1088,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1088 |      coin_selection_params.tx_noinputs_size = 10 + GetSizeOfCompactSize(vecSend.size()); // bytes for output count
1089 |  
1090 |      // vouts to the payees
1091 | +    txNew.vout.reserve(txNew.vout.size() + vecSend.size());
```
theuni commented at 4:27 PM on May 13, 2024:

A few suggestions:

Looks to me like txNew.vout is always empty here, so I'm not sure why we'd bother to add txNew.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()+1 just in case.

I think the txouts could be std::moved (in both places) instead to save a copy similar to #30094?

Any reason not .reserve() for the txNew.vin as well?
l0rinc commented at 8:12 PM on May 13, 2024:

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 to emplace_back instead, 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!
theuni commented at 4:27 PM on May 13, 2024: member

Concept ACK
l0rinc force-pushed on May 13, 2024

Type	Reviewers
ACK	TheCharlatan, stickies-v, achow101
Concept ACK	josibake
Stale ACK	theuni

in src/wallet/spend.cpp:1102 in a1c3d58795 outdated

1098 | @@ -1098,7 +1099,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1099 |          if (IsDust(txout, wallet.chain().relayDustFee())) {
1100 |              return util::Error{_("Transaction amount too small")};
1101 |          }
1102 | -        txNew.vout.push_back(txout);
1103 | +        txNew.vout.emplace_back(txout);

theuni commented at 8:46 PM on May 13, 2024:

This will just be forwarded to the copy ctor. push_back(std::move(txout)) would be more correct (and clear).

l0rinc commented at 8:02 AM on May 14, 2024:

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?

l0rinc force-pushed on May 14, 2024
l0rinc force-pushed on May 14, 2024
l0rinc force-pushed on May 14, 2024
DrahtBot added the label CI failed on May 16, 2024
l0rinc force-pushed on May 16, 2024
DrahtBot removed the label CI failed on May 16, 2024
l0rinc force-pushed on May 29, 2024
josibake commented at 9:43 AM on June 10, 2024: member

Concept ACK

I noticed you have benchmarks, would be nice to have the comparison of old vs new in the PR description.
l0rinc commented at 4:25 PM on June 10, 2024: contributor

Thanks for the review @josibake, I've added the conclusions of the benchmarks to the commit message (but copied it to the PR description as well now), can you please check if https://github.com/bitcoin/bitcoin/pull/30093/commits/ebf8667f634653a2eebe404469bfb62c5bb3add2 answers your questions? @theuni, appreciate your previous reviews and insights, please take a look at the changes I did since.
theuni commented at 2:47 PM on June 11, 2024: member

Changes look good. The bench is not really useful though, because it's testing things that aren't in our code.

I believe @josibake was asking for a bench that demonstrates a before/after of CreateTransactionInternal. I'm guessing that's not really feasible though, so I think it's enough to use your bench numbers without actually committing it.
l0rinc force-pushed on Jun 11, 2024
l0rinc commented at 3:01 PM on June 11, 2024: contributor

Moved the benchmark out to https://gist.github.com/paplorinc/812007eef71d5285be0654375ea3e03e

in src/wallet/spend.cpp:1094 in a5b91758e7 outdated

1090 | @@ -1091,15 +1091,14 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1091 |      txNew.vout.reserve(vecSend.size() + 1); // accommodate the possible later inserts
1092 |      for (const auto& recipient : vecSend)
1093 |      {
1094 | -        CTxOut txout(recipient.nAmount, GetScriptForDestination(recipient.dest));
1095 | +        auto& txout = txNew.vout.emplace_back(recipient.nAmount, GetScriptForDestination(recipient.dest));

theuni commented at 3:10 PM on June 11, 2024:

One last nit: because it's not modified after creation, const auto& txout

l0rinc commented at 3:12 PM on June 11, 2024:

Done, thanks

l0rinc force-pushed on Jun 11, 2024
theuni approved
theuni commented at 4:40 PM on June 11, 2024: member

utACK db2a31c339ad697f1828cde162f4df2231c9b0f1
DrahtBot requested review from josibake on Jun 11, 2024
l0rinc renamed this:
~~refactor: reserve memory allocation for transaction outputs~~
refactor: reserve memory allocation for transaction inputs/outputs
on Jun 13, 2024
l0rinc renamed this:
~~refactor: reserve memory allocation for transaction inputs/outputs~~
Optimization: reserve memory allocation for transaction inputs/outputs
on Jun 23, 2024
l0rinc renamed this:
~~Optimization: reserve memory allocation for transaction inputs/outputs~~
optimization: reserve memory allocation for transaction inputs/outputs
on Jun 23, 2024
DrahtBot added the label Needs rebase on Jun 27, 2024
l0rinc force-pushed on Jun 27, 2024

Reserve space for transaction outputs in CreateTransactionInternal

Accommodating possible later insert as well

Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>

c76aaaf900

Reserve space for transaction inputs in CreateTransactionInternal
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
ec585f11c3
l0rinc force-pushed on Jun 27, 2024
DrahtBot removed the label Needs rebase on Jun 27, 2024
TheCharlatan approved
TheCharlatan commented at 8:07 AM on August 15, 2024: contributor

ACK ec585f11c38bbe277a596dcea3c813e7c6626050
DrahtBot requested review from theuni on Aug 15, 2024
achow101 requested review from achow101 on Oct 15, 2024
stickies-v approved
stickies-v commented at 3:35 PM on October 15, 2024: contributor

ACK ec585f11c38bbe277a596dcea3c813e7c6626050
achow101 commented at 12:37 PM on October 16, 2024: member

ACK ec585f11c38bbe277a596dcea3c813e7c6626050
achow101 merged this on Oct 16, 2024
achow101 closed this on Oct 16, 2024
l0rinc deleted the branch on Oct 16, 2024
TheCharlatan referenced this in commit 8bb47d4c2c on Nov 2, 2024
achow101 referenced this in commit 5a4bc5c036 on Nov 26, 2024
bug-castercv502 referenced this in commit 403bebd591 on Sep 28, 2025
bitcoin locked this on Oct 19, 2025