-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't use RwLocks #3
Comments
https://git.asonix.dog/asonix/mmo-rust-server Here's how I'd do it |
This is absolutely beautiful. I love your solution to cloning the sink, and you managed to rebuild the project with no mutexes. I can tell you're becoming very proficient in Rust. So in a nutshell, your idea is to spawn an actor in the thread pool to handle client updates synchronously in a single task. In theory this sacrifices no speed from the original program because any task that locks the state for writing blocks any other task from using the state at the same time anyway. On top of that, the design also avoids tasks blocking as they wait on another task that has locked the resource. Instead of needing the lock, a task just passes a message to the mpsc queue for the actor to process at a later point. I don't want to update this repository because it would no longer match the article. But what do you think about me doing a v2 article explaining your design? |
Go for it! I've been playing a lot with Tokio for various projects of mine, and this is the cleanest I've been able to come up with for doing asynchronous state-management with pure tokio. I do like Actix, though. |
oh also, there's part of your code that does match true {
true => Ok(Loop::Continue())
false => Ok(Loop::Break())
} but you could do Ok(Loop::Continue()) and it would do the same thing |
@asonix It makes sense that a blocking write lock could potentially be very bad for performance, but my intuition is that a RwLock is ok so long as the critical section (lifetime) is small and fast. I benchmarked the two solutions with artillery and found that the RwLock implementation actually has better performance. I am definitely a novice when it comes to benchmarking, but I played with it for a while and I'm reasonably convinced. config:
target: "ws://127.0.0.1:8080/"
phases:
- duration: 60
arrivalRate: 20
scenarios:
- engine: "ws"
flow:
- loop:
- send: "down"
- send: "right"
- send: "left"
- send: "up"
count: 10
- think: 1 Note that I commented out |
Here's the really interesting thing, though. Using artillery with your config, I'm getting opposite results from what you've shown. I'm wondering if this is an OS or CPU (or both) thing. In this screenshot, the messaging implementation is on the left, and the lock implementation is on the right. the messaging implementation performs better in the p95 and p99, and in max latency. Here's the specs for the machine I'm testing on: I've run the tests a couple times, and get similar results each time. note: Before testing each project, I've commented out all println!()s, and run my branch: https://git.asonix.dog/asonix/mmo-rust-server/src/branch/asonix/artillery |
In related news, tokio just merged some synchronization primatives: tokio-rs/tokio#839 This means that using mpsc and oneshot channels should be faster (if the tokio versions are used over the futures versions) and there's a semaphore and an AtomicTask primitive for exclusive data access. |
I think my early "scenarios" were far too simplistic to find the problems with lock contention. Originally I was testing with, I switched to testing with, and I got similar results to your own (messages win). However, as I started to increase the complexity of the benchmark I saw locks start to win, again. I really didn't want to believe locks would hurt performance that much. Mostly because reworking every shared data structure into a separate future, channel, and messages feels like cruft, but the impression started to build that while messaging might impose some constant in overhead initially it holds out under heavy load. So, to really stress the server I think we need to add more "thinks" to the script so clients hang around to receive messages. I'd be interested to see if you have the same results, but when you add the "thinks" and increase the number of clients I believe you can see the lock contention begin to cause connection timeouts and a really significant degradation in connectivity. I used a |
I'm going to be honest, I don't know enough about benchmarking and artillery to understand what adding more thinks does. The docs say "To pause the virtual user for N seconds, use a think action" but does that mean pausing artillery? Does it pause in the middle of a request? between requests? |
Another reason why my implementation may fare better than Jay's implementation when many requests are made is mine properly handles dropping the streams and sinks for disconnected clients. I'm not sure it has an effect over short time periods, since the server might not notice the client is gone immediately, but if artillery properly disconnects and doesn't just vanish, the server will immediately drop the associated stream and sink. |
You are probably tired of this thread and my blundering, long-winded comments, but Jay your post was perfectly timed for a project I am working on. So, thank you and please forgive all my noise. Just glanced at tokio-sync, and it looks really promising. Might make this all moot, but I really want to know how bad locks are in tokio futures. I'm a bit stubborn so I couldn't let it go and decided to profile Jay's solution, and I've found that benchmarking them against each other is not a fair comparison. First, I found valgrind's DRD tool, which can detect lock contention:
After startup, no significant contention is reported, even under heavy load. So, if the locks aren't causing the service degradation, what is? Right? Back to valgrind:
It looks like This brings me to why benchmarking the solutions against each other is not fair for comparing messaging and locking.
@asonix actually fixes all of these problems, so the benchmarks aren't actually revealing anything about locking vs messages. I say "fixes", but I guess if the game is designed such that clients state should be persisted after disconnect, number one is not a problem and there is not really a "memory leak", but the game servers do behave differently. Anyways, I used your solution asonix to refactor the code, and I am going to try to benchmark again to see if lock contention causes performance problems. My changes: jaybutera:0d22d4fc636...kwarrick:9dc1698e94 Let me know what you think, and I will come back with my results if you are interested. |
Nice catch I'm looking forward to seeing the results. |
hey @kwarrick can I ask what you're doing with this sink here? It doesn't seem to get used anywhere, and when a tx half of a channel is dropped, the rx side's stream ends. |
oh nevermind, you insert it into connections |
I misread that as |
Using RwLocks in a tokio threadpool can cause performance issues since one future on one thread can block other futures on other threads, when those other threads could potentially be doing other work.
I don't have a clean solution for this example project using
tokio_threadpool::blocking
because you need to put blocking inside apoll_fn
which requires values moved into thePollFn
beClone
, but part of what we need inside theseblocking
sections is the write-half of aTcpStream
, which is notClone
.My idea for a way around this is we have a stream that folds over some state, and other futures interact with that state by sending and receiving messages.
The text was updated successfully, but these errors were encountered: