Skip to content

Network optimalizations #2409

Merged
merged 6 commits into from Mar 31, 2013

6 participants

@sipa
Bitcoin member
sipa commented Mar 24, 2013

This pull request contains:

  • The commits from #2016 and #2017
  • Fixes and memory improvements for those
  • A change from per-connection to per-message send buffers
  • A change to break processing of getdata into several stages, when the send buffer fills up

This should improve network latency and memory usage.

@jgarzik
Bitcoin member
jgarzik commented Mar 24, 2013

ACK

@BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3987a1cabbdd263dfa7bed92b7326274bc625d09 for binaries and test log.

@sipa
Bitcoin member
sipa commented Mar 25, 2013

Don't merge this yet; an instance of this segfaulted for me.

@sipa
Bitcoin member
sipa commented Mar 28, 2013

Ok, fixed an obscure bug: ProcessMessages (while holding cs_vRecvMsg) sometimes calls PushMessage, which since Jeff's patches sometimes calls SocketSendData, which can fail and cause the connection to be closed. Closing the connection however wipes vRecvMsg... while ProcessMessages is still iterating over it, invalidating its iterator, and eventually trying to remove the entries it already processed, causing a double free.

Seems to be running smoothly now.

Jeff Garzik and others added some commits Nov 15, 2012
Jeff Garzik P2P, cosmetic: break out buffer send(2) code into separate function bc2f5aa
Jeff Garzik P2P: improve RX/TX flow control
1) "optimistic write": Push each message to kernel socket buffer immediately.

2) If there is write data at select time, that implies send() blocked
   during optimistic write.  Drain write queue, before receiving
   any more messages.

This avoids needlessly queueing received data, if the remote peer
is not themselves receiving data.

Result: write buffer (and thus memory usage) is kept small, DoS
potential is slightly lower, and TCP flow control signalling is
properly utilized.

The kernel will queue data into the socket buffer, then signal the
remote peer to stop sending data, until we resume reading again.
b9ff297
Jeff Garzik P2P: parse network datastream into header/data components in socket t…
…hread

Replaces CNode::vRecv buffer with a vector of CNetMessage's.  This simplifies
ProcessMessages() and eliminates several redundant data copies.

Overview:

* socket thread now parses incoming message datastream into
  header/data components, as encapsulated by CNetMessage
* socket thread adds each CNetMessage to a vector inside CNode
* message thread (ProcessMessages) iterates through CNode's CNetMessage vector

Message parsing is made more strict:

* Socket is disconnected, if message larger than MAX_SIZE
  or if CMessageHeader deserialization fails (latter is impossible?).
  Previously, code would simply eat garbage data all day long.
* Socket is disconnected, if we fail to find pchMessageStart.
  We do not search through garbage, to find pchMessageStart.  Each
  message must begin precisely after the last message ends.

ProcessMessages() always processes a complete message, and is more efficient:

* buffer is always precisely sized, using CDataStream::resize(),
  rather than progressively sized in 64k chunks.  More efficient
  for large messages like "block".
* whole-buffer memory copy eliminated (vRecv -> vMsg)
* other buffer-shifting memory copies eliminated (vRecv.insert, vRecv.erase)
607dbfd
@sipa sipa Some fixes to CNetMessage processing
* Change CNode::vRecvMsg to be a deque instead of a vector (less copying)
* Make sure to acquire cs_vRecvMsg in CNode::CloseSocketDisconnect (as it
  may be called without that lock).
967f245
@sipa sipa Use per-message send buffer, rather than per connection 41b052a
@sipa
Bitcoin member
sipa commented Mar 29, 2013

Added a commit, which breaks up the processing of getdata requests when the send buffer size limit is exceeded. This results in far smaller send buffers being allocated for peers downloading blocks from us, and doesn't seem to have a noticable impact on performance/bandwidth (I can still download blocks from my VPS running this, at ~6 MiB/s, same as before).

@BitcoinPullTester

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/e96ba341587ebdb0583ce3cd0567411164f1ec44 for binaries and test log.

This could happen for one of several reasons:
1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
4. The test suite fails on either Linux i386 or Win32
5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This is an automated test script which runs test cases on each commit every time is updated.
It, however, dies sometimes and fails to test properly, if you are waiting on a test, please check timestamps and if the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
and contact BlueMatt on freenode if something looks broken.

@sipa
Bitcoin member
sipa commented Mar 30, 2013

@TheBlueMatt can you put a link to the patches applied for pulltester in the produced message?

@sipa sipa Process getdata invs separately until send buffer overflows
There exists a per-message-processed send buffer overflow protection,
where processing is halted when the send buffer is larger than the
allowed maximum.

This protection does not apply to individual items, however, and
getdata has the potential for causing large amounts of data to be
sent. In case several hundreds of blocks are requested in one getdata,
the send buffer can easily grow 50 megabytes above the send buffer
limit.

This commit breaks up the processing of getdata requests, remembering
them inside a CNode when too many are requested at once.
c7f039b
@BitcoinPullTester

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/c7f039b674b43b741f20bf7521eb8a68426f4275 for binaries and test log.

This could happen for one of several reasons:
1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
4. The test suite fails on either Linux i386 or Win32
5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This is an automated test script which runs test cases on each commit every time is updated.
It, however, dies sometimes and fails to test properly, if you are waiting on a test, please check timestamps and if the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
and contact BlueMatt on freenode if something looks broken.

@mikehearn

I'm testing this+Matts patches now - 231 connections with only 193mb RSS. Pretty fantastic results. Before this my node was running out of memory in less than 24 hours.

@TheBlueMatt

This plus other recent network pulls has been stable for many hours under high load (150-200 connections).

ACK

@jgarzik jgarzik merged commit e3c063b into bitcoin:master Mar 31, 2013
@Diapolo
Diapolo commented Apr 2, 2013

Is there a 0.8.2 planned in the near future? What will be in, was that yet decided?

@sipa sipa deleted the sipa:txoptim branch May 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.