-
Notifications
You must be signed in to change notification settings - Fork 35.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[p2p] Pimpl AddrMan to abstract implementation details #22950
[p2p] Pimpl AddrMan to abstract implementation details #22950
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
9a35229
to
37adb27
Compare
Fixed the lint failure that was caused by the circular dependencies check More info about updated fix here: #22950 (comment) |
Concept ACK. I'm a big fan of compilation firewall patterns. From Herb Sutter:
(https://herbsutter.com/gotw/_100/) Both are relevant here. addrman.h is eventually included by just about everything, so minimizing its compilation time is potentially a big win. There's also a plan to rework its implementation. If we first make addrman into a pimpl, that could theoretically be done without any impact on the rest of the codebase. |
Concept ACK on improved separation and shorter build times. Debug build clean. Just read through pages 147-156 of Scott Meyers' "Effective Modern C++" about the (unique) pointer to implementation idiom and started looking at the commit organisation, particularly the choices in the second ("Introduce CAddrMan::Impl to encapsulate addrman implementation") and third ("Remove external dependencies on CAddrInfo objects") commits where the action appears to be. Edit, noting for myself, also:
|
Concept ACK
Yes, at some point there was discussion about doing this for |
bda37cf
to
295cd7f
Compare
adopted a different fix to the detected circular dependency- I made the impl a stand-alone class, so it's now |
concept ack! |
295cd7f
to
7b8fba0
Compare
-BEGIN VERIFY SCRIPT- git grep -l CAddrMan src/ test/ | xargs sed -i 's/CAddrMan/AddrMan/g' -END VERIFY SCRIPT-
-BEGIN VERIFY SCRIPT- git grep -l CAddrInfo src/ | xargs sed -i 's/CAddrInfo/AddrInfo/g' -END VERIFY SCRIPT-
0cc7031
to
021f869
Compare
Thank you for the thoughtful reviews @mzumsande, @martinus, @jnewbery & @theuni !! I believe I've incorporated all the review comments. A note on the latest push:Mostly I rebased & addressed comments, but in doing so I added another commit- f2e5f38. I generally attempted to get the grouping / ordering of functions to be consistent, between the declarations & definitions in AddrMan & AddrManImpl. Since (for now) the functions are repeated a few times, having predictable ordering seems valuable for making the file easier for human parsing. Thoughts on future work:1. Reduce the redundancy of having all interface functions defined 3 times. @mzumsande, @jnewbery, @theuni
Hehe, I've been meaning to play out the variadic template and take a look, it seems very appealing to reduce the boilerplate code. That said, I think incorporating 2. Improving the interface of
I agree, it's unfortunate that this PR made things slightly worse because addrman's interface now exposes an Both of these functions are exclusively used by Making the code change is not difficult, but we'd have to make the change with care because it would logically change the handling of repeated failures here. So, this is a long winded way of saying, I hope returning a pair is a temporary thing. 3. Improving doxygen comments of |
ACK 021f869 The incremental commits make this really easy to review. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept + Code Review ACK 021f869
To check the effect of compilation firewall, I added a private integer member in CAddrMan
of master
and AddrManImpl
of #22950.
Verified that the addition causes recompilation of many tests and and other object files in master
, where as in case of #22950, only the compilation of relevant test files (that includes addrman_impl header) are triggered..
Overall the commit changes are very clear and easy to review. Compilation time reduction is significant and very much observable.
Also thanks for such an elaborate review club description. Helped me to learn a ton about compiler firewall.
Below is one naming question for my own understanding.
utACK 021f869 |
ACK 021f869 Reviewed the code and did some sanity checks running this for a while on mainnet with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 021f869
I had a last-minute panic that some of the unsuspicious moves from CAddrInfo to CAddress could quietly change the behavior of calls like addr.IsValid()
if isValid
had previously been overridden by CAddrInfo, but fortunately no such overrides existed.
@@ -759,8 +759,9 @@ void AddrManImpl::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, s | |||
|
|||
// gather a list of random nodes, skipping those of low quality | |||
const int64_t now{GetAdjustedTime()}; | |||
std::vector<CAddress> addresses; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nit for one of the refactor PRs: addresses.reserve(std::min(vRandom.size(), nNodes));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-merge concept and code-review ACK 021f869
These were introduced in bitcoin#22950, but they're not used in the header, rather equivalent includes in addrman.cpp do the work.
Summary: ``` In preparation for introducing the pimpl pattern to addrman, move all function bodies out of the header file. ``` Partial backport of [[bitcoin/bitcoin#22950 | core#22950]]: bitcoin/bitcoin@5faa7dd Depends on D12326. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12327
Summary: ``` Also move `Check` and `ForceCheckAddrman` to be after the `FunctionName_` functions. ``` Also move Clear() at the end together with MakeDeterministic(), as both these methods don't exist in core's codebase. Partial backport of [[bitcoin/bitcoin#22950 | core#22950]]: bitcoin/bitcoin@f2e5f38 Depends on D12327. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12328
…ion. Summary: ``` Introduce the pimpl pattern for CAddrMan to separate the implementation details from the externally used object representation. This reduces compile-time dependencies and conceptually clarifies AddrMan's interface from the implementation specifics. Since the unit & fuzz tests currently rely on accessing CAddrMan internals, this commit introduces addrman_impl.h, which is exclusively imported by addrman.cpp and test files. ``` Partial backport of [[bitcoin/bitcoin#22950 | core#22950]]: bitcoin/bitcoin@8af5b54 Depends on D12328. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12329
Summary: ``` CAddrInfo objects are an implementation detail of how AddrMan manages and adds metadata to different records. Encapsulate this logic by updating Select & SelectTriedCollision to return the additional info that the callers need. ``` Partial backport of [[bitcoin/bitcoin#22950 | core#22950]]: bitcoin/bitcoin@7cba9d5 Depends on D12329. Test Plan: ninja all check Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12330
Summary: ``` Now that no bitcoind callers require knowledge of the CAddrInfo object, it can be moved into the test-only header file. ``` Partial backport of [[bitcoin/bitcoin#22950 | core#22950]]: bitcoin/bitcoin@e3f1ea6#diff-164bd9e2e30f54d0a79eb7cc372309e2f2155edc6c3f051290ab078f03f6a771 Depends on D12330. Test Plan: ninja all check ninja bitcoin-fuzzers Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12331
Summary: ``` Since knowledge of CAddrInfo is limited to callsites that import addrman_impl.h, only objects in addrman.cpp or the tests have access. Thus we can remove calling them friends and make the members public. ``` Partial backport of [[bitcoin/bitcoin#22950 | core#22950]]: bitcoin/bitcoin@7cf41bb Depends on D12331. Test Plan: ninja all check Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12332
Summary: Partial backport of [[bitcoin/bitcoin#22950 | core#22950]]: bitcoin/bitcoin@40acd6f Depends on D12332. Test Plan: ninja all check Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12333
Summary: ``` Update so the internal function signature matches the external one, as is the case for the other addrman functions. ``` Partial backport of [[bitcoin/bitcoin#22950 | core#22950]]: bitcoin/bitcoin@14f9e00 Depends on D12333. Test Plan: ninja all check Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12334
Summary: ``` Maintain comments on the external interfaces rather than on the internal functions that implement them. ``` Partial backport of [[bitcoin/bitcoin#22950 | core#22950]]: bitcoin/bitcoin@29727c2 Depends on D12334. Test Plan: Read the comments Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12335
Summary: Partial backport of [[bitcoin/bitcoin#22950 | core#22950]]: bitcoin/bitcoin@3c263d3 Depends on D12335. Test Plan: ninja all check Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12336
Summary: Partial backport of [[bitcoin/bitcoin#22950 | core#22950]]: bitcoin/bitcoin@dd8f7f2 Depends on D12336. There is no change in behavior. Test Plan: ninja all check-all ninja bitcoin-fuzzers ninja bench-bitcoin Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12337
Summary: Completes backport of [[bitcoin/bitcoin#22950 | core#22950]]: bitcoin/bitcoin@3757503 No change in behavior. Depends on D12337. Test Plan: ninja all check ninja bitcoin-fuzzers Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D12338
Introduce the pimpl pattern for AddrMan to separate the implementation details from the externally used object representation. This reduces compile-time dependencies and conceptually clarifies AddrMan's interface from the implementation specifics.
Since the unit & fuzz tests currently rely on accessing AddrMan internals, this PR introduces addrman_impl.h, which is exclusively imported by addrman.cpp and test files.