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

pull theuni wants to merge 4 commits into bitcoin:master from theuni:no-net-signals2 changing 8 files +186 −206
  1. 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
  2. fanquake added the label P2P on Jul 7, 2017
  3. 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).

  4. TheBlueMatt commented at 12: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.

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

  6. 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)

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

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

  9. 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?

  10. theuni commented at 12: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.

  11. theuni force-pushed on Jul 11, 2017
  12. theuni commented at 4:37 PM on July 11, 2017: member

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

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

  14. gmaxwell approved
  15. gmaxwell commented at 7:18 PM on July 11, 2017: contributor

    ACK.

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

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

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

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

  20. 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<NetEventsInterface>

    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.

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

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

  23. theuni force-pushed on Aug 16, 2017
  24. theuni commented at 6:22 PM on August 16, 2017: member

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

  25. jonasschnelli added this to the "Blockers" column in a project
  26. 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 12:44 AM on August 18, 2017:

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

  27. 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?

  28. theuni force-pushed on Sep 5, 2017
  29. theuni commented at 11:12 PM on September 5, 2017: member

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

  30. laanwj assigned laanwj on Sep 6, 2017
  31. 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!

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

  33. 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
  34. 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
  35. net: drop unused connman param
    The copy in PeerLogicValidation can be used instead.
    80e2e9d0ce
  36. 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

    /**
 * Interface for message handling
 */
    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.

  37. theuni force-pushed on Sep 6, 2017
  38. 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 12: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.

  39. JeremyRubin approved
  40. JeremyRubin commented at 12:50 AM on September 7, 2017: contributor

    utack 80e2e9d

  41. net: stop both net/net_processing before destroying them
    This should avoid either attempting to use an invalid reference/pointer to the
other.
    2525b972af
  42. 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).

  43. TheBlueMatt commented at 6:37 PM on September 7, 2017: member

    utACK 2525b972af6645ca239ac1078cffb132b402bfbb

  44. laanwj merged this on Sep 7, 2017
  45. laanwj closed this on Sep 7, 2017
  46. laanwj referenced this in commit 723e580657 on Sep 7, 2017
  47. laanwj removed this from the "Blockers" column in a project
  48. MarcoFalke referenced this in commit e529907431 on Nov 1, 2017
  49. MarcoFalke referenced this in commit f0d318c6a4 on Nov 1, 2017
  50. MarcoFalke referenced this in commit 0e3400b32c on Nov 1, 2017
  51. MarcoFalke referenced this in commit 24ea21bc8f on Nov 1, 2017
  52. MarcoFalke referenced this in commit 8aee55af3d on Nov 2, 2017
  53. MarcoFalke referenced this in commit dc897e53d8 on Nov 2, 2017
  54. MarcoFalke referenced this in commit b4136f21cf on Nov 2, 2017
  55. MarcoFalke referenced this in commit 0a5477c7e3 on Nov 2, 2017
  56. PastaPastaPasta referenced this in commit d9684ed18a on Sep 24, 2019
  57. codablock referenced this in commit 022e80a1c7 on Sep 26, 2019
  58. codablock referenced this in commit 2edc29ee32 on Sep 29, 2019
  59. barrystyle referenced this in commit a2bd5cd8fb on Jan 22, 2020
  60. jnewbery referenced this in commit 591d6632ee on Aug 24, 2020
  61. jnewbery referenced this in commit a09c57bbf5 on Aug 28, 2020
  62. jnewbery referenced this in commit 2f6eaa0b38 on Aug 28, 2020
  63. jnewbery referenced this in commit 534b4a2d86 on Aug 28, 2020
  64. jnewbery referenced this in commit 7853b63fd7 on Sep 5, 2020
  65. jnewbery referenced this in commit 58bd369b0d on Sep 7, 2020
  66. MarcoFalke referenced this in commit 147d50d63e on Sep 7, 2020
  67. sidhujag referenced this in commit 0144132984 on Sep 9, 2020
  68. janus referenced this in commit e1bafa5ae2 on Dec 7, 2020
  69. furszy referenced this in commit f932b2d87e on Jul 1, 2021
  70. random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
  71. DrahtBot locked this on Sep 8, 2021
Contributors
theunigmaxwellTheBlueMattjnewberylaanwjJeremyRubin
Labels
P2P
Linked (view graph)
#10652 Small step towards demangling cs_main from CNodeState#19791 [net processing] Move Misbehaving() to PeerManager
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: 2026-04-22 03:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me