Finish tokio migration #606
Conversation
|
Leaving this up to ash - I haven't been following the tokio migration work closely. |
|
@Johannesd3 I'm going to merge this into Thanks again! :-) |
|
Did you try it already? The rodio backend seems like a tough nut. |
|
Or if you want, I could try to merge it this evening. |
|
Rodio back-end seems to compile, but had to leave to work before testing in earnest. Will have time only later this week for proper testing.. |
| } | ||
| } | ||
|
|
||
| #[tokio::main] |
There was a problem hiding this comment.
I'm not sure what we were doing before but maybe we should change this? This is going to start one OS thread per logical core for tokio. On my machine that'd be 16 threads. Seems like librespot probably doesn't need those kinds of resources?
There was a problem hiding this comment.
So what do you think is a reasonable number of threads? I think with tokio-core it was only single-threaded.
There was a problem hiding this comment.
Single threaded sounds good to me, you can do this with the current_thread builder. I'm not sure if there's a macro associated with this.
There was a problem hiding this comment.
The thread count can be configured using #[tokio::main(flavor = "multi_thread", worker_threads = 10)]. To get the single thread behaviour back, #[tokio::main(flavor = "current_thread")] can be used. Would it be worth adding a CLI argument such as --multithread XX where XX is the thread count desired by the user, otherwise defaulting to single thread operation? We'd probably need to check user input didn't exceed the number of system cores if we opted to do this.
There was a problem hiding this comment.
Aren't we anyway network bound? Not sure if having more threads is going to help much. Although would be great to have some numbers to compare. :-)
There was a problem hiding this comment.
Many people try hard to use all cores in the best way, and encounter strange bugs while trying this. We don't have concurrency bugs, but still we're discussing whether to make it single-threaded. 😀
There was a problem hiding this comment.
I'm not sure I follow, the type of runtime you need will be dictated by your requirements. Nothing about the project README indicates that we want a high-throughput server capable of handling tens of thousands of req/s, so I think having the default configuration such that the footprint is minimal is good for now.
There was a problem hiding this comment.
I was mainly joking. It would be interesting to see the differences between different numbers of threads, but I agree, 16 threads are not necessary.
There was a problem hiding this comment.
Maybe you would like to create a PR? (the commit history of this branch looks too monotonous anyway)
There was a problem hiding this comment.
Sorry my comment went to the wrong place, sarcasm is sometimes hard over the internet 😄 Do you want to just update this PR to use [tokio::main(flavor = "current_thread")] I think that would be sufficient if @sashahilton00 agrees
edit: Oh, I just realized this has all been merged now. Apologies.
| audio_filter: audio_filter, | ||
| event_senders: [event_sender].to_vec(), | ||
| }; | ||
| let handle = thread::spawn(move || { |
There was a problem hiding this comment.
I'm not too familiar with this code but why does this use thread::spawn as opposed to using a tokio task? Or rather, if it's using blocking code, should it not be spawned with spawn_blocking into the tokio blocking threadpool?
There was a problem hiding this comment.
This is definitely blocking (I tried to rewrite it as future, but encountered strange bugs). Also it's a long-running thread, powering the playback, so an own thread is ok, I think.
|
|
||
| tokio::spawn(async move { | ||
| if let Some(data) = loader.load_track(spotify_id, position_ms).await { | ||
| std::thread::spawn(move || { |
There was a problem hiding this comment.
Same question about thread::spawn here
This PR finishes the tokio migration. The application is working again - after all it was easier than I thought -, but I'm pretty sure it's not perfect yet.
I had to figure out how to make the player blocking again because there were some mysterious bugs in the async version (now it's again unimportant whether the sinks are Send, but it's nice to have).
@sashahilton00 @ashthespy How do we proceed? Before merging it into dev it should be tested more thoroughly, and I would like to do some refactoring.