optimization: reserve memory allocation for transaction inputs/outputs

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

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:

0|               ns/op |                op/s |    err% |     total | benchmark
1|--------------------:|--------------------:|--------:|----------:|:----------
2|            2,089.91 |          478,489.15 |    0.1% |      1.10 | `BenchmarkAddOutputsEmplaceBackReserve`
3|            1,257.81 |          795,031.71 |    0.1% |      1.10 | `BenchmarkAddOutputsInPlaceEmplaceBackReserve`
4|            2,594.12 |          385,487.12 |    0.2% |      1.10 | `BenchmarkAddOutputsPushBack`
5|            1,667.85 |          599,573.52 |    0.1% |      1.10 | `BenchmarkAddOutputsPushBackMoveReserve`
6|            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

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

Zidane115 commented at 4:12 pm on October 19, 2024: none

Wwe

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

Code Coverage

Reviews

Conflicts