Add in support for enabling tcp::no_delay and QUICKACK on both client and listener sockets

[mobileben]

Some applications will benefit from being able to enable tcp::no_delay as well as QUICKACK within both the client (http_client_asio.cpp) and listener (http_server_asio.cpp).

I have previously opened #1189. And I think this should potentially address #468 (This actually does mention enabling tcp::no_delay by someone recently.

I was previously experiencing horrendous RTT (roughly 100ms - 400ms+). Applying this on the client (both client and server use cpprestsdk) saw an improvement where now the average is 70ms RTT. I am still doing some experiments to see why this isn't lower, but this is a marked improvement because server profiling showed I was being blocked and hence "server CPU time" was more or less the RTT. Now, the "server CPU time" is roughly 4ms. So that in itself is dramatic.

Note that from my initial testing, the client side seems more important. Yet I'm still also indicating we allow this enabled on the listeners as well. I am by no means claiming to be an expert on this. I am only suggesting the ability to enable on the listener for parity and to allow for easier experimentation.

Ideally I wanted to create a wrapper handler in asio_connection's async_connect. This way anyone calling async_connect would have this automatically applied. However I could not get the syntax correct, so I ended up putting the code directly in the calling functions' handlers. Does anyone know how to properly create an "intermediary" handler so this could be more cleanly done?

I have a branch with my changes. Note this forces it into this mode. The clean solution is to make this optional. But if anyone wants to test for timings to help see if this reduces some performance issues, that would be awesome.

https://github.com/mobileben/cpprestsdk/tree/nagle-quickack

Note the "nagle" in the branch name is a little misleading. This ends up disabling Nagle by enabling tcp::no_delay.

[garethsb]

👍 Will definitely try this out, thanks for posting the branch.

[mobileben]

@garethsb-sony you ever get a chance to experiment some more with that? I just did a recent push. I've discovered that TCP_QUICKACK needs to be re-applied after reads since the state doesn't persist. I actually now have an Ubuntu 16.04 that is 25-28ms RTT on average versus the 40ms+ people were seeing. I am going to make some more experiments. So getting an extra set of eyes would be helpful. I will add my details to #468 later.

One thing I can't figure out is localhost, however, while faster, still suffers. it is a little faster than hitting my AWS instance from my desktop. Surprising. I am comparing it against the simple "Hello world!" golang which is smidge faster from desktop to instance, but 3X faster on localhost.

[garethsb]

Sorry, @mobileben, have been tied up with other things. As you've said, cpprestsdk does well at "convenient enough to use", but there's no doubt that analysis of its performance bottlenecks as you've been doing must show places where it could be improved significantly. With regard to nodelay and quickack specifically, I'm no expert, but unlike fixing any fundamental issues with use/implementation of pplx, I guess this part of performance tuning should take account of different request/response sizes and number/frequency of req/res per connection, number of simultaneous clients, etc.

[mobileben]

No worries. TCP_QUICKACK and tcp:no_delay are definitely usage dependent. I think I have a better pulse on how to make a wrapper function and may create a PR for this. The idea is to make it optional (off by default) so it can be turned on.

The performance issues are pretty bad on Ubuntu/Linux. I'm guessing at this point Microsoft really doesn't care and/or are not capable of fixing this, which is curious as they are supposed to be in the mode where they want to be on all platforms. They seem enamored with pplx usage which unfortunately seems like one of the sources for the bottlenecks.

Sadly I'll have to find the time to find a replacement or build my own, which I really don't want to do. I had planned on doing a game that uses web sockets, but it's clear that cppsdk would not be a good choice for this given the track record.

[garethsb]

OK, it's taken me too long to get to this, but I can now confirm I see similar behaviour.

I make a sequence of POST requests with bodies of approx 100B-1kB (i.e. less than one MTU) from a cpprestsdk-based client to a cpprestsdk-based server on the same Linux (Ubuntu 14.04) virtual machine.

With cpprestsdk master, each request takes ~40ms before the body is available to the listener/handler.

With just the proposed change to set TCP_NODELAY in asio_context::handle_connect, this is reduced to ~1ms, and round-trip time is <2ms.

FWIW, even with cpprestsdk master, POST requests with no body also take <2ms RTT.

This does seem to demonstrate the conflict between Nagle and Delayed Ack is the largest part of the performance problem with the Boost.Asio-based implementation.

I wonder if similar improvement might be demonstrated if the separate calls to async_write for the headers and body were coalesced if the body was fully available at the point that the request headers are being written?

[garethsb]

@mobileben @BillyONeal
I've had confirmation from a user that a sequence of network operations in their environment goes from 12s to 1.3s with the patch below, so I'd really like a way to enable this.

Is it the case that always disabling Nagle for http_client is unlikely to have adverse side-effects because the async_write calls are as large as possible?

I had hoped that http_client_config::set_nativehandle_options might allow a user to do this from outside C++ REST SDK, but it's intentionally called before the socket is connected. So unless we add a second callback hook, I don't see how.

diff --git a/Release/src/http/client/http_client_asio.cpp b/Release/src/http/client/http_client_asio.cpp
index a90ca6a..ad4238a 100644
--- a/Release/src/http/client/http_client_asio.cpp
+++ b/Release/src/http/client/http_client_asio.cpp
@@ -330,6 +330,12 @@ public:

     void start_reuse() { m_is_reused = true; }

+    void enable_no_delay()
+    {
+        boost::asio::ip::tcp::no_delay option(true);
+        m_socket.set_option(option);
+    }
+
 private:
     // Guards concurrent access to socket/ssl::stream. This is necessary
     // because timeouts and cancellation can touch the socket at the same time
@@ -610,6 +616,7 @@ public:
             if (!ec)
             {
                 m_context->m_timer.reset();
+                m_context->m_connection->enable_no_delay();
                 m_context->m_connection->async_write(m_request,
                                                      boost::bind(&ssl_proxy_tunnel::handle_write_request,
                                                                  shared_from_this(),
@@ -1006,6 +1013,7 @@ private:
         m_timer.reset();
         if (!ec)
         {
+            m_connection->enable_no_delay();
             write_request();
         }
         else if (ec.value() == boost::system::errc::operation_canceled ||

[garethsb]

@BillyONeal, would you accept a PR with the above simple patch for inclusion in master and a v2.10.15 release?

[BillyONeal]

I'm a bit uncomfortable with adding knobs that aren't respected by at least the asio backend and WinHTTP backend, as one of the most confusing aspects of cpprestsdk right now is that some options are silently ignored by many of the backends and this makes that problem worse.

I'm not opposed to providing the knob in principle :)

[mobileben]

@garethsb-sony sorry for the delay in getting back. From what I could tell, it should not have adverse effects, but then again, it does seem with things like this, it becomes a bit more of a trial and error sort of thing. Many of the articles I read sort of listed out things to try out when you had high delays.

@BillyONeal what are the thoughts on making this a compile flag to enable versus always active? One main issue I see right now with cpprestsdk is the performance isn't very good. You definitely get improvement with tcp::no_delay, but I've documented in some of the issues various timings.

#468 (comment)
#1189
#1206

I get that the goal of the project is more for making it easier to build a service, but people that are using C++ do probably want some performance as well...

[garethsb]

Yes, with my cut down patch above, I'm not proposing a knob, I'm proposing an always-on change. For HTTP, it seems reasonable, and I suspect is what WinHTTP already does under the hood.

[BillyONeal]

One main issue I see right now with cpprestsdk is the performance isn't very good.

Yeah I don't think we've ever done a serious perf audit here. Part of it is that we know of serious perf issues that would be API breaking to address, e.g. exposing const&s from the URI builder. Maybe something for a hypothetical v3 if we do that.

I'm proposing an always-on change.

That makes sense to me, though it would be helpful if you can characterize the perf consequences (what scenarios get better, what ones are likely to get worse).

[garethsb]

Part of it is that we know of serious perf issues that would be API breaking to address, e.g. exposing const&s from the URI builder.

I haven't profiled it, but that seems like an example of a micro-perf improvement, whereas things like TCP_NODELAY that directly affect the network stack behaviour can make a significant difference - a factor of 10 in system level performance in my customer's use case!

That makes sense to me, though it would be helpful if you can characterize the perf consequences (what scenarios get better, what ones are likely to get worse).

Thanks. There's a good summary of TCP_NODELAY here: best practices

By default, Nagle's algorithm and Delayed ACK are broadly implemented across networks, including the internet. Nagle's algorithm effectively only allows one packet to be actively transporting on the network at any given time, this tends to hold back traffic due to the interactions between the Nagle's algorithm and delayed ACKs. Hence Nagle's algorithm is undesirable in highly interactive environments.

For example: Delayed ACK tries to send more data per segment if it can. But part of Nagle's algorithm depends on an ACK to send data. Nagle's algorithm and Delayed ACKs together create a problem because Delayed ACKs are waiting around to send the ACK while Nagle's is waiting around to receive the ACK! This creates random stalls of 200-500ms on segments that could otherwise be sent immediately and delivered to the receive-side stack and apps above it.

The typical example given for the problem caused by Nagle's algorithm (and its interaction with delayed ACK) is interactive applications that really do benefit by immediately sending a single key-press or mouse-movement despite the additional packet-header overhead of sending such incomplete packets.

That doesn't sound much like our application (C++ REST SDK http_client) but it applies because the HTTP request headers and body are written separately, and the headers of a typical request are more than likely not to fill a packet. Without disabling Nagle, the client has to wait for an ACK on the headers packet before transmitting the body.

Since C++ REST SDK implementation controls how headers are written (in a single operation), I can't at the moment see a downside to making this change.