Currently there is a 100ms sleep in the message processing thread that adds delay to processing messages.
This patch skips the sleep when there is additional work to do.
Additionally ProcessMessages & ProcessGetData are modified such that their maximum run time is reduced; this improves the average latency peers see when there is high load. (This has a significant impact on high latency networks during initial block synchronization).
The problem is this isn’t entirely about IO.
SendMessages is actually largely a polling function with timers and other logic not dependent on IO.
I understand this is important and it should be validated and pulled. I took a look at the code and as I understand it, it does the following:
Although I’m not particularly well versed in the bitcoin codebase this commit has my ACK on a code basis.
I have built and tested it on my laptop and a vm which I -connect= and it synced the chain just fine. My laptop is showing surprisingly high processor load but it’s also serving a lot of block downloads on the order of 100kB-1500kB/s upload speed. Does anyone know if 100% processor load on one core is normal for bitcoin under these circumstances? I want to be sure there is no perf regression in the ProcessMessages codepath since it drops out of the function for each message when there are multiple messages stacked.
1525@@ -1523,8 +1526,13 @@ void ThreadMessageHandler()
1526 {
1527 TRY_LOCK(pnode->cs_vRecvMsg, lockRecv);
1528 if (lockRecv)
1529+ {
1530 if (!g_signals.ProcessMessages(pnode))
1531 pnode->CloseSocketDisconnect();
1532+
1533+ if (!pnode->vRecvGetData.empty() || !pnode->vRecvMsg.empty())
1534+ fSleep = false;
My assumption 1. turns out to have been wrong. This does not actually change the behavior of the node WRT how it handles requests with multiple getdata(block) in them. It simply drops out and leaves the balance of the requests till later which will be at most 100ms later when ProcessMessages() is next called.
In https://github.com/pstratem/bitcoin/blob/3c08b7a23fe3afc8025658316b99f750c9a90fbf/src/main.cpp#L3974
0 if (!pfrom->vRecvGetData.empty())
1 ProcessGetData(pfrom);
You’ll want to add another if (!pfrom->vRecvGetData.empty()) return fOk; so that it doesn’t try to continue parsing until the previous getdata is completely parsed.
The goal here is for the loop to process 1 item from vRecvGetData and/or 1 item from vRecvMsg per peer.
Probably ProcessGetData should be moved outside of ProcessMessages entirely, but that is outside the scope of this patch (I think).
ACK on approach and implementation. Haven’t tested.
Some commits can be squashed together. In particular 3rd, which is a bugfix for the second.
There are only 2 paths where ProcessMessages can run without consuming a message in vRecvMsg.
Either the send buffer is full or the next message in the queue is not complete.
I agree that a more general solution should be applied, but probably later as part of a larger reorganization of the networking code.
Tested on OSX, behaves properly.
Message-handling should be eventually be reworked to use boost:asio (never sleep, be I/O event driven as @gmaxwell suggests) and to use some sort of active queue management to mitigate denial-of-service attacks.
But this code is better than what we’ve got, so I am going to merge.
Ah, I already had a similar change in my bitcoind for a year or so (though in my case, I simply reduced this 100ms to 10ms). I guess that’s why blockchain.info always picked up so much stuff. I thought it may be because of the 20ms latency..
It would explain the four blocks picked up by blockchain.info from 198.12.127.2, which I’m running w/ similar settings (just sending out far fewer transactions).. didn’t think it’d have such an impact
3934@@ -3929,6 +3935,8 @@ bool ProcessMessages(CNode* pfrom)
3935
3936 if (!fRet)
3937 LogPrintf("ProcessMessage(%s, %u bytes) FAILED\n", strCommand.c_str(), nMessageSize);
3938+
3939+ break;
3940 }