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.

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

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:

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

after:

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

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.

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 1319958 addrman: net: use const ref for network in GetAddresses/GetAddr

Done. Thanks, @vasild.

Rebased to fix CI.

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.

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.

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".

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.

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

Closing for now.

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.

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.