Disable Nagle's algorithm. #256
Conversation
|
@ejona86 Please take a look. |
|
Can we investigate where the double write that is causing trouble is coming from? Netty doesn't suffer from it at present, so it would be good to know whether it is a OkHttp problem that we should investigate fixing or whether Netty just hasn't triggered it yet. I could totally believe we would want to disable Nagle's for both transports, but I also expect our current flushing behavior in OkHttp would produce really poor results on the wire if Nagle's is off. Right now we do a lot of flushes, in both OutboundFlowController and OkHttp's Http2.Writer. Those all make sense, but if we are disabling Nagle they could each turn into their own TCP segment and cause a lot of waste. |
|
It came from the data message and the halfClose. In Stubby bi-directional streaming, we provided an API sendAndHalfClose() to optimize such scenario. I think Netty already disabled Nagle's, see https://github.com/netty/netty/search?utf8=%E2%9C%93&q=settcpnodelay&type=Code. I agree disabling Nagle's would increase waste, but I think Netty has the same problem, I'll send you a tcpdump file, which shows Netty put each data frame into its own TCP segment. |
|
@madongfly, okay, let's enable it. LGTM |
|
I just noticed netty/netty#939, which confirms Netty disabled Nagle's, but it also says disabling Nagle's may hurt Android, I'll consult Android guys. |
…chmark, the streamingInputCall() method happened to have the write-write-read pattern which added 40ms extra delay. According to our micro benchmark, this change improves the performance, see https://screenshot.googleplex.com/cfxjPGxkgF.png for the previous results, there were 4 out of 6 scenarios had higher latency than netty transport. https://screenshot.googleplex.com/Rq92tYMvBE.png shows the results after this change, the latencies in all scenarios are less than netty transport. This commit fixes grpc#60.
2 participants
This was exposed by our internal micro benchmark, the streamingInputCall() method happened to have the write-write-read pattern which added 40ms extra delay.
According to our micro benchmark, this change improves the performance, see https://screenshot.googleplex.com/cfxjPGxkgF.png for the previous results, there were 4 out of 6 scenarios had higher latency than netty transport.
https://screenshot.googleplex.com/Rq92tYMvBE.png shows the results after this change, the latencies in all scenarios are less than netty transport.
This commit fixes #60.