stepancheg / grpc-rust

Rust implementation of gRPC
MIT License
1.37k stars 124 forks source link

server: support multi threads #53

Closed siddontang closed 7 years ago

siddontang commented 7 years ago

Hi @stepancheg

For #27, we discuss that supporting multi thread can improve the server performance. I do a little work to verify the feasibility. I just do:

  1. Create the listener https://github.com/stepancheg/grpc-rust/blob/master/http2/src/server.rs#L83 like tokio-proto does https://github.com/tokio-rs/tokio-proto/blob/e42b3bfbf5f35402feb595363f42fcba4ef14079/src/tcp_server.rs#L152 . In unix, we can use reuse_port to let multi threads bind the same address.
  2. Add a worker size configuration in GrpcServerConf. If > 1, spawn threads, clone the HttpServerConf and service_definition, then move to the new thread to create the HttpServer. Here we only need to care arg addr whose trait is ToSocketAddrs so we must use <A: ToSocketAddrs + Clone + Send + 'static>.

It works fine, if you think this change is ok, I can send you a PR.

siddontang commented 7 years ago

/cc @overvenus

stepancheg commented 7 years ago

I'm not sure that's good idea. Mostly, because even if you spawn multiple event loop, they won't distribute load evenly, for example, it will be possible that one thread (one event loop) accepts all connections, and other threads will do nothing. That could be misleading for users.

Anyway, if you want it, I'm thinking about simpler version. AFAIU, you don't need to reuse_port, you can just create a listening socket, and clone it to multiple workers.

So, if you need it, I think GrpcServer could have additional constructor GrpcServer::with_socket(listen_socket, ...). So users of grpc-rust could create a listening socket, bind it on port, and then spawn several GrpcServer instances.

If that works for you, I will be glad to merge a PR.

siddontang commented 7 years ago

@stepancheg

I think the way with GrpcServer::with_socket(listen_socket, ...) is ok, but this may cause thundering herd problem sometimes, seem this is not a problem in our case.

/cc @overvenus

stepancheg commented 7 years ago

this may cause thundering herd problem sometimes

Isn't it the case when multiple workers listen on the same port with reuse_port?

siddontang commented 7 years ago

For reuse_port, multi workers will listen on different socket, so maybe no herd problem here. . And I even think reuse_port may distribute load evenly.

See https://lwn.net/Articles/542629/

siddontang commented 7 years ago

@stepancheg

Btw, how do you format your code, what's your rustfmt version. I try to use cargo fmt or rustfmt but find that many files changed, do you forget formatting your code?

siddontang commented 7 years ago

@stepancheg

I think it is very easy to let HttpServer using reuse_port, and we only need to change let listen = TcpListener::bind(&listen_addr, &lp.handle()).unwrap(); to the way https://github.com/stepancheg/grpc-rust/blob/master/http2/src/server.rs#L83 does.

Then we can spawn multi GrpcServer instances, no need to change other codes.

stepancheg commented 7 years ago

@siddontang yes, if you only want to add reuse_port option I'm OK with that either.

stepancheg commented 7 years ago

@siddontang

Btw, how do you format your code, what's your rustfmt version

I don't format with rustfmt. I'd love to use it, but at the moment it has a couple of issues needed to be fixed first. One of them is: https://github.com/rust-lang-nursery/rustfmt/issues/609