TimonPost / laminar

A simple semi-reliable UDP protocol for multiplayer games
817 stars 66 forks source link

Use a thread pool in TcpListener. #46

Closed CBenoit closed 5 years ago

CBenoit commented 5 years ago

(probably 0.2.0 milestone)

A thread pool or some other way of limiting thread creation should be used here: https://github.com/amethyst/laminar/blob/master/src/net/tcp.rs#L200

We briefly discussed about it with @fhaynes on the Discord. You were thinking about using https://github.com/nathansizemore/epoll ? I see several issues with using this. First, as you said, the point is to use only one thread (epoll is basically about monitoring multiple file descriptors to see whether I/O is possible on any of them). So there is no true parallelism. Second, this use Unix specific API that is not portable. We want laminar to run on windows, right?

What I suggest is to really use a thread pool. A basic one can be done really simply (like discussed the Rust book: https://doc.rust-lang.org/stable/book/2018-edition/ch20-03-graceful-shutdown-and-cleanup.html ) Or we could use another crate. Rayon provide a thread pool: https://docs.rs/rayon/1.0.2/rayon/struct.ThreadPool.html Tokyo can also provide a thread pool and even an interface similar to epoll if you prefer a single thread (but not specific to Unix!): https://github.com/tokio-rs/tokio/tree/master/tokio-threadpool Alternatively there is this crate dedicated to thread pool: https://github.com/rust-threadpool/rust-threadpool

TcpListener should include a way to specify how many threads the thread pool should spawn if we indeed use a thread pool.

LucioFranco commented 5 years ago

As for the epoll issue, we can use MIO which should support all our target platforms.

torkleyy commented 5 years ago

Fully agree with using epoll / equivalents. Using multithreading for polling on IO is not ideal. This is exactly what those libraries are for. (As for mio, I think we might need to implement some stuff ourselves.)

torkleyy commented 5 years ago

Related discussion: https://github.com/amethyst/amethyst/issues/499#issue-277740260

LucioFranco commented 5 years ago

I believe we are planning on moving to MIO after 0.1.0 so that we can provide benchmark results to be able to compare both implementations. I personally believe that we can reduce the amount of threads and lock sharing with mio. So, I am going to mark this issue for 0.2.0, so that we can revisit.

fhaynes commented 5 years ago

Yes, mio abstracts it away. That being said, I see no problem with someone moving the current version of TCP handling to a thread pool based implementation, as long as they understand it will go away in 0.2.0.

TimonPost commented 5 years ago

Please checkout #47 for more discussions about whether TCP should be added to this crate. Feel free to add your vision. If TCP does not belong here we are done with this subject and we can move further with or original id a of building an rudp implementation.

TimonPost commented 5 years ago

We are removing TCP from the lib see PR #56