-
Notifications
You must be signed in to change notification settings - Fork 12
feat: Add arty crate with generic Spawner trait for spawn tasks for different async runtimes #193
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #193 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 106 107 +1
Lines 6881 6905 +24
=======================================
+ Hits 6881 6905 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Have you thought about applying this guide to spawner? The idea is that For example: Tokio: let spawner = Spawner::new_tokio();Custom Runtime: let spawner = Spawner::new_custom(move |future| runtime_handle.spawn(future));The The downside of this approach is that we would enforce Send and Sync bounds, but this is aligned with https://microsoft.github.io/rust-guidelines/guidelines/libs/interop/index.html?highlight=Send#M-TYPES-SEND. |
I can see the value here. I guess the tradeoff would be some additional heap allocations and indirection internally, since the new_custom(...) closure will have to accept a boxed future. Once Oxidizer RT moves here we can add a direct enum variant for it to avoid that additional cost. In fact, we can probably remove this abstraction altogether at that point. |
I see the value of this type even when oxidizer rt is public. Sometimes, the library only needs to care about how to spawn tasks and using this type can simplify those scenarios. |
|
What's the link between your logo and this crate's use? |
|
|
||
| - [Crates](#crates) | ||
| - [About this Repo](#about-this-repo) | ||
| - [The Oxidizer Project](#the-oxidizer-project) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these changes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was generated by my VS Code markdown extension. It automatically updates the table of contents to match the headers in the file. Should we keep it this way since it's more accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather nuke it, since the first entry in the table of contents ends up being the whole page, which is silly.
Throwing the plane was like a fire-and-forget operation, also I had named this "wing" before. It probably isn't as relevant now. In fact, I might use it for uniflight, instead. |
| <div align="center"> | ||
| <img src="./logo.png" alt="Arty Logo" width="96"> | ||
|
|
||
| # Arty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda hoped that arty crate would be the actual runtime, and we could use something more generic for the abstraction layer (runtime-abstraction or something). Not a strong opinion, just feel like arty is a missed opportunity for just the abstraction layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Ralf suggested I use arty. @ralfbiedert what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any_arty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martin-kolinek you mean the ox... umbrella becomes arty? I don't have super strong feelings either, would be onboard with that. I'm not a big fan of runtime-abstraction though, and also not really wing, the former is too long, the latter too unrelated. But let's discuss naming on Teams.
crates/arty/src/spawner.rs
Outdated
| /// assert_eq!(result, 2); | ||
| /// # } | ||
| /// ``` | ||
| pub async fn run<T: Send + 'static>(&self, work: impl Future<Output = T> + Send + 'static) -> T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this perform in comparison to tokio::spawn (which already returns an impl Future)? Probably worth a benchmark as we don't want to provide an abstraction that slows things down unnecessarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martintmk was also suggesting we avoid using oneshot here for tokio, since it already has its own join handle and all that
ralfbiedert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing
crates/arty/src/spawner.rs
Outdated
| pub fn spawn(&self, work: impl Future<Output = ()> + Send + 'static) { | ||
| match &self.0 { | ||
| #[cfg(feature = "tokio")] | ||
| SpawnerKind::Tokio => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting the implementation of tokio support to be just like the Custom type, except that the callbacks are provided by this library instead of by the caller. This would eliminate the need for match here. Basically, make all Spawners custom, but for tokio provide a "built-in custom"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benefit of doing it this way is that for tokio we can avoid the performance hit of allocating the future to heap. Although maybe tokio does that anyways, I'm not actually sure.
crates/arty/src/spawner.rs
Outdated
| /// assert_eq!(result, 2); | ||
| /// # } | ||
| /// ``` | ||
| pub async fn run<T: Send + 'static>(&self, work: impl Future<Output = T> + Send + 'static) -> T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of run. I rather think your spawn should return its own JoinHandle (defined in arty) which you map for each of your variants.
Also, I am wondering if we should give people on arty already an API to express parallelism intent. Like have spawn and spawn_anywhere (names TBD). The former would make a best-effort to spawn on the same thread, while the latter would make a best-effort not to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was originally defining a JoinHandle type here, though that was back when I was also using a Spawner trait, too. Martin Tomka suggested simplifying it in this way. @martintmk any comments here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I pushed a version with a JoinHandle. @martintmk is this any better than the way I was using JoinHandle before?
| <div align="center"> | ||
| <img src="./logo.png" alt="Arty Logo" width="96"> | ||
|
|
||
| # Arty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martin-kolinek you mean the ox... umbrella becomes arty? I don't have super strong feelings either, would be onboard with that. I'm not a big fan of runtime-abstraction though, and also not really wing, the former is too long, the latter too unrelated. But let's discuss naming on Teams.
…es. Throw compile error without any features.
No description provided.