Conversation
|
Hmmm, miri clearly does not like this even though the leak is intentional. Not sure how to tackle this without wholesale disabling leak detection. |
|
Another alternative is to follow through with the |
notgull
left a comment
notgull
left a comment
There was a problem hiding this comment.
Would this be useful for LocalExecutor as well?
Paging @smol-rs/admins as I would appreciate another review as well
src/lib.rs
Outdated
| pub unsafe fn spawn_scoped<'a, T: Send + 'static>( | ||
| &self, | ||
| future: impl Future<Output = T> + Send + 'a, | ||
| ) -> Task<T> { |
There was a problem hiding this comment.
Would this kind of lifetime relaxation be useful on Executor/LocalExecutor as well?
There was a problem hiding this comment.
Perhaps, it would at the very minimum remove the need for std::mem::transmutes, which I would consider much more prone to unsoundness due to uncertainty around layout, to implement ThreadPool::scope-style APIs. Though it's pretty evidently not easy to properly use such an API, and proving the soundness even harder.
There was a problem hiding this comment.
that sounds interesting, could you elaborate?
There was a problem hiding this comment.
The primary API in question is bevy_tasks::TaskPool::scope. The actual implementation can be seen here, though it's pretty tangled due to our need to explicitly schedule tasks onto a separate executor since !Send data must be accessed and dropped from the thread it was created from.
The function is technically non-blocking since the thread yields to the executor while waiting for the tasks in the scope to complete.
Co-authored-by: John Nunley <[email protected]>
|
It took me a second to wrap around what this does conceptually, but now that I get it I think this is an interesting idea as an optimization. I have no opinion on the actual impl, so I'll defer to someone else to review. As perhaps a tiny bikeshed point to raise: when I saw However, I think we could reframe what this does as: "it enables an executor to exist for the duration of the entire program". This is something we often called "static" (e.g. 'static, |
|
can we ensure/check that the |
notgull
left a comment
notgull
left a comment
There was a problem hiding this comment.
I'm a fan of StaticExecutor, but I'd still like the method to be named leak
I don't know how this would be automatically checked, or if it really matters, given that the benchmarks seem to be faster than normal anyways |
|
@fogti we could desugar the user-facing versions of it and force a // before
pub fn async run<T>(&self, fut: impl Future<Output = T>) -> T;
// after
pub fn run<'r, T: 'r>(&'r self, future: impl Future<Output = T> + 'r) -> impl Future<Output = T> + 'r; |
|
@james7132 of course we can (and in the past I would've suggested exactly that), but it would be a good idea to check at least if the |
|
|
Co-authored-by: Alain Emilia Anna Zscheile <[email protected]>
|
Hmm, are benchmarks not built in CI? Odd. |
|
Some of the benchmarks appear to be a bit too small, some have a very large variance, making them unsuitable to properly measure their performance in larger systems... perf report, top overhead |
|
I also noted the variance with For a more realistic workload, I tested this against Bevy's
|
|
Once this is rebased it can be merged. |
4 participants
Resolves #111. Creates a
StaticExecutortype under a feature flag and allows constructing it from anExecutorviaExecutor::leak. Unlike the executor it came from, it's a wrapper around aStateand omits all changes toactive.Note, unlike the API proposed in #111, this PR also includes a unsafe
StaticExecutor::spawn_scopedfor spawning non-'static tasks, where the caller is responsible for ensuring that the task doesn't outlive the borrowed state. This would be required for Bevy to migrate to this type, where we're currently using lifetime transmutation onExecutorto enableThread::scope-like APIs for working with borrowed state.StaticExecutordoes not have an external lifetime parameter so this approach is infeasible without such an API.The performance gains while using the type are substantial: