I am trying to understand better the performance of this. Posting here to avoid cluttering the PR’s main discussion thread.
#29436#pullrequestreview-1891096814:
I tried to test the performance impact on the “normal” case of an addrman that only has addrs for reachable nets:
If I change the AddrManSelect benchmark to use addrman.Select(false, g_reachable_nets.All()); the performance goes down by a factor of ~5 for me
One thing to note here is that the benchmark fills addrman solely with IPv6 addresses. That’s not representative. So I borrowed the peers.dat from my mainnet node and ran the tests below.
As a start I began from 7edb07ca80 (this PR before the performance related tweaks). My aim was to 1. confirm that there is indeed a performance issue and 2. to understand where exactly it comes from so it can be addressed in the best way. Then I compared with the PR and then I compared with the PR + a change to make the last argument of Select() const ref.
addrman size:
0IPV4: 49123
1IPV6: 11071
2ONION:15096
3I2P: 1863
4CJDNS: 2
Numbers in ns/op (bigger number => slower).
| \ |
Baseline (master @ 7edb07ca80~3) |
PR (7edb07ca80) |
PR (7edb07ca80) + const ref argument |
| any (empty optional or empty map) |
130 |
140 A |
135 |
| IPV4 or {IPV4} |
190 |
320 |
200 |
| IPV6 or {IPV6} |
490 |
640 |
510 |
| ONION or {ONION} |
1200 |
1370 |
1270 |
| I2P or {I2P} |
2500 |
2650 |
2550 |
| CJDNS or {CJDNS} |
1500000 |
1700000 |
1600000 |
| {IPV4, IPV6, ONION, I2P, CJDNS} |
- |
550 B |
150 C |
A vs B is probably what @mzumsande observed in #29436#pullrequestreview-1891096814. But fixing the arg changes that to A vs C.
Based on the above, I think that it is best to go back to 7edb07ca80 and only fix the Select() argument.
0diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp
1index f044feebba..dd45b9e69b 100644
2--- a/src/bench/addrman.cpp
3+++ b/src/bench/addrman.cpp
4@@ -1,12 +1,15 @@
5 // Copyright (c) 2020-2022 The Bitcoin Core developers
6 // Distributed under the MIT software license, see the accompanying
7 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
8
9+#include <addrdb.h>
10 #include <addrman.h>
11 #include <bench/bench.h>
12+#include <chainparams.h>
13+#include <common/args.h>
14 #include <netbase.h>
15 #include <netgroup.h>
16 #include <random.h>
17 #include <util/check.h>
18 #include <util/time.h>
19
20@@ -81,12 +84,37 @@ static void AddrManAdd(benchmark::Bench& bench)
21 bench.run([&] {
22 AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
23 AddAddressesToAddrMan(addrman);
24 });
25 }
26
27+static void AddrManSelectMain(benchmark::Bench& bench)
28+{
29+ SelectParams(ChainType::MAIN);
30+ gArgs.ForceSetArg("checkaddrman", "0");
31+ gArgs.ForceSetArg("datadir", "/tmp");
32+ auto result = LoadAddrman(EMPTY_NETGROUPMAN, gArgs);
33+ AddrMan& addrman = *(*result).get();
34+
35+ std::cout << "addrman size:\n";
36+ std::cout << "IPv4: " << addrman.Size(NET_IPV4) << "\n";
37+ std::cout << "IPv6: " << addrman.Size(NET_IPV6) << "\n";
38+ std::cout << "Tor: " << addrman.Size(NET_ONION) << "\n";
39+ std::cout << "I2P: " << addrman.Size(NET_I2P) << "\n";
40+ std::cout << "CJDNS:" << addrman.Size(NET_CJDNS) << "\n";
41+
42+ //const std::optional<Network> n{NET_CJDNS};
43+ //const std::unordered_set<Network> n{NET_IPV4, NET_IPV6, NET_ONION, NET_I2P, NET_CJDNS};
44+ //const std::unordered_set<Network> n{};
45+ //const std::unordered_set<Network> n{g_reachable_nets.All()};
46+ bench.run([&] {
47+ const auto& address = addrman.Select(/*new_only=*/false, n);
48+ assert(address.first.IsValid());
49+ });
50+}
51+
52 static void AddrManSelect(benchmark::Bench& bench)
53 {
54 AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
55
56 FillAddrMan(addrman);
57
58@@ -167,11 +195,12 @@ static void AddrManAddThenGood(benchmark::Bench& bench)
59
60 markSomeAsGood(addrman);
61 });
62 }
63
64 BENCHMARK(AddrManAdd, benchmark::PriorityLevel::HIGH);
65+BENCHMARK(AddrManSelectMain, benchmark::PriorityLevel::HIGH);
66 BENCHMARK(AddrManSelect, benchmark::PriorityLevel::HIGH);
67 BENCHMARK(AddrManSelectFromAlmostEmpty, benchmark::PriorityLevel::HIGH);
68 BENCHMARK(AddrManSelectByNetwork, benchmark::PriorityLevel::HIGH);
69 BENCHMARK(AddrManGetAddr, benchmark::PriorityLevel::HIGH);
70 BENCHMARK(AddrManAddThenGood, benchmark::PriorityLevel::HIGH);
0diff --git i/src/addrman.cpp w/src/addrman.cpp
1index 4e551e81c3..bde2d90ef2 100644
2--- i/src/addrman.cpp
3+++ w/src/addrman.cpp
4@@ -705,13 +705,13 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, NodeSeconds
5 if (fCountFailure && info.m_last_count_attempt < m_last_good) {
6 info.m_last_count_attempt = time;
7 info.nAttempts++;
8 }
9 }
10
11-std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::unordered_set<Network> networks) const
12+std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, const std::unordered_set<Network>& networks) const
13 {
14 AssertLockHeld(cs);
15
16 if (vRandom.empty()) return {};
17
18 size_t new_count = nNew;
19@@ -1208,13 +1208,13 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::SelectTriedCollision()
20 Check();
21 auto ret = SelectTriedCollision_();
22 Check();
23 return ret;
24 }
25
26-std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, std::unordered_set<Network> networks) const
27+std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, const std::unordered_set<Network>& networks) const
28 {
29 LOCK(cs);
30 Check();
31 auto addrRet = Select_(new_only, networks);
32 Check();
33 return addrRet;
34@@ -1315,13 +1315,13 @@ void AddrMan::ResolveCollisions()
35
36 std::pair<CAddress, NodeSeconds> AddrMan::SelectTriedCollision()
37 {
38 return m_impl->SelectTriedCollision();
39 }
40
41-std::pair<CAddress, NodeSeconds> AddrMan::Select(bool new_only, std::unordered_set<Network> networks) const
42+std::pair<CAddress, NodeSeconds> AddrMan::Select(bool new_only, const std::unordered_set<Network>& networks) const
43 {
44 return m_impl->Select(new_only, networks);
45 }
46
47 std::vector<CAddress> AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
48 {
49diff --git i/src/addrman.h w/src/addrman.h
50index 2d2846993a..b6fdb07f82 100644
51--- i/src/addrman.h
52+++ w/src/addrman.h
53@@ -157,13 +157,13 @@ public:
54 * guarantee a tried entry).
55 * [@param](/bitcoin-bitcoin/contributor/param/)[in] networks Select only addresses of these networks (empty = all). Passing networks may
56 * slow down the search.
57 * [@return](/bitcoin-bitcoin/contributor/return/) CAddress The record for the selected peer.
58 * seconds The last time we attempted to connect to that peer.
59 */
60- std::pair<CAddress, NodeSeconds> Select(bool new_only = false, std::unordered_set<Network> networks = {}) const;
61+ std::pair<CAddress, NodeSeconds> Select(bool new_only = false, const std::unordered_set<Network>& networks = {}) const;
62
63 /**
64 * Return all or many randomly selected addresses, optionally by network.
65 *
66 * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_addresses Maximum number of addresses to return (0 = all).
67 * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_pct Maximum percentage of addresses to return (0 = all).
68diff --git i/src/addrman_impl.h w/src/addrman_impl.h
69index bb89213bb7..2a4596e81f 100644
70--- i/src/addrman_impl.h
71+++ w/src/addrman_impl.h
72@@ -123,13 +123,13 @@ public:
73 EXCLUSIVE_LOCKS_REQUIRED(!cs);
74
75 void ResolveCollisions() EXCLUSIVE_LOCKS_REQUIRED(!cs);
76
77 std::pair<CAddress, NodeSeconds> SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs);
78
79- std::pair<CAddress, NodeSeconds> Select(bool new_only, std::unordered_set<Network> networks) const
80+ std::pair<CAddress, NodeSeconds> Select(bool new_only, const std::unordered_set<Network>& networks) const
81 EXCLUSIVE_LOCKS_REQUIRED(!cs);
82
83 std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const
84 EXCLUSIVE_LOCKS_REQUIRED(!cs);
85
86 std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries(bool from_tried) const
87@@ -250,13 +250,13 @@ private:
88 bool Good_(const CService& addr, bool test_before_evict, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs);
89
90 bool Add_(const std::vector<CAddress>& vAddr, const CNetAddr& source, std::chrono::seconds time_penalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
91
92 void Attempt_(const CService& addr, bool fCountFailure, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs);
93
94- std::pair<CAddress, NodeSeconds> Select_(bool new_only, std::unordered_set<Network> networks) const EXCLUSIVE_LOCKS_REQUIRED(cs);
95+ std::pair<CAddress, NodeSeconds> Select_(bool new_only, const std::unordered_set<Network>& networks) const EXCLUSIVE_LOCKS_REQUIRED(cs);
96
97 /** Helper to generalize looking up an addrman entry from either table.
98 *
99 * [@return](/bitcoin-bitcoin/contributor/return/) int The nid of the entry. If the addrman position is empty or not found, returns -1.
100 * */
101 int GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs);
