Skip to content

Conversation

@alu
Copy link
Contributor

@alu alu commented Feb 20, 2018

Support TCP_NODELAY for performance improvement.

I tested performance in my environment, but that was not well.
It was caused by TCP_NODELAY being disabled.

My test code is here

#[test]
fn memcache_bench() {
    extern crate memcache;

    let mut conn = memcache::Client::new("memcache://localhost:20343").unwrap();

    conn.set_nodelay(true).unwrap();
    let start = ::std::time::Instant::now();
    for _ in 0..1000 {
        let _: Option<String> = conn.get("foo").unwrap();
    }
    let elapsed = start.elapsed();
    println!(
        "Elapsed (nodelay enabled): {}.{}",
        elapsed.as_secs(),
        elapsed.subsec_nanos() / 1_000_000
    );

    conn.set_nodelay(false).unwrap();
    let start = ::std::time::Instant::now();
    for _ in 0..1000 {
        let _: Option<String> = conn.get("foo").unwrap();
    }
    let elapsed = start.elapsed();
    println!(
        "Elapsed (nodelay disabled): {}.{}",
        elapsed.as_secs(),
        elapsed.subsec_nanos() / 1_000_000
    );
}

And results is

$ cargo test memcache_bench --release --lib -- --nocapture
    Finished release [optimized] target(s) in 0.0 secs
     Running *****-3c8e21312813a3b5

running 1 test
Elapsed (nodelay enabled): 0.47
Elapsed (nodelay disabled): 44.163
test pool::tests::memcache_bench ... ok

Thanks.

@coveralls
Copy link

coveralls commented Feb 20, 2018

Coverage Status

Coverage remained the same at 0.0% when pulling bcabf52 on alu:master into 8a1e3e1 on aisk:master.

@aisk
Copy link
Owner

aisk commented Feb 22, 2018

This is awesome and I think we should merge this 👍

But I think we should do a small change for this PR. The memcache::Client.set_nodelay function can be called whether there was some requests to memcached on the fly, and I think this will make some unexceptable behaviors. So I think we should just let users set this options when they initialize the memcache client, and can't set back after that.

So can you modify this PR, and add a new new_with_options method to the memcache::Client, and this method will receive a memcache::Client::Options struct to indicate this?

@alu
Copy link
Contributor Author

alu commented Feb 22, 2018

@aisk Thank you for review, i changed the implement.

But memcache::connection::Connection::set_nodelay() is still public, i think it is not the best.

And there are some problems.

  1. The function will be called from outside of this crate.
  2. There is case that is unix and tcp connection mixed.
  3. The option is not for Client, it's for connection.

So, I thinks pass options by query is more better such as Client::new("memcache://localhost:112211/?tcp_nodelay=true").

How dou you think it?

@aisk
Copy link
Owner

aisk commented Feb 22, 2018

@alu Good idea!

@alu
Copy link
Contributor Author

alu commented Feb 22, 2018

@aisk I tried it, please review again.

let stream = TcpStream::connect(addr.clone())?;
let tcp_nodelay = addr.query_pairs()
.find(|&(ref k, ref v)| {
k == &Cow::Borrowed("tcp_nodelay") && v == &Cow::Borrowed("true")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can compare them by k == "tcp_nodelay" since Cow have implemented the Eq trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed it.

@aisk
Copy link
Owner

aisk commented Feb 22, 2018

Sorry for another thing, I think we can add a simple test at the end of connection.rs like this:

#[cfg(test)]
mod tests {
    #[test]
    fn tcp_nodelay() {
        super::Connection::connect("memcache://localhost:12345?tcl_nodelay=true").unwrap();
    }
}

@alu
Copy link
Contributor Author

alu commented Feb 22, 2018

@aisk Added the test, thanks.

@aisk
Copy link
Owner

aisk commented Feb 22, 2018

Merged, thank you @alu ! I will release a new version now.

@aisk aisk merged commit 5fe0197 into aisk:master Feb 22, 2018
@alu
Copy link
Contributor Author

alu commented Feb 22, 2018

@aisk Thank!

I did not rebase for cleanup commits, it's ok?

@aisk
Copy link
Owner

aisk commented Feb 22, 2018

@alu No problem, I've merged this PR using squash merge.

0.7.0 is released, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants