net, refactor: move StartExtraBlockRelayPeers() from header to implementation #25119
Conversation
where all the other logging actions in src/net.{h,cpp} are located.
StartExtraBlockRelayPeers() does not appear to be a hotspot that needs to be
inlined for performance, as it is called from CheckForStaleTipAndEvictPeers(),
called in turn from StartScheduledTasks() with a scheduleEvery delta of 45
seconds, called at the end of AppInitMain() on bitcoind startup.
This allows dropping `#include <logging.h>` from net.h, which can improve
compile time/speed. Currently, none of the other includes in net.h use
logging.h, except src/sync.h if DEBUG_LOCKCONTENTION is defined.
|
Note: |
|
Concept ACK
Is this commit part of another PR? Github says "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository." (but it must belong to this repository, otherwise the link wouldn't work right? :D) |
|
Just so I understand (and may be helpful to others), this PR compiles as is, but As is (without this other commit), I'm probably not explaining this well, making it more complicated than it is, but do I have this right? If so, my opinion is that it would be good to add that commit, but I don't feel strongly either way. |
There was a problem hiding this comment.
ACK 51ec96b
will re-ack if you decide to include the extra commit
|
@LarryRuane thanks! that seems right to me (IIUC, building isn't slower by including what you use, as otherwise it just wouldn't compile). I'll run iwyu on that commit to double-check and maybe add it. |
| @@ -13,7 +13,6 @@ | |||
| #include <crypto/siphash.h> | |||
| #include <hash.h> | |||
| #include <i2p.h> | |||
There was a problem hiding this comment.
Unrelated, but:
net.h should add these lines:
#include <assert.h> // for assert
#include <stddef.h> // for size_t
#include <algorithm> // for max, min
#include <chrono> // for seconds, microseconds, operator""s
#include <set> // for set
#include <string> // for string, basic_string, operator<
#include <utility> // for move
#include "threadsafety.h" // for GUARDED_BY, EXCLUSIVE_LOCKS_REQUIRED
#include "version.h" // for INIT_PROTO_VERSION
class CChainParams;
class NetGroupManager;
class Sock;
namespace i2p::sam { class Session; }
net.h should remove these lines:
- #include <chainparams.h> // lines 9-9
- #include <common/bloom.h> // lines 10-10
- #include <consensus/amount.h> // lines 12-12
- #include <i2p.h> // lines 15-15
- #include <netgroup.h> // lines 20-20
- #include <policy/feerate.h> // lines 21-21
- #include <util/sock.h> // lines 30-30
- class CNodeStats; // lines 102-102
There was a problem hiding this comment.
@MarcoFalke thanks. I ran iwyu python tool on src/net.{h,cpp} with the following (LMK if that incantation can be improved or how you are running it): python3 <path-to>/iwyu_tool src/net.cpp -p . -j5 -- -Xiwyu --cxx17ns -Xiwyu --mapping_file=<path-to>/contrib/devtools/iwyu/bitcoin.core.imp
and this was its output for me
net.h should add these lines:
#include <assert.h> // for assert
#include <stddef.h> // for size_t
#include <algorithm> // for min
#include <chrono> // for seconds, microseconds, operator""s
#include <set> // for set
#include <string> // for string, operator<=>
#include <utility> // for move
#include "threadsafety.h" // for GUARDED_BY, EXCLUSIVE_LOCKS_REQUIRED
#include "tinyformat.h" // for format_error
#include "version.h" // for INIT_PROTO_VERSION
class CChainParams;
class NetGroupManager;
class Sock;
namespace i2p::sam { class Session; }
net.h should remove these lines:
- #include <chainparams.h> // lines 9-9
- #include <common/bloom.h> // lines 10-10
- #include <consensus/amount.h> // lines 12-12
- #include <i2p.h> // lines 15-15
- #include <netgroup.h> // lines 19-19
- #include <policy/feerate.h> // lines 20-20
- #include <util/sock.h> // lines 29-29
- class CNodeStats; // lines 101-101
net.cpp should add these lines:
#include <net/if.h> // for IFF_UP
#include <netinet/in.h> // for htonl, IPV6_V6ONLY, in_addr, IN6A...
#include <netinet/tcp.h> // for TCP_NODELAY
#include <string.h> // for memcpy, memcmp, strcmp
#include <sys/sdt.h> // for __sdt_type, _SDT_ASM_OPERANDS_6
#include <sys/socket.h> // for size_t, sockaddr_storage, bind
#include <compare> // for operator>, operator<, strong_orde...
#include <exception> // for exception
#include <iterator> // for back_insert_iterator, back_inserter
#include <ratio> // for ratio
#include <tuple> // for tie, tuple
#include "chainparams.h" // for Params, CChainParams
#include "crypto/common.h" // for ReadLE32
#include "crypto/siphash.h" // for CSipHasher
#include "hash.h" // for Hash, CHash256
#include "logging.h" // for LogPrintf_, LogPrint, NET, LogPrintf
#include "netgroup.h" // for NetGroupManager
#include "prevector.h" // for prevector
#include "serialize.h" // for SER_NETWORK, ser_writedata32, ser...
#include "span.h" // for Span, AsBytes, MakeByteSpan
#include "streams.h" // for CDataStream, CAutoFile, CVectorWr...
#include "sync.h" // for LOCK, AssertLockNotHeldInternal
#include "threadinterrupt.h" // for CThreadInterrupt
#include "timedata.h" // for GetAdjustedTime
#include "util/time.h" // for GetTime, count_seconds, GetTimeMi...
net.cpp should remove these lines:
- #include <crypto/sha256.h> // lines 18-18
- #include <fcntl.h> // lines 40-40
- #include <math.h> // lines 58-58
…from header to implementation 51ec96b refactor: move StartExtraBlockRelayPeers from header to implementation (Jon Atack) Pull request description: where all the other logging actions in src/net.{h,cpp} are located. StartExtraBlockRelayPeers() does not appear to be a hotspot that needs to be inlined for performance, as it is called from CheckForStaleTipAndEvictPeers(), called in turn from StartScheduledTasks() with a scheduleEvery delta of 45 seconds, called at the end of AppInitMain() on bitcoind startup. This allows dropping `#include <logging.h>` from net.h, which can improve compile time/speed. Currently, none of the other includes in net.h use logging.h, except src/sync.h if DEBUG_LOCKCONTENTION is defined. ACKs for top commit: LarryRuane: ACK 51ec96b theStack: ACK 51ec96b Tree-SHA512: 69b2c09163c48bfcb43355af0aa52ee7dd81efc755a7aa6a10f5e400b5e14109484437960a62a1cfac2524c2cfae981fee082846b19526b540ef5b86be97f0fe
where all the other logging actions in src/net.{h,cpp} are located.
StartExtraBlockRelayPeers() does not appear to be a hotspot that needs to be inlined for performance, as it is called from CheckForStaleTipAndEvictPeers(), called in turn from StartScheduledTasks() with a scheduleEvery delta of 45 seconds, called at the end of AppInitMain() on bitcoind startup.
This allows dropping
#include <logging.h>from net.h, which can improve compile time/speed. Currently, none of the other includes in net.h use logging.h, except src/sync.h if DEBUG_LOCKCONTENTION is defined.