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

l0rinc · 2024-06-23T13:17:35Z

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 · 2024-06-23T13:17:37Z

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.

tdb3

ACK 6e519b8

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

maflcko

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

maflcko · 2024-06-25T08:10:33Z

src/node/miner.h

@@ -172,6 +172,7 @@ class BlockAssembler

 private:
    const Options m_options;
+    const bool m_fPrintPriority;


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.

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)

test/functional/mining_prioritisetransaction.py

src/node/miner.h

src/node/miner.cpp

tdb3

re ACK 7759cd8

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

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

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

Still showing a speedup. Measured about 5%.

furszy · 2024-06-30T20:38:06Z

src/node/miner.h

@@ -171,6 +172,8 @@ class BlockAssembler

 private:
    const Options m_options;
+    const bool m_print_modified_fee = gArgs.GetBoolArg("-printpriority", DEFAULT_PRINTPRIORITY);


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.

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

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>

l0rinc · 2024-06-30T21:16:55Z

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

tdb3

re ACK 323ce30

Re-ran same tests in
#30324 (review)

same results.

furszy

utACK 323ce30

furszy · 2024-06-30T22:16:38Z

src/node/miner.h

@@ -30,7 +30,7 @@ class ChainstateManager;
 namespace Consensus { struct Params; };

 namespace node {
-static const bool DEFAULT_PRINTPRIORITY = false;
+static const bool DEFAULT_PRINT_MODIFIED_FEE = false;


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

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

maflcko · 2024-07-02T19:35:34Z

ACK 323ce30

achow101 · 2024-07-02T20:36:07Z

ACK 323ce30

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> Github-Pull: bitcoin#30324 Rebased-From: 323ce30

l0rinc marked this pull request as ready for review June 23, 2024 16:07

l0rinc changed the title ~~Moved repeated -printpriority fetching out of AddToBlock~~ Optimization: Moved repeated -printpriority fetching out of AddToBlock Jun 23, 2024

l0rinc changed the title ~~Optimization: Moved repeated -printpriority fetching out of AddToBlock~~ optimization: Moved repeated -printpriority fetching out of AddToBlock Jun 23, 2024

DrahtBot mentioned this pull request Jun 24, 2024

scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] #29641

Draft

tdb3 approved these changes Jun 25, 2024

View reviewed changes

maflcko reviewed Jun 25, 2024

View reviewed changes

l0rinc force-pushed the paplorinc/AssembleBlock branch 2 times, most recently from 0649a5b to 3c0b795 Compare June 25, 2024 11:49

maflcko reviewed Jun 25, 2024

View reviewed changes

test/functional/mining_prioritisetransaction.py Outdated Show resolved Hide resolved

l0rinc force-pushed the paplorinc/AssembleBlock branch from 3c0b795 to bc752ac Compare June 25, 2024 12:51

fanquake reviewed Jun 25, 2024

View reviewed changes

src/node/miner.h Outdated Show resolved Hide resolved

l0rinc force-pushed the paplorinc/AssembleBlock branch from 71753ac to a114ced Compare June 25, 2024 13:03

This was referenced Jun 25, 2024

RFC: Instanced logs #30338

Closed

Cluster mempool implementation #28676

Open

maflcko reviewed Jun 28, 2024

View reviewed changes

src/node/miner.cpp Outdated Show resolved Hide resolved

l0rinc requested review from tdb3, maflcko and fanquake June 28, 2024 12:34

l0rinc force-pushed the paplorinc/AssembleBlock branch from a114ced to 7759cd8 Compare June 28, 2024 12:34

tdb3 approved these changes Jun 30, 2024

View reviewed changes

furszy reviewed Jun 30, 2024

View reviewed changes

l0rinc force-pushed the paplorinc/AssembleBlock branch from 7759cd8 to 323ce30 Compare June 30, 2024 21:15

tdb3 approved these changes Jun 30, 2024

View reviewed changes

furszy reviewed Jun 30, 2024

View reviewed changes

This was referenced Jul 1, 2024

refactor: add coinbase constraints to BlockAssembler::Options #30356

Merged

Stratum v2 Template Provider (take 3) #29432

Closed

achow101 merged commit be63674 into bitcoin:master Jul 2, 2024

bitcoin locked and limited conversation to collaborators Jul 2, 2025

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

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

Uh oh!

Conversation

l0rinc commented Jun 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

l0rinc Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

furszy Jun 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Jun 30, 2024

Choose a reason for hiding this comment

Uh oh!

l0rinc commented Jun 30, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy Jun 30, 2024

Choose a reason for hiding this comment

Uh oh!

l0rinc Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 2, 2024

Uh oh!

achow101 commented Jul 2, 2024

Uh oh!

Uh oh!

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

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

l0rinc commented Jun 23, 2024 •

edited

Loading

DrahtBot commented Jun 23, 2024 •

edited

Loading

l0rinc Jun 25, 2024 •

edited

Loading

furszy Jun 30, 2024 •

edited

Loading