Repository: blackbeam/rust-mysql-simple (language: rust)

Issue:
Preparing statements without socket connection is very slow
Consider the following program which just prepares a lot of queries in a loop (the statement cache is turned off to force the prepare so we can benchmark it):
```rust
extern crate mysql;
fn main() {
    let mut opts = mysql::OptsBuilder::from_opts("mysql://localhost");
    opts.stmt_cache_size(0);
    opts.prefer_socket(false);
    let mut conn = mysql::Conn::new(opts).unwrap();

    loop {
        let start = ::std::time::Instant::now();
        conn.prepare("SELECT 1").unwrap();
        println!("{:?}", start.elapsed());
    }
}
```
The prepare seems to always take about **40ms**, while the CPU of both the server and the client sits idle. The client is usually waiting in:
```
(gdb) bt
#0  0x00007f87a6372dbd in recv () from /usr/lib/libpthread.so.0
#1  0x000056109e547821 in std::sys::imp::net::Socket::recv_with_flags () at /checkout/src/libstd/sys/unix/net.rs:231
#2  std::sys::imp::net::Socket::read () at /checkout/src/libstd/sys/unix/net.rs:240
#3  std::sys_common::net::TcpStream::read () at /checkout/src/libstd/sys_common/net.rs:248
#4  std::net::tcp::{{impl}}::read () at /checkout/src/libstd/net/tcp.rs:549
#5  0x000056109e4ca27a in <bufstream::BufStream<S> as std::io::Read>::read ()
#6  0x000056109e4c6175 in std::io::Read::read_exact ()
#7  0x000056109e4d21c0 in mysql::conn::Conn::read_packet ()
#8  0x000056109e4d4aed in mysql::conn::Conn::_prepare ()
#9  0x000056109e4c46d9 in mysql_rs_issue::main ()
```
In contrast, using `query` (instead of `prepare`) takes ~30**µ**s. That's a three orders of magnitude difference. Surely that isn't expected behavior?

This issue mirrors that exhibited in https://github.com/amphp/mysql/issues/55, so this may be a MySQL/MariaDB server issue, not a client issue, but it is not yet clear. It could also be some kind of subtle protocol mismatch that causes the server to wait for more bytes from the client?


Discussion / hints:
Hi!
That's quite interesting.

I'm on `mysql  Ver 14.14 Distrib 5.7.17, for macos10.12 (x86_64)` and can't reproduce this issue using your code. `query` is strictly under 1ms for me and `prepare` some times takes over 1ms but no more than 2ms.

However it is reproducable on my linux box with `mysql  Ver 15.1 Distrib 10.1.30-MariaDB, for Linux (x86_64)`. First prepare takes ~300µs and all subsequent is near 40ms.

I'll investigate further.
Oh, that's interesting! Can you reproduce with MySQL (i.e., not MariaDB) on Linux, if you have access to that? What about MariaDB on macOS?
I can reproduce with Cloud SQL on GCP, running fairly stock MySQL 5.7
I've done some more digging on this in a slightly different context, and found that this kind of 40ms delay can generally be directly attributed to Nagle's algorithm (the `TCP_NODELAY` option), and particularly comes to bite you when requests or responses are small. There's a more elaborate explanation of exactly why this happens in https://github.com/mit-pdos/distributary/commit/be9d5ab8ce8375f8d887a7b62c8aef54a277e56d, but suffice to say that Nagle's algorithm on Linux will generally add exactly 40ms of delay. So it seems like this might be the culprit.

The way to fix this would likely be to always set `TCP_NODELAY` in the library, which should work well if you are using a `BufWriter` or generally avoid doing many small writes to the socket. If you don't, then you could make it optional by adding is an an option to `Opts`.

I haven't yet had a chance to test out whether this fixes the issue outlined above, but will give it a shot once I have the time.
I can confirm that the following diff reduces the latency of the original code from 40ms per prepare to 20µs per prepare:
```diff
diff --git a/src/io/tcp.rs b/src/io/tcp.rs
index a33ceef..19f264b 100644
--- a/src/io/tcp.rs
+++ b/src/io/tcp.rs
@@ -115,6 +115,7 @@ impl<T: ToSocketAddrs> MyTcpBuilder<T> {
                 stream.set_read_timeout(read_timeout)?;
                 stream.set_write_timeout(write_timeout)?;
                 stream.set_keepalive_ms(keepalive_time_ms)?;
+                stream.set_nodelay(true)?;
                 Ok(stream)
             })
     }
```

Fix patch:
diff --git a/src/conn/mod.rs b/src/conn/mod.rs
index 297a8d6b..17fcad05 100644
--- a/src/conn/mod.rs
+++ b/src/conn/mod.rs
@@ -722,6 +722,7 @@ impl Conn {
         let read_timeout = self.opts.get_read_timeout().clone();
         let write_timeout = self.opts.get_write_timeout().clone();
         let tcp_keepalive_time = self.opts.get_tcp_keepalive_time_ms().clone();
+        let tcp_nodelay = self.opts.get_tcp_nodelay();
         let tcp_connect_timeout = self.opts.get_tcp_connect_timeout();
         let bind_address = self.opts.bind_address().cloned();
         let stream = if let Some(ref socket) = *self.opts.get_socket() {
@@ -733,6 +734,7 @@ impl Conn {
                                 read_timeout,
                                 write_timeout,
                                 tcp_keepalive_time,
+                                tcp_nodelay,
                                 tcp_connect_timeout,
                                 bind_address)?
         } else {
diff --git a/src/conn/opts.rs b/src/conn/opts.rs
index 3d28b1d9..f09778f1 100644
--- a/src/conn/opts.rs
+++ b/src/conn/opts.rs
@@ -64,6 +64,12 @@ pub struct Opts {
     /// connection to `127.0.0.1` if `true`.
     prefer_socket: bool,
 
+    /// Whether to enable `TCP_NODELAY` (defaults to `true`).
+    ///
+    /// This option disables Nagle's algorithm, which can cause unusually high latency (~40ms) at
+    /// some cost to maximum throughput. See #132.
+    tcp_nodelay: bool,
+
     /// TCP keep alive time for mysql connection.
     tcp_keepalive_time: Option<u32>,
 
@@ -195,6 +201,11 @@ impl Opts {
         self.verify_peer = val;
     }
 
+    /// Whether `TCP_NODELAY` will be set for mysql connection.
+    pub fn get_tcp_nodelay(&self) -> bool {
+        self.tcp_nodelay
+    }
+
     /// TCP keep alive time for mysql connection.
     pub fn get_tcp_keepalive_time_ms(&self) -> Option<u32> {
         self.tcp_keepalive_time
@@ -240,6 +251,7 @@ impl Default for Opts {
             verify_peer: false,
             ssl_opts: None,
             tcp_keepalive_time: None,
+            tcp_nodelay: true,
             local_infile_handler: None,
             tcp_connect_timeout: None,
             bind_address: None,
@@ -338,6 +350,15 @@ impl OptsBuilder {
         self
     }
 
+    /// Set the `TCP_NODELAY` option for the mysql connection (defaults to `true`).
+    ///
+    /// Setting this option to false re-enables Nagle's algorithm, which can cause unusually high
+    /// latency (~40ms) but may increase maximum throughput. See #132.
+    pub fn tcp_nodelay(&mut self, nodelay: bool) -> &mut Self {
+        self.opts.tcp_nodelay = nodelay;
+        self
+    }
+
     /// Prefer socket connection (defaults to `true`). Available as `prefer_socket` url parameter
     /// with value `true` or `false`.
     ///
diff --git a/src/io/mod.rs b/src/io/mod.rs
index f6773737..974cccae 100644
--- a/src/io/mod.rs
+++ b/src/io/mod.rs
@@ -384,6 +384,7 @@ impl Stream {
                        read_timeout: Option<Duration>,
                        write_timeout: Option<Duration>,
                        tcp_keepalive_time: Option<u32>,
+                       nodelay: bool,
                        tcp_connect_timeout: Option<Duration>,
                        bind_address: Option<SocketAddr>) -> MyResult<Stream>
     {
@@ -393,6 +394,7 @@ impl Stream {
             .read_timeout(read_timeout)
             .write_timeout(write_timeout)
             .keepalive_time_ms(tcp_keepalive_time)
+            .nodelay(nodelay)
             .bind_address(bind_address);
         builder.connect()
             .map(|stream| Stream::TcpStream(Some(TcpStream::Insecure(BufStream::new(stream)))))
diff --git a/src/io/tcp.rs b/src/io/tcp.rs
index a33ceef5..2bd7456e 100644
--- a/src/io/tcp.rs
+++ b/src/io/tcp.rs
@@ -31,6 +31,7 @@ pub struct MyTcpBuilder<T> {
     read_timeout: Option<Duration>,
     write_timeout: Option<Duration>,
     keepalive_time_ms: Option<u32>,
+    nodelay: bool,
 }
 
 impl<T: ToSocketAddrs> MyTcpBuilder<T> {
@@ -39,6 +40,11 @@ impl<T: ToSocketAddrs> MyTcpBuilder<T> {
         self
     }
 
+    pub fn nodelay(&mut self, nodelay: bool) -> &mut Self {
+        self.nodelay = nodelay;
+        self
+    }
+
     pub fn write_timeout(&mut self, write_timeout: Option<Duration>) -> &mut Self {
         self.write_timeout = write_timeout;
         self
@@ -69,6 +75,7 @@ impl<T: ToSocketAddrs> MyTcpBuilder<T> {
             read_timeout: None,
             write_timeout: None,
             keepalive_time_ms: None,
+            nodelay: true,
         }
     }
 
@@ -80,6 +87,7 @@ impl<T: ToSocketAddrs> MyTcpBuilder<T> {
             read_timeout,
             write_timeout,
             keepalive_time_ms,
+            nodelay,
         } = self;
         let err_msg = if bind_address.is_none() {
             "could not connect to any address"
@@ -115,6 +123,7 @@ impl<T: ToSocketAddrs> MyTcpBuilder<T> {
                 stream.set_read_timeout(read_timeout)?;
                 stream.set_write_timeout(write_timeout)?;
                 stream.set_keepalive_ms(keepalive_time_ms)?;
+                stream.set_nodelay(nodelay)?;
                 Ok(stream)
             })
     }


Test patch: