Page MenuHomePhabricator

addrman: Improve performance of Good
ClosedPublic

Authored by PiRK on Jan 20 2022, 17:00.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC934ec8eff88b: addrman: Improve performance of Good
Summary

Currently, CAddrman::Good() is rather slow because the process of moving an addr from new to tried involves looping over the new tables twice:

  • In Good_(), there is a loop searching for a new bucket the addr is currently in, but this information is never used except for aborting if it is not found anywhere (since this commit it is no longer passed to MakeTried) This is unnecessary because in a non-corrupted addrman, an address that is not in New must be either in Tried or not at all in addrman, both cases in which we'd return early in Good_() and never get to this point. I removed this loop (and left a check for nRefCount as a belt-and-suspenders check).
  • In MakeTried(), which is called from Good_(), another loop removes all instances of this address from new. This can be spedup by stopping the search at nRefCount==0. Further reductions in nRefCount would only lead to an assert anyway. Moreover, the search can be started at the bucket determined by the source of the addr for which Good was called, so that if it is present just once in New, no further buckets need to be checked.

Improving performance of Good_ is done by removing an unnecessary loop in Good_() and looping
through the new tables in MakeTried() more efficiently, choosing a
starting value that allow us to stop early in typical cases.

Co-authored-by: John Newbery <john@johnnewbery.com>

This is a partial backport of core#22974
https://github.com/bitcoin/bitcoin/pull/22974/commits/eb2e113df13c7b1ede279878f5cbad877af49f8e

Depends on D10855

The other two commits are not applicable due to a large stack of missing fuzz backports.

Test Plan

ninja all check-all

Benchmark: ./src/bench/bitcoin-bench -filter=AddrManGood

Before:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|       96,564,809.00 |               10.36 |    0.2% |      0.49 | `AddrManGood`

|      100,490,917.00 |                9.95 |    0.8% |      0.51 | `AddrManGood`

|       99,080,029.00 |               10.09 |    0.9% |      0.50 | `AddrManGood`

After:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|        1,355,126.00 |              737.94 |    2.1% |      0.01 | `AddrManGood`

|        1,330,379.00 |              751.67 |    5.7% |      0.01 | :wavy_dash: `AddrManGood` (Unstable with ~1.0 iters. Increase `minEpochIterations` to e.g. 10)

|          859,394.00 |            1,163.61 |    1.3% |      0.00 | `AddrManGood`

Event Timeline

PiRK requested review of this revision.Jan 20 2022, 17:01

Tail of the build log:

[333/510] Building CXX object src/CMakeFiles/util.dir/util/message.cpp.o
[334/510] Building CXX object src/CMakeFiles/util.dir/util/settings.cpp.o
[335/510] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[336/510] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/consensus/tx_check.cpp.o
[337/510] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/primitives/transaction.cpp.o
[338/510] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/util/strencodings.cpp.o
[339/510] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[340/510] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/pubkey.cpp.o
[341/510] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[342/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[343/510] Building CXX object src/CMakeFiles/util.dir/util/time.cpp.o
[344/510] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[345/510] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[346/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[347/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[348/510] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[349/510] Building CXX object src/CMakeFiles/util.dir/util/system.cpp.o
[350/510] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[351/510] Linking CXX static library src/libutil.a
[352/510] Linking CXX static library src/librpcclient.a
[353/510] Linking CXX static library src/libbitcoinconsensus.a
[354/510] Linking CXX static library src/libscript.a
[355/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[356/510] Linking CXX static library src/libcommon.a
[357/510] Linking CXX shared library src/libbitcoinconsensus.so.0.24.10
[358/510] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[359/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[360/510] Linking CXX executable src/bitcoin-cli
[361/510] Linking CXX executable src/bitcoin-tx
[362/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[363/510] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[364/510] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqabstractnotifier.cpp.o
[365/510] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[366/510] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[367/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[368/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[369/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[370/510] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqnotificationinterface.cpp.o
[371/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[372/510] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqutil.cpp.o
[373/510] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[374/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[375/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[376/510] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqrpc.cpp.o
[377/510] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[378/510] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqpublishnotifier.cpp.o
[379/510] Linking CXX static library src/zmq/libzmq.a
[380/510] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[381/510] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[382/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[383/510] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[384/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[385/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[386/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[387/510] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[388/510] Linking CXX static library src/wallet/libwallet.a
[389/510] Linking CXX static library src/wallet/libwallet-tool.a
[390/510] Linking CXX executable src/bitcoin-wallet
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/addrman.cpp
179 ↗(On Diff #31873)

all these ints should really be size_t, but out of scope for this diff.

This revision is now accepted and ready to land.Jan 21 2022, 09:52
This revision was automatically updated to reflect the committed changes.