addrman: improve performance of `GetAddr` when specifying network

brunoerg commented at 1:44 pm on March 6, 2024: contributor

GetAddr returns all or many randomly selected addresses, it uses vRandom (randomly-ordered vector of all nIds) to get the addresses. However, if a network is specified, we could check the number of entries in vRandom from that network using m_network_counts since it contains the number of entries in addrman per network. This way we could avoid unnecessary searchs in vRandom. So…

This PR improve the performance of addrman’s GetAddr by checking m_network_counts when specifying a network.

AddrManGetAddrByNetwork bench:

master:

0|               ns/op |                op/s |    err% |     total | benchmark
1|--------------------:|--------------------:|--------:|----------:|:----------
2|          797,416.00 |            1,254.05 |    0.2% |      0.01 | `AddrManGetAddrByNetwork`

this PR:

0|               ns/op |                op/s |    err% |     total | benchmark
1|--------------------:|--------------------:|--------:|----------:|:----------
2|          462,875.00 |            2,160.41 |    5.6% |      0.01 | :wavy_dash: `AddrManGetAddrByNetwork` (Unstable with ~2.2 iters. Increase `minEpochIterations` to e.g. 22)

DrahtBot commented at 1:44 pm on March 6, 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	vasild

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label P2P on Mar 6, 2024

jess2505 approved

paplorinc commented at 11:01 pm on March 8, 2024: none

I was able to reproduce the diff, but please rebase the changes so that the benchmark is before the change so that we can easily check before/after.

Running it locally:

make && ./src/bench/bench_bitcoin –filter=AddrManGetAddrByNetwork –min-time=1000

Before:

0|               ns/op |                op/s |    err% |     total | benchmark
1|--------------------:|--------------------:|--------:|----------:|:----------
2|          819,423.49 |            1,220.37 |    0.3% |      1.07 | `AddrManGetAddrByNetwork`

after:

0|               ns/op |                op/s |    err% |     total | benchmark
1|--------------------:|--------------------:|--------:|----------:|:----------
2|          418,538.38 |            2,389.27 |    1.7% |      1.08 | `AddrManGetAddrByNetwork`

in src/addrman.cpp:818 in 98cc6ac935 outdated

814+    if (network) {
815+        auto it = m_network_counts.find(*network);
816+        if (it == m_network_counts.end()) return {};
817+        const auto counts{it->second};
818+        nNodes = counts.n_new + counts.n_tried;
819+    }

paplorinc commented at 11:06 pm on March 8, 2024:

nit: To make it clear that we’re overwriting the same value:

0    size_t nNodes;
1    if (network) {
2        auto it = m_network_counts.find(*network);
3        if (it == m_network_counts.end()) return {};
4        nNodes = it->second.n_new + it->second.n_tried;
5    } else {
6        nNodes = vRandom.size();
7    }

brunoerg commented at 2:16 pm on March 9, 2024:

I’ll leave it as is for now.

brunoerg force-pushed on Mar 9, 2024

brunoerg commented at 2:17 pm on March 9, 2024: contributor

I was able to reproduce the diff, but please rebase the changes so that the benchmark is before the change so that we can easily check before/after.

Nice idea, force-pushed changing the order of the commits.

vasild approved

vasild commented at 10:11 am on March 12, 2024: contributor

ACK 57db2e7e5ecaa6d23efd9e30ceac48fb6489d213

Changing the argument from std::optional<Network> to const std::optional<Network>& seems unnecessary because it is very cheap to copy that, it is 8 bytes. I would drop commit 1319958b45455a18800cb83d483448a3021b2aba addrman: net: use const ref for network in GetAddresses/GetAddr

brunoerg force-pushed on Mar 12, 2024

brunoerg commented at 10:41 am on March 12, 2024: contributor

Changing the argument from std::optional to const std::optional& seems unnecessary because it is very cheap to copy that, it is 8 bytes. I would drop commit https://github.com/bitcoin/bitcoin/commit/1319958b45455a18800cb83d483448a3021b2aba addrman: net: use const ref for network in GetAddresses/GetAddr

Done. Thanks, @vasild.

vasild approved

vasild commented at 11:09 am on March 12, 2024: contributor

ACK e1e107893594be1f1c79e4432fa345a0bbc1d8a4

addrman: bench: add benchmark for `GetAddr` with network afa1ca5ad4

addrman: improve `GetAddr` when specifying a network bb11488aa5

Preallocate addresses in GetAddr based on nNodes

> make && ./src/bench/bench_bitcoin --filter=AddrManGetAddr --min-time=1000

Before:
```
|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           76,852.79 |           13,011.89 |    0.4% |      1.07 | `AddrManGetAddr`
|           76,598.21 |           13,055.14 |    0.2% |      1.07 | `AddrManGetAddr`
|           76,296.32 |           13,106.79 |    0.1% |      1.07 | `AddrManGetAddr`
```
After:
```
|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           65,966.97 |           15,159.10 |    0.3% |      1.07 | `AddrManGetAddr`
|           66,075.40 |           15,134.23 |    0.2% |      1.06 | `AddrManGetAddr`
|           66,306.34 |           15,081.51 |    0.3% |      1.06 | `AddrManGetAddr`
```

32a44d77eb

brunoerg force-pushed on Mar 13, 2024

brunoerg commented at 8:22 pm on March 13, 2024: contributor

Rebased to fix CI.

in src/addrman.cpp:817 in bb11488aa5 outdated

809@@ -810,6 +810,12 @@ std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct
810     AssertLockHeld(cs);
811 
812     size_t nNodes = vRandom.size();
813+    if (network) {
814+        auto it = m_network_counts.find(*network);
815+        if (it == m_network_counts.end()) return {};
816+        const auto counts{it->second};
817+        nNodes = counts.n_new + counts.n_tried;

mzumsande commented at 8:33 pm on March 13, 2024:

I think that this changes behavior in theory: If both a network and max_pct are specified, it will now return max_pct of nodes from that network (before, max_pct would apply to all nodes). In practice, that doesn’t really matter because we specify either the one or the other in existing calls.

brunoerg commented at 8:59 pm on March 13, 2024:

Yes, I noticed it but ignored because in practice it won’t happen. Perhaps worth adding an assertion?

vasild commented at 2:07 pm on March 27, 2024:

I think it is fine without an assert because nothing bad will happen if both are provided.

vasild approved

vasild commented at 2:04 pm on March 27, 2024: contributor

ACK 32a44d77eb5cc0ff56fd05b2f9da914878e5746a

dergoegge commented at 2:55 pm on March 27, 2024: member

This PR improve the performance of addrman’s GetAddr

I know I’m gonna make myself unpopular by saying this but these PRs require hours and hours from reviewers and authors that I’d rather see work on more impactful things. Given that this project has extremely limited resources, the cost to benefit ratio just doesn’t make any sense to me here.

Why does the performance of GetAddr going from 797 to 460 micro seconds per call matter to this project? If you would create a trace for a running node instance, GetAddr would make up a tiny tiny tiny fraction of cpu time.

I’m open to being convinced but I’m very skeptical that there are no other more impactful performance improvements we could be spending our time on instead.

brunoerg commented at 4:06 pm on March 27, 2024: contributor

This PR improve the performance of addrman’s GetAddr

I know I’m gonna make myself unpopular by saying this but these PRs require hours and hours from reviewers and authors that I’d rather see work on more impactful things. Given that this project has extremely limited resources, the cost to benefit ratio just doesn’t make any sense to me here.

Why does the performance of GetAddr going from 797 to 460 micro seconds per call matter to this project? If you would create a trace for a running node instance, GetAddr would make up a tiny tiny tiny fraction of cpu time.

I’m open to being convinced but I’m very skeptical that there are no other more impactful performance improvements we could be spending our time on instead.

I super respect your opinion and agree with your point about focusing on more impactful things. However, I do not think this PR requires hours and hours from reviewers/authors, the impactful change here is around 6 lines of code and it does not conflict with any other more important PR (better, any other PR). Also, GetAddr/GetAddresses is used in different scenarios: replying GETADDR, getnodeaddresses RPC, ASMapHealthCheck. I think this case worths, but I’m open and understand your point.

paplorinc commented at 4:10 pm on March 27, 2024: none

these PRs require hours and hours from reviewers and authors that I’d rather see work on more impactful things

This is how the new generation is recruited, newcomers cannot (and should not) start with “more impactful things”.

dergoegge commented at 4:39 pm on March 27, 2024: member

I do not think this PR requires hours and hours from reviewers/authors, the impactful change here is around 6 lines of code and it does not conflict with any other more important PR (better, any other PR).

One of these PRs might be harmless, but PRs like this one keep happening and the cumulative review time piles up.

Also, GetAddr/GetAddresses is used in different scenarios: replying GETADDR, getnodeaddresses RPC, ASMapHealthCheck.

Yes GetAddr is used but whether or not it can be called a 1000 or 2000 times per second doesn’t matter to any of the use cases.

No application requires calling getnodeaddresses 1000s of times per second (is GetAddr even the bottleneck of that rpc?)
getaddr messages are on average received one time per connection (we only send it once during the version handshake)
ASMapHealthCheck is called once every 24 hours

This is how the new generation is recruited, newcomers cannot (and should not) start with “more impactful things”. @paplorinc I see your point but I don’t think that “more impactful” implies non-trivial. Easy and smaller yet still impactful changes are certainly possible but I think this project could do better in providing guidance to newcomers in that regard.

brunoerg commented at 6:19 pm on March 27, 2024: contributor

@dergoegge I’ll consider your point for further PRs (including reviews). Thanks.

brunoerg commented at 7:03 am on April 9, 2024: contributor

Closing for now.

brunoerg closed this on Apr 9, 2024

vasild commented at 11:11 am on April 19, 2024: contributor

The development of Bitcoin Core is decentralized because every developer decides for themselves what is important and what not.

Asserting one’s opinion on others is more in line with centralized corporate management.

maflcko commented at 11:31 am on April 19, 2024: member

opinion

Performance improvements should be measured in numbers by machines, which does not seem like an opinion. According to #29578 (comment) , the code can only be reached by RPC and P2P. For those places no end-to-end performance difference has been provided. I’d say if there is a measurable (reproducible) performance improvement for a users’ use case, or another use case that hasn’t been mentioned yet, the change can be considered based on that. Otherwise, the code change seems like a style change, which largely is opinion-based.

addrman: improve performance of GetAddr when specifying network #29578

Code Coverage

Reviews

addrman: improve performance of `GetAddr` when specifying network #29578