/bitcoin

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

Open
wants to merge 3 commits into
from
+198 −214

Conversation

Reviewers

@jnewbery jnewbery

@gmaxwell gmaxwell

Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
No milestone
5 participants
@theuni @gmaxwell @TheBlueMatt @jnewbery @fanquake
@theuni
Member

theuni commented Jul 6, 2017 • edited

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 P2P label Jul 7, 2017

jnewbery referenced this pull request Jul 7, 2017

Open

[WIP] [wallet] Remove Wallet dependencies from init.cpp #10762

@gmaxwell
Member

gmaxwell commented Jul 9, 2017 • edited

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
Contributor

TheBlueMatt commented Jul 10, 2017

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

jnewbery commented Jul 10, 2017

@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.
@jnewbery

jnewbery reviewed Jul 10, 2017
View changes

Tested ACK f785e84. 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?

src/net.cpp
@@ -1996,15 +1996,16 @@ void CConnman::ThreadMessageHandler()
continue;
// Receive messages
@jnewbery

jnewbery Jul 10, 2017

Member

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)

src/net.h
@@ -422,18 +423,18 @@ struct CombinerAll
};
// Signals for message handling
@jnewbery

jnewbery Jul 10, 2017

Member

Change comment to Interface for message handling

src/net_processing.h
+ * Send queued protocol messages to be sent to a give node.
+ *
+ * @param[in] pto The node which we are sending messages to.
+ * @param[in] connman The connection manager for that node.
@jnewbery

jnewbery Jul 10, 2017

Member

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

@theuni
Member

theuni commented Jul 11, 2017

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 added some commits Jul 6, 2017

@theuni theuni 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.
 257f15a
@theuni theuni 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.
 8b73f97
@theuni theuni net: drop unused connman param 
The copy in PeerLogicValidation can be used instead.
1f166fd
@theuni
Member

theuni commented Jul 11, 2017

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

gmaxwell commented Jul 11, 2017

@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

gmaxwell approved these changes Jul 11, 2017
View changes

ACK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment