Skip to content

Conversation

gmaxwell
Copy link
Contributor

Nagle appears to be a significant contributor to latency now that the static
sleeps are gone. Most of our messages are relatively large compared to
IP + TCP so I do not expect this to create enormous overhead.

This may also reduce traffic burstyness somewhat.

@jgarzik
Copy link
Contributor

jgarzik commented Oct 21, 2015

ut ACK

Needs some measuring - I do not expect any overhead, given that messages are composed and then sent to write(2), as much as write(2) will swallow. Should create packets as large as path MTU and conditions will permit.

@gmaxwell
Copy link
Contributor Author

Gavin did a test which was basically the right test for measuring the performance impact, I pinged him and asked him to try this.

We do have some messages that are fairly small-- things like pings. There will be some amplification as a result, but I doubt it will be much. I'll see if I can figure out a setup for measuring that.

@ptschip
Copy link
Contributor

ptschip commented Oct 22, 2015

I think you'll also need it in SocketSendData:

   -int nBytes = send(pnode->hSocket, &data[pnode->nSendOffset], data.size() - pnode->nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT);
   +int nBytes = send(pnode->hSocket, &data[pnode->nSendOffset], data.size() - pnode->nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT | TCP_NODELAY);

@sdaftuar
Copy link
Member

I ran my test (similar to gavin's, testing block relay at the tip on regtest) and this brings the time from receiving the inv to receiving the block down to (just over) the round-trip time between the peers.

@gmaxwell
Copy link
Contributor Author

@ptschip I don't believe so. Have a citation?

@ptschip
Copy link
Contributor

ptschip commented Oct 22, 2015

@gmaxwell I think what I was thinking of was the outgoing connections also need it. I didn't see you had that already covered. My eyes are a bit blurry tonight.

Looks good.

@TheBlueMatt
Copy link
Contributor

Concept ACK.

@theuni
Copy link
Member

theuni commented Oct 22, 2015

@gmaxwell Are you sure this option is inherited by accept()ed sockets?

@laanwj
Copy link
Member

laanwj commented Oct 22, 2015

@gmaxwell Are you sure this option is inherited by accept()ed sockets?

Is TCP_NODELAY inherited from listening sockets on your platform? seems to indicate "yes" here, but I'm not sure either that's a safe assumption cross-platform.
Let's add it to accepted sockets too just to be sure.

@gavinandresen
Copy link
Contributor

Tested ACK, on OSX and Linux.

Decreases a 4-hop cross-country-twice block relay from 1.3 seconds to 0.7 seconds.

@theuni
Copy link
Member

theuni commented Oct 22, 2015

@laanwj nice link. Interestingly, osx (here, anyway) inherits TCP_NODELAY, but not O_NONBLOCK! That seems suspicious, as I assume that would be pretty obvious. I'll investigate separately.

Nagle appears to be a significant contributor to latency now that the static
 sleeps are gone.  Most of our messages are relatively large compared to
 IP + TCP so I do not expect this to create enormous overhead.

This may also reduce traffic burstyness somewhat.
@gmaxwell
Copy link
Contributor Author

Updated to set it on the accepted sockets as well, "just in case".

@btcdrak
Copy link
Contributor

btcdrak commented Oct 23, 2015

ACK

@laanwj
Copy link
Member

laanwj commented Oct 23, 2015

utACK

@laanwj laanwj merged commit a4e28b3 into bitcoin:master Oct 23, 2015
laanwj added a commit that referenced this pull request Oct 23, 2015
a4e28b3 Set TCP_NODELAY on P2P sockets. (Gregory Maxwell)
@laanwj laanwj added the P2P label Oct 23, 2015
gmaxwell added a commit that referenced this pull request Oct 23, 2015
Nagle appears to be a significant contributor to latency now that the static
 sleeps are gone.  Most of our messages are relatively large compared to
 IP + TCP so I do not expect this to create enormous overhead.

This may also reduce traffic burstyness somewhat.

Conflicts:
	src/net.cpp

Rebased-From: a4e28b3
Github-Pull: #6867
gmaxwell added a commit that referenced this pull request Oct 23, 2015
Nagle appears to be a significant contributor to latency now that the static
 sleeps are gone.  Most of our messages are relatively large compared to
 IP + TCP so I do not expect this to create enormous overhead.

This may also reduce traffic burstyness somewhat.

Conflicts:
	src/net.cpp

Rebased-From: a4e28b3
Github-Pull: #6867
Infernoman pushed a commit to Crowndev/crown-core that referenced this pull request Jan 25, 2017
Nagle appears to be a significant contributor to latency now that the static
 sleeps are gone.  Most of our messages are relatively large compared to
 IP + TCP so I do not expect this to create enormous overhead.

This may also reduce traffic burstyness somewhat.

Conflicts:
	src/net.cpp

Rebased-From: a4e28b3
Github-Pull: bitcoin#6867
reddink pushed a commit to reddcoin-project/reddcoin-3.10 that referenced this pull request May 27, 2020
Nagle appears to be a significant contributor to latency now that the static
 sleeps are gone.  Most of our messages are relatively large compared to
 IP + TCP so I do not expect this to create enormous overhead.

This may also reduce traffic burstyness somewhat.

Conflicts:
	src/net.cpp

Rebased-From: a4e28b3
Github-Pull: bitcoin#6867
(cherry picked from commit 5297194)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants