addrman: improve performance of GetAddr when specifying network #29578

pull brunoerg wants to merge 3 commits into bitcoin:master from brunoerg:2024-03-addrman-getaddr changing 2 files +28 −0
  1. 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)
    
  2. 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.

  3. DrahtBot added the label P2P on Mar 6, 2024
  4. jess2505 approved
  5. 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`
    
  6. 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.
  7. brunoerg force-pushed on Mar 9, 2024
  8. 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.

  9. vasild approved
  10. 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

  11. brunoerg force-pushed on Mar 12, 2024
  12. 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.

  13. vasild approved
  14. vasild commented at 11:09 am on March 12, 2024: contributor
    ACK e1e107893594be1f1c79e4432fa345a0bbc1d8a4
  15. addrman: bench: add benchmark for `GetAddr` with network afa1ca5ad4
  16. addrman: improve `GetAddr` when specifying a network bb11488aa5
  17. 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
  18. brunoerg force-pushed on Mar 13, 2024
  19. brunoerg commented at 8:22 pm on March 13, 2024: contributor
    Rebased to fix CI.
  20. 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.
  21. vasild approved
  22. vasild commented at 2:04 pm on March 27, 2024: contributor
    ACK 32a44d77eb5cc0ff56fd05b2f9da914878e5746a
  23. 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.

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

  25. 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”.

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

  27. brunoerg commented at 6:19 pm on March 27, 2024: contributor
    @dergoegge I’ll consider your point for further PRs (including reviews). Thanks.
  28. brunoerg commented at 7:03 am on April 9, 2024: contributor
    Closing for now.
  29. brunoerg closed this on Apr 9, 2024

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

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


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me