net processing: swap out signals for an interface class

theuni commented at 8:26 pm on July 6, 2017: member

See individual commits. Benefits:

Allows us to begin moving stuff out of CNode and into CNodeState (after #10652 and follow-ups)
Drops boost dependency and overhead
Drops global signal registration
Friendlier backtraces

fanquake added the label P2P on Jul 7, 2017

gmaxwell commented at 11:35 am on July 9, 2017: contributor

Super mega concept ACK. FWIW, I tested something functionally like this before (eliminating the signals with a direct function call) and saw a measurable performance improvement, I assume because there is synchronization inside the signals stuff (plus a super deep call stack).

TheBlueMatt commented at 0:22 am on July 10, 2017: member

This is awesome. Lots of future work to do, but good first step. Will circle back around post-15.

jnewbery commented at 1:59 pm on July 10, 2017: member

@gmaxwell

tested something functionally like this before (eliminating the signals with a direct function call) and saw a measurable performance improvement

How did you test that? It’d be good to benchmark whether this gives us a performance improvement.

in src/net.cpp:1994 in f785e8479a outdated

1995@@ -1996,15 +1996,16 @@ void CConnman::ThreadMessageHandler()
1996                 continue;
1997 
1998             // Receive messages

jnewbery commented at 2:07 pm on July 10, 2017:

Nit: move Receive messages comment into the if block (the block covers both receiving and sending messages. Having the comment outside the block suggests it’s just for receiving messages)

in src/net.h:425 in f785e8479a outdated

422@@ -422,18 +423,18 @@ struct CombinerAll
423 };
424 
425 // Signals for message handling

jnewbery commented at 2:11 pm on July 10, 2017:

Change comment to Interface for message handling

in src/net_processing.h:46 in f785e8479a outdated

48+    bool ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt) override;
49+    /**
50+    * Send queued protocol messages to be sent to a give node.
51+    *
52+    * @param[in]   pto             The node which we are sending messages to.
53+    * @param[in]   connman         The connection manager for that node.

jnewbery commented at 2:18 pm on July 10, 2017:

remove this line. connman is no longer a param for this function.

jnewbery commented at 2:26 pm on July 10, 2017: member

Tested ACK f785e8479a8ffca6c89d71f9dd6f1c1b455c2671. A few nitty comments inline, plus one more: https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.cpp#L126 seems to be a comment that’s got stranded from its code, and is now nonsensical after this PR. Consider removing it entirely.

I’ve run this over the function test suite and it seemed to run slightly faster, though within expected variance. Do you have any ways of more focused performance testing?

theuni commented at 0:19 am on July 11, 2017: member

Remembering @gmaxwell’s mention of it before, I was hoping to see a slight speedup here, but I haven’t been able to observe anything substantial either. No test varies more than 1% from any other. @jnewbery my test was to do a sync to block 150000 between two localhost nodes running from ramdisk datadirs. I believe that focuses on the hot-spot well enough.

theuni force-pushed on Jul 11, 2017

theuni commented at 4:37 pm on July 11, 2017: member

Needed rebase after #10179, so I went ahead and squashed it down too.

gmaxwell commented at 7:17 pm on July 11, 2017: contributor

@jnewbery GBE connected hosts running a patch like this, out of ram, may have also required turning up our networking buffers. maybe that other locking changes we made mooted the improvement. I’ll go see if I can find the relevant chat logs.

gmaxwell approved

gmaxwell commented at 7:18 pm on July 11, 2017: contributor

ACK.

in src/net.cpp:2006 in 1f166fd63e outdated

2011+                fMoreWork |= (fMoreNodeWork && !pnode->fPauseSend);
2012+                if (flagInterruptMsgProc)
2013+                    return;
2014+                // Send messages
2015+                {
2016+                    LOCK(pnode->cs_sendProcessing);

TheBlueMatt commented at 6:37 pm on August 2, 2017:

Can you drop cs_sendProcessing while you’re here? Its literally entirely unused except for this lock here.

theuni commented at 8:35 pm on August 2, 2017:

Sure. It’s safe to drop it now, but we’d need to put it back for parallel processing.

in src/net.cpp:2216 in 1f166fd63e outdated

2212@@ -2212,6 +2213,7 @@ CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In) : nSeed0(nSeed0In), nSe
2213     nMaxAddnode = 0;
2214     nBestHeight = 0;
2215     clientInterface = NULL;
2216+    messageInterface = NULL;

TheBlueMatt commented at 6:58 pm on August 2, 2017:

I believe we’re nullptr’ing now?

theuni commented at 8:35 pm on August 2, 2017:

ok

in src/net.cpp:1968 in 1f166fd63e outdated

1963@@ -1966,7 +1964,9 @@ bool CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
1964     if (fAddnode)
1965         pnode->fAddnode = true;
1966 
1967-    GetNodeSignals().InitializeNode(pnode, *this);
1968+    if (messageInterface) {
1969+        messageInterface->InitializeNode(pnode);

TheBlueMatt commented at 7:00 pm on August 2, 2017:

I feel like we should just go for the direct deref here. A CConnman without a messageInterface feels like a client-of-the-API failure. Maybe just assert(messageInterface) in ::Start?

theuni commented at 8:49 pm on August 2, 2017:

Heh, I went back and forth on this. Was waiting for someone to complain. Agreed.

in src/net.h:429 in 1f166fd63e outdated

430-    boost::signals2::signal<bool (CNode*, CConnman&, std::atomic<bool>&), CombinerAll> ProcessMessages;
431-    boost::signals2::signal<bool (CNode*, CConnman&, std::atomic<bool>&), CombinerAll> SendMessages;
432-    boost::signals2::signal<void (CNode*, CConnman&)> InitializeNode;
433-    boost::signals2::signal<void (NodeId, bool&)> FinalizeNode;
434+protected:
435+    NetEventsInterface() = default;

TheBlueMatt commented at 7:01 pm on August 2, 2017:

Why did you define the constructor here?

theuni commented at 8:39 pm on August 2, 2017:

Just a habit of mine to declare the ctor as protected for classes that should be inherited from. Since there are pure virtuals here, it’s not really necessary.

in src/net.h:431 in 1f166fd63e outdated

432-    boost::signals2::signal<void (CNode*, CConnman&)> InitializeNode;
433-    boost::signals2::signal<void (NodeId, bool&)> FinalizeNode;
434+protected:
435+    NetEventsInterface() = default;
436+public:
437+    virtual ~NetEventsInterface() = default;

TheBlueMatt commented at 7:02 pm on August 2, 2017:

Why do we need to make the destructor virtual? CConnman doesn’t care about construction/destruction, it just takes a ptr.

theuni commented at 8:46 pm on August 2, 2017:

To make sure that the base gets cleaned up if, for example, it’s stored as an abstract std::unique_ptr

TheBlueMatt commented at 11:22 pm on August 2, 2017:

Well my point was the CConnman (and related net stuff), which “owns” this interface explicitly does not do so (and does not have any desire to ever own the object), so adding anything additional to the interface feels strange to me.

theuni commented at 10:49 pm on August 3, 2017:

I must be missing your point. This is just a safety precaution in case anything ever wants to hold an abstract pointer. This may be the case if, for example, we create a vector of message processors in order to run them against each-other.

I can remove it if you’d like, but I’m not understanding the objection to being cautious.

TheBlueMatt commented at 11:02 pm on August 3, 2017:

Well my objection was just that it forces you into a virtual destructor unless you remember to call derived classes final, and since I dont think anything should ever have anything other than a reference to (ie will not destruct directly) a NetEventsInterface, no need to make anything virtual. I don’t care too strongly, however.

TheBlueMatt commented at 7:03 pm on August 2, 2017: member

This is awesome. As mentioned, after this, PeerLogicValidation should probably get a raname, as the “Validation” referred to ValidationInterface.

TheBlueMatt commented at 11:22 pm on August 2, 2017: member

Note that some of the Process functions are likely to move again into CNodeState or so, I believe, but this is an obviously good step.

theuni force-pushed on Aug 16, 2017

theuni commented at 6:22 pm on August 16, 2017: member

Rebased, updated for recent changes, and addressed @TheBlueMatt’s feedback

jonasschnelli added this to the "Blockers" column in a project

in src/net_processing.cpp:372 in 6e6f06c8d9 outdated

368@@ -417,20 +369,20 @@ void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connman) {
369                 return;
370             }
371         }
372-        connman.ForNode(nodeid, [&connman](CNode* pfrom){
373+        connman->ForNode(nodeid, [&connman](CNode* pfrom){

TheBlueMatt commented at 0:44 am on August 18, 2017:

Can drop the & now for connman, can just copy (and in a few other places).

TheBlueMatt commented at 10:48 pm on August 18, 2017: member

utACK 6e6f06c8d971f21262c6f2272b70170d19670c2a. Feel like renaming PeerLogicValidation now that its not strictly about “Validation” anymore?

theuni force-pushed on Sep 5, 2017

theuni commented at 11:12 pm on September 5, 2017: member

Rebased. Same as before, plus I took the suggestion here: #10756 (review)

laanwj assigned laanwj on Sep 6, 2017

in src/net.cpp:92 in 927036f96d outdated

88@@ -89,10 +89,6 @@ std::string strSubVersion;
89 
90 limitedmap<uint256, int64_t> mapAlreadyAskedFor(MAX_INV_SZ);
91 
92-// Signals for message handling

laanwj commented at 9:10 pm on September 6, 2017:

Another global block gone, nice!

in src/net.h:165 in 927036f96d outdated

160         nSendBufferMaxSize = connOptions.nSendBufferMaxSize;
161         nReceiveFloodSize = connOptions.nReceiveFloodSize;
162         nMaxOutboundTimeframe = connOptions.nMaxOutboundTimeframe;
163         nMaxOutboundLimit = connOptions.nMaxOutboundLimit;
164         vWhitelistedRange = connOptions.vWhitelistedRange;
165+

laanwj commented at 9:12 pm on September 6, 2017:

Empty line?

theuni commented at 11:34 pm on September 6, 2017:

Fixed.

net: pass CConnman via pointer rather than reference

There are a few too many edge-cases here to make this a scripted diff.

The following commits will move a few functions into PeerLogicValidation, where
the local connman instance can be used. This change prepares for that usage.

28f11e9406

net: use an interface class rather than signals for message processing

Drop boost signals in favor of a stateful class. This will allow the message
processing loop to actually move to net_processing in a future step.

8ad663c1fa

net: drop unused connman param

The copy in PeerLogicValidation can be used instead.

80e2e9d0ce

in src/net.h:442 in 927036f96d outdated

438@@ -436,19 +439,16 @@ struct CombinerAll
439     }
440 };
441 
442-// Signals for message handling
443-struct CNodeSignals
444+// Interface for message handling

laanwj commented at 9:13 pm on September 6, 2017:

Ideally we’d use a doxygen-compliant comment here, so that it shows the class description

0/**
1 * Interface for message handling
2 */

theuni commented at 11:34 pm on September 6, 2017:

I had to rebase anyway, so I went ahead and fixed these two nits as well.

theuni force-pushed on Sep 6, 2017

in src/net_processing.h:25 in 80e2e9d0ce outdated

26-void RegisterNodeSignals(CNodeSignals& nodeSignals);
27-/** Unregister a network node */
28-void UnregisterNodeSignals(CNodeSignals& nodeSignals);
29-
30-class PeerLogicValidation : public CValidationInterface {
31+class PeerLogicValidation : public CValidationInterface, public NetEventsInterface {

JeremyRubin commented at 0:50 am on September 7, 2017:

Maybe it makes sense to mark stuff final/not be overriding an abc here? NetEventsInterface feels overkill unless we anticipate per-connection different handlers.

theuni commented at 5:47 am on September 7, 2017:

I don’t anticipate per-connection, but definitely per-connman. I have immediate plans to add different handlers and run them against each-other for testing.

Making this final is a good idea, but I’d rather not keep squashing more changes in. Let’s do that in a follow up that also renames PeerLogicValidation to something that better reflects what it’s doing.

JeremyRubin approved

JeremyRubin commented at 0:50 am on September 7, 2017: contributor

utack 80e2e9d

net: stop both net/net_processing before destroying them

This should avoid either attempting to use an invalid reference/pointer to the
other.

2525b972af

in src/init.cpp:199 in 80e2e9d0ce outdated

194@@ -195,11 +195,10 @@ void Shutdown()
195 #endif
196     MapPort(false);
197     UnregisterValidationInterface(peerLogic.get());
198-    peerLogic.reset();
199     g_connman.reset();
200+    peerLogic.reset();

TheBlueMatt commented at 5:52 pm on September 7, 2017:

As discussed, peerLogic should probably get destroyed before g_connman, though g_connman needs to be stopped first (and all references to m_msgproc in CConnman need to be in threads that will be stopped or should be gated on running - OpenNetworkConnection I believe is the only current violation of this).

TheBlueMatt commented at 6:37 pm on September 7, 2017: member

utACK 2525b972af6645ca239ac1078cffb132b402bfbb

laanwj merged this on Sep 7, 2017

laanwj closed this on Sep 7, 2017

laanwj referenced this in commit 723e580657 on Sep 7, 2017

laanwj removed this from the "Blockers" column in a project

MarcoFalke referenced this in commit e529907431 on Nov 1, 2017

MarcoFalke referenced this in commit f0d318c6a4 on Nov 1, 2017

MarcoFalke referenced this in commit 0e3400b32c on Nov 1, 2017

MarcoFalke referenced this in commit 24ea21bc8f on Nov 1, 2017

MarcoFalke referenced this in commit 8aee55af3d on Nov 2, 2017

MarcoFalke referenced this in commit dc897e53d8 on Nov 2, 2017

MarcoFalke referenced this in commit b4136f21cf on Nov 2, 2017

MarcoFalke referenced this in commit 0a5477c7e3 on Nov 2, 2017

PastaPastaPasta referenced this in commit d9684ed18a on Sep 24, 2019

codablock referenced this in commit 022e80a1c7 on Sep 26, 2019

codablock referenced this in commit 2edc29ee32 on Sep 29, 2019

barrystyle referenced this in commit a2bd5cd8fb on Jan 22, 2020

jnewbery referenced this in commit 591d6632ee on Aug 24, 2020

jnewbery referenced this in commit a09c57bbf5 on Aug 28, 2020

jnewbery referenced this in commit 2f6eaa0b38 on Aug 28, 2020

jnewbery referenced this in commit 534b4a2d86 on Aug 28, 2020

jnewbery referenced this in commit 7853b63fd7 on Sep 5, 2020

jnewbery referenced this in commit 58bd369b0d on Sep 7, 2020

MarcoFalke referenced this in commit 147d50d63e on Sep 7, 2020

sidhujag referenced this in commit 0144132984 on Sep 9, 2020

janus referenced this in commit e1bafa5ae2 on Dec 7, 2020

furszy referenced this in commit f932b2d87e on Jul 1, 2021

random-zebra referenced this in commit b4751e10ce on Aug 11, 2021

DrahtBot locked this on Sep 8, 2021

net processing: swap out signals for an interface class #10756