mirror of
https://github.com/ramsayleung/rspotify.git
synced 2026-04-26 16:05:53 +03:00
[GH-ISSUE #112] Cleaning up the blocking module #34
Labels
No labels
Stale
bug
discussion
enhancement
good first issue
good first issue
help wanted
pull-request
question
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/rspotify#34
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @marioortizmanero on GitHub (Aug 15, 2020).
Original GitHub issue: https://github.com/ramsayleung/rspotify/issues/112
Originally assigned to: @marioortizmanero on GitHub.
This is a continuation of #102, which discussed how to properly export the
blockingmodule without copy-pasting the async functions and removingasyncandawait. The current solution makes it painful to maintain rspotify, and compilation times when using the blocking client are huge because you're basically compiling rspotify twice, which kinda defeats the point of #108.I first thought that using an attribute macro could be a good idea because all that needs to be modified from the async implementations is removing the
asyncandawaitkeywords. See this comment for a demonstration. But this still doesn't fix the "compiling the codebase twice" issue, because the code would still be repeated, now after the macro is ran instead of manually. The macro in question would make compilation times even longer, and could be messy to implement. The only possible way to only "compile the codebase once" would be by having theblockingfeature only export the blocking interface when used, but I'm not sure we can assume that the users won't need both the async and blocking interfaces for a program.The second possible solution is calling the async implementations in the blocking functions using
block_on. This might sound less efficient at first, but here's a comparison I made to prove my point:What?
block_onwas just as fast as the copypaste? Turns out that this is more or less whatreqwestdoes with its ownblockingmodule!. So both the copypaste and the runtime solutions are basically doing the same. This way we don't even need theblockingfeature inreqwest, and rspotify will only be "compiled once".This solution could use a smaller macro that automatically generates the boilerplate
block_onfunctions to avoid some repetition.The best part is that the example isn't even properly optimized. It generates a runtime every time the function is called. If it were global, it would only have to be initialized once. Here's a more complex and improvable (since it doesn't seem to be actually faster for some reason) version with
lazy_static:@ramsayleung commented on GitHub (Aug 16, 2020):
Totally agree with your point, the copy-paste blocking module makes
rspotifypainful to maintain, you have to maintain two versions of code if you are adding a new endpoint.This solution is good, we don't have generate code by hand, can't help to see it.
And there is one more thing which we should keep eyes on, will state of blocking call become more complicated to maintain?
After discussion in this PR https://github.com/ramsayleung/rspotify/pull/102, I come out with an intuition almost as same as this:
And my problem is that is there more state-machine and error we have to take care of compared with
copy-pastecode, because we plan to introducetokei runtimeto handle the execution of callingreqwestfunction.Yeah, but I think it's some kind of thing like:
reqwestgives,tokiotakes away :)@marioortizmanero commented on GitHub (Aug 16, 2020):
I don't understand what you mean with that. The way I see it is that performance/complexity-wise it's basically the same as the copy paste version.
reqwestalso introduces a blocking tokio runtime under the hood, with an even more complicated method to run the blocking code than just usingblock_on. We have less code to take care of than the copy-paste version because we only have to callruntime.block_on(async move { ... }): the implementation only needs to be written once.I'll start working on this once #95, #110 and #113 are finished, but I still need to figure out how to only create a single global runtime and how to make it more performant.
@ramsayleung commented on GitHub (Aug 16, 2020):
Yeap, I get your point
How about the Singleton design pattern, would it be helpful?
@marioortizmanero commented on GitHub (Aug 16, 2020):
I'm not a big fan of singletons but I guess it's the only way to do it in this case. Isn't what I suggested with
lazy_statica singleton anyway? What I don't understand is why the singleton performs worse than using multiple runtimes, perhaps theArc<Mutex<T>>overhead is greater than instantiating a new runtime? :/@Darksonn commented on GitHub (Aug 29, 2020):
I recommend using a shared runtime, but with a small change compared to what you have now:
Arc<Mutex<...>>Handle::block_onto block on many things concurrently.Alternatively you could spawn the operation on the shared runtime and use
Handle::block_onon theJoinHandlereturned bytokio::spawn.I'm not sure which of the two are better.
As slight variation on the above, you can use a basic scheduler, but spawn a thread that calls block on, waiting on e.g. a channel with jobs. This is what reqwest's blocking module does afaik.
@marioortizmanero commented on GitHub (Aug 29, 2020):
Thanks for the recommendations, @Darksonn!
I'm extending the initial benchmark with all the possibilities. The benchmark obviously isn't reliable enough for the choice, but I think it gives an idea of how the performance is affected:
main.rs
Cargo.toml
Results on an Intel Pentium G4560 (4) @ 3.500GHz (which, to be fair, is a terrible CPU, specially when multithreading). It'd be a good idea to run the benchmark on more systems:
The
reqwestsolution you suggested is basically the same aswith_copypaste, since it's what it uses under the hood.@ramsayleung commented on GitHub (Aug 29, 2020):
Perhaps we could do that with the
Github Actionwithruns-onlabel?