Add a way to batch spawn tasks #92
Conversation
| /// [`Executor::spawn_many`]: Executor::spawn_many | ||
| pub fn spawn_many<T: Send + 'a, F: Future<Output = T> + Send + 'a>( | ||
| &self, | ||
| futures: impl IntoIterator<Item = F>, |
There was a problem hiding this comment.
This forces the futures to be the same type and also requires something to collect the tasks. If we're ok with users potentially injecting extra non-blocking code via iterator implementations (with the doc comments as a deterrent), wouldn't a SpawnScope<'a> be a more flexible API?
There was a problem hiding this comment.
It's much harder to accidentally create a blocking Iterator than it is to accidentally hold a lock type across an await point. I've seen the former a couple of times and the latter all the time.
| futures: impl IntoIterator<Item = F>, | ||
| handles: &mut impl Extend<Task<F::Output>>, | ||
| ) { | ||
| let mut active = self.inner().state().active.lock().unwrap(); |
There was a problem hiding this comment.
For a local executor, why does it need a lock at all? Probably not necessary to address in this PR, but I thought the local executor itself could be !Send and !Sync, so wouldn't a RefCell be sufficient for a LocalExecutor state?
There was a problem hiding this comment.
It would require a reimplementation of LocalExecutor's internals, which is something I don't feel like doing right now as the current implementation works well enough. I would accept a PR for this.
There was a problem hiding this comment.
ok, as a change this would propagate through the entire code base because it would require a different State struct to actually eliminate all the mutexes, etc, which then probably needs either a duplication of all the other internal structs which hold references to State, or an abstraction over the two variants of State.
There was a problem hiding this comment.
It's probably doable with macro_rules, but that might have a serious impact on readability.
There was a problem hiding this comment.
I think a proper abstraction would require a lot of interior mutability, we just can't directly abstract all that without pulling in niche dependencies, I suppose..
There was a problem hiding this comment.
Generally thread unsafe primitives are provided by unsend instead of smol. In this case see unsend::Executor
src/lib.rs
Outdated
| task | ||
| for future in futures { | ||
| // SAFETY: F and T are both `Send`. | ||
| handles.extend(Some(unsafe { |
There was a problem hiding this comment.
std::iter::once may be preferable to Option, though I assume the branch it's iterator implementation gets optimized out at some point.
There was a problem hiding this comment.
I've realized that we can just convert futures into an Iterator of Tasks, and then use handles.extend with that iterator. That should probably enable more optimizations.
notgull
force-pushed
the
notgull/batch-spawn
branch
from
b139978 to
72febd5
Compare
|
@smol-rs/admins I would appreciate a review on this, as it's blocking performance work in Bevy. |
|
A common bug with *_many functions in Rust is that memory safety is violated when the iterator panics. So could you please add a Miri-compatible test for such a case? (I do not believe that this implementation violates memory safety on that case, but at least the implementation does not seem to care about cases where the iterator panics.) |
notgull
force-pushed
the
notgull/batch-spawn
branch
from
4f03951 to
bbefd6a
Compare
Added! I've also added a workaround for the mutex poisoning that shows up here. |
|
It seems miri reported memory leaks. https://github.com/smol-rs/async-executor/actions/runs/7999577501/job/21847610801?pr=92 |
notgull
force-pushed
the
notgull/batch-spawn
branch
from
bbefd6a to
dff5a59
Compare
|
Hmm, looks like the issue was related to the thread-optimized backend? It looks like it doesn't leak memory now. |
For some workloads many tasks are spawned at a time. This requires locking and unlocking the executor's inner lock every time you spawn a task. If you spawn many tasks this can be expensive. This commit exposes a new "spawn_batch" method on both types. This method allows the user to spawn an entire set of tasks at a time. Closes #91 Signed-off-by: John Nunley <[email protected]>
notgull
force-pushed
the
notgull/batch-spawn
branch
from
dff5a59 to
b0c6f7a
Compare
For some workloads many tasks are spawned at a time. This requires
locking and unlocking the executor's inner lock every time you spawn a
task. If you spawn many tasks this can be expensive.
This commit exposes a new "spawn_many" method on both types. This
method allows the user to spawn an entire set of tasks at a time.
Closes #91