optimization: Moved repeated `-printpriority` fetching out of AddToBlock

paplorinc commented at 1:17 pm on June 23, 2024: contributor

AddToBlock was called repeatedly from addPackageTxs where the constant value of printpriority is recalculated every time.

This showed up during profiling of AssembleBlock, fetching it once in the constructor results in a small speed increase for many iterations.

./src/bench/bench_bitcoin –filter=‘AssembleBlock’ –min-time=10000

before:

ns/op	op/s	err%	total	benchmark
156,460.15	6,391.40	0.1%	11.03	`AssembleBlock`

after:

ns/op	op/s	err%	total	benchmark
149,289.55	6,698.39	0.3%	10.97	`AssembleBlock`

The slight speedup shows up in CI as well:

DrahtBot commented at 1:17 pm on June 23, 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	tdb3, furszy, maflcko, achow101

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:

#30356 (refactor: add coinbase constraints to BlockAssembler::Options by Sjors)
#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
#29432 (Stratum v2 Template Provider (take 3) by Sjors)

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 marked this as ready for review on Jun 23, 2024

paplorinc renamed this:
~~Moved repeated `-printpriority` fetching out of AddToBlock~~
Optimization: Moved repeated `-printpriority` fetching out of AddToBlock
on Jun 23, 2024

paplorinc renamed this:
~~Optimization: Moved repeated `-printpriority` fetching out of AddToBlock~~
optimization: Moved repeated `-printpriority` fetching out of AddToBlock
on Jun 23, 2024

tdb3 approved

tdb3 commented at 2:10 am on June 25, 2024: contributor

ACK 6e519b83188e77c401bb9024196c499bee67da68

Thanks for optimizing. Saw around a 6% speedup on my machine.

in src/node/miner.h:175 in 6e519b8318 outdated

171@@ -172,6 +172,7 @@ class BlockAssembler
172 
173 private:
174     const Options m_options;
175+    const bool m_fPrintPriority;

maflcko commented at 8:10 am on June 25, 2024:

This doesn’t print the priority (it no longer exists in this codebase and was removed years ago). It prints the modified fee. So the name should reflect that. Also, snake_case for new code.

paplorinc commented at 11:48 am on June 25, 2024:

The commit by @morcos indicates there’s still use for the print statement - even though it wasn’t used correctly in the tests, I’ve removed that reference and renamed the variable (leaving the -printpriority option, adding comment for the behavior change)

maflcko commented at 8:11 am on June 25, 2024: member

Not sure if this is used at all. Maybe it can be removed completely? Otherwise, see the feedback.

paplorinc force-pushed on Jun 25, 2024

in test/functional/mining_prioritisetransaction.py:27 in 3c0b79555c outdated

23@@ -24,10 +24,7 @@
24 class PrioritiseTransactionTest(BitcoinTestFramework):
25     def set_test_params(self):
26         self.num_nodes = 1
27-        self.extra_args = [[
28-            "-printpriority=1",
29-            "-datacarriersize=100000",
30-        ]] * self.num_nodes
31+        self.extra_args = [["-datacarriersize=100000"]] * self.num_nodes

maflcko commented at 12:19 pm on June 25, 2024:

See “Lost baseline coverage " in https://corecheck.dev/bitcoin/bitcoin/pulls/30324. So this seems wrong

paplorinc commented at 12:49 pm on June 25, 2024:

My bad, I assumed that since nothing was printed it was omitted, but

0    if (m_print_modified_fee) {
1        throw std::runtime_error("m_print_modified_fee is not implemented");
2    }

fails - thanks for pointing it out.

paplorinc force-pushed on Jun 25, 2024

in src/node/miner.h:175 in 71753aca85 outdated

171@@ -171,6 +172,8 @@ class BlockAssembler
172 
173 private:
174     const Options m_options;
175+    const bool m_print_modified_fee = gArgs.GetBoolArg("-printpriority", DEFAULT_PRINTPRIORITY); // -printpriority behavior was changed in 400b15147cf1c4757927935222611e8d3481e739

fanquake commented at 1:00 pm on June 25, 2024:

I don’t think you need to add this comment.

paplorinc commented at 1:03 pm on June 25, 2024:

Removed, left it only in the commit message

paplorinc force-pushed on Jun 25, 2024

in src/node/miner.cpp:412 in a114ced719 outdated

410-            AddToBlock(sortedEntries[i]);
411+        for (const auto& entry : sortedEntries) {
412+            AddToBlock(entry);
413             // Erase from the modified set, if present
414-            mapModifiedTx.erase(sortedEntries[i]);
415+            mapModifiedTx.erase(entry);

maflcko commented at 9:53 am on June 28, 2024:

NACK on refactoring code that will be removed anyway in the future. The change has no benefit and is causing merge conflicts and hassle down the line.

paplorinc commented at 12:34 pm on June 28, 2024:

Makes sense, removed

paplorinc requested review from tdb3 on Jun 28, 2024

paplorinc requested review from maflcko on Jun 28, 2024

paplorinc requested review from fanquake on Jun 28, 2024

paplorinc force-pushed on Jun 28, 2024

tdb3 approved

tdb3 commented at 7:59 pm on June 30, 2024: contributor

re ACK 7759cd87ccf16e1de8f385924daf4d27afd94464

Tested that the arg still worked properly after the change. The message was missing without -printpriority=1. The message was present with it.

0src/bitcoind -debug -regtest -loglevel=trace -printpriority=1
1src/bitcoin-cli -regtest createwallet test
2src/bitcoin-cli -regtest -generate 101
3src/bitcoin-cli -regtest -named sendtoaddress address=bcrt1qcv96frlnhzddst4dnlaa9nvdmvc4darw335wr9 fee_rate=3 amount=2
4src/bitcoin-cli -regtest -named sendtoaddress address=bcrt1qnqyyh5xr73lkhfr9tj47u8kevtnahkgrqrnd4s fee_rate=2 amount=6
5src/bitcoin-cli getblocktemplate '{"rules": ["segwit"]}'

02024-06-30T19:43:14Z fee rate 0.00003000 BTC/kvB txid e4bf26ea72e8cbbef9a6e239d1e57fd141c298dbe48696421b753794e20e9922
12024-06-30T19:43:14Z fee rate 0.00002000 BTC/kvB txid 98837b4e4d02399205cbc4e1d21266229b43186fd682e9461a915bd42c1de735

Still showing a speedup. Measured about 5%.

in src/node/miner.h:175 in 7759cd87cc outdated

171@@ -171,6 +172,8 @@ class BlockAssembler
172 
173 private:
174     const Options m_options;
175+    const bool m_print_modified_fee = gArgs.GetBoolArg("-printpriority", DEFAULT_PRINTPRIORITY);

furszy commented at 8:38 pm on June 30, 2024:

By doing this, you include common/args.h in all files that include node/miner.h. It is better to add the field to the BlockAssembler::Options struct and parse it inside ApplyArgsManOptions.

paplorinc commented at 9:15 pm on June 30, 2024:

Thanks a lot, it’s so much better this way!

Moved the repeated -printpriority fetching out of AddToBlock

AddToBlock was called repeatedly from `addPackageTxs` where the constant value of `printpriority` is recalculated every time.
Since its behavior was changed in 400b151, I've named the variable accordingly.

This showed up during profiling of AssembleBlock, fetching it once in the constructor results in a measurable speed increase for many iterations.

> ./src/bench/bench_bitcoin --filter='AssembleBlock' --min-time=1000

before:
|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|          155,558.97 |            6,428.43 |    0.1% |      1.10 | `AssembleBlock`

after:
|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|          148,083.68 |            6,752.94 |    0.1% |      1.10 | `AssembleBlock`

Co-authored-by: furszy <mfurszy@protonmail.com>

323ce30308

paplorinc force-pushed on Jun 30, 2024

paplorinc commented at 9:16 pm on June 30, 2024: contributor

Thanks a lot for your help @tdb3, appreciate your reviews and verifications!

tdb3 approved

tdb3 commented at 9:56 pm on June 30, 2024: contributor

re ACK 323ce303086d42292ed9fe7c98e8b459bdf6d1a6

Re-ran same tests in #30324#pullrequestreview-2150128035

same results.

in src/node/miner.h:33 in 323ce30308

29@@ -30,7 +30,7 @@ class ChainstateManager;
30 namespace Consensus { struct Params; };
31 
32 namespace node {
33-static const bool DEFAULT_PRINTPRIORITY = false;
34+static const bool DEFAULT_PRINT_MODIFIED_FEE = false;

furszy commented at 10:16 pm on June 30, 2024:

nano nit: would call it “DEFAULT_PRINT_FEERATE” as the “modified” word is internal and might change over time.

paplorinc commented at 8:20 am on July 1, 2024:

I didn’t change the external option, just the internal name - as requested here #30324 (review)

furszy commented at 10:16 pm on June 30, 2024: member

utACK 323ce303086

maflcko commented at 7:35 pm on July 2, 2024: member

ACK 323ce303086d42292ed9fe7c98e8b459bdf6d1a6

achow101 commented at 8:36 pm on July 2, 2024: member

ACK 323ce303086d42292ed9fe7c98e8b459bdf6d1a6

achow101 merged this on Jul 2, 2024

achow101 closed this on Jul 2, 2024

optimization: Moved repeated -printpriority fetching out of AddToBlock #30324

Code Coverage

Reviews

Conflicts

optimization: Moved repeated `-printpriority` fetching out of AddToBlock #30324