feat: add unsafe spawn_unchecked version of spawn without 'static bound#1274
feat: add unsafe spawn_unchecked version of spawn without 'static bound#1274Enduriel wants to merge 1 commit into
Conversation
| /// | ||
| /// # Safety | ||
| /// See [spawn_unchecked] for the safety requirements due to the lack of 'static bound. | ||
| pub(super) unsafe fn spawn_unchecked_in<F>(func: F, registry: &Arc<Registry>) |
There was a problem hiding this comment.
Independently from whether this API is included, for the implementation it would most likely be preferable if the safe version call into the unsafe ones instead of duplicating their implementation.
There was a problem hiding this comment.
I thought about that, but then the safe versions would lose the compile time bounds check for their lifetimes. I'm happy to adjust it that way, just want to confirm that that is indeed desirable in this context.
In an ideal world rust would have a way to unsafely extend the lifetime of a non-static value to 'static but to my understanding this does not exist.
There was a problem hiding this comment.
In an ideal world rust would have a way to unsafely extend the lifetime of a non-static value to 'static but to my understanding this does not exist.
This sounds like something that transmute or even casting raw pointers is able to do?
There was a problem hiding this comment.
It's not a lot of duplication, but it could be reduced by having spawn_job return the full Box<HeapJob> and do the into_job_ref or into_static_job_ref separately.
There was a problem hiding this comment.
@adamreichold If you have an alternative solution here I would love to see one, but unfortunately I wasn't able to make it work with generics.
There was a problem hiding this comment.
You can always achieve that using the "option dance", c.f. https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=ba7ef9dea5bc92b2f8c457f9f46fc82f
There was a problem hiding this comment.
(Still not pretty, but it does avoid the heap allocation.)
There was a problem hiding this comment.
The caller also has to ensure this remains exclusive, never aliasing that &mut.
Right. &F and F: Fn() are probably the less foot-gunny choice at the cost of interior mutability. Box is beginning to look better and better. ;-)
There was a problem hiding this comment.
So
use std::cell::Cell;
fn upgrade(f: impl FnOnce() + Send) -> impl Fn() + Send {
let f = Cell::new(Some(f));
move || f.take().unwrap()()
}and
use rayon_core::spawn;
unsafe fn spawn_unchecked<F>(f: &F)
where
F: Fn() + Send,
{
struct SendPtr(*const ());
unsafe impl Send for SendPtr {}
let ptr = SendPtr(f as *const F as *const ());
spawn(move || {
let ptr = ptr;
let f = unsafe { &*(ptr.0 as *const F) };
f();
});
}There was a problem hiding this comment.
yeah I mean tbh that doesn't feel like a bad approach to me.
Do you have a real example where this would be wanted? If we do add this, it should also be mirrored as a |
My day job is working on You can see my usage of this here. Basically, since rust currently has no way of doing this, I guarantee that I will not This allows me to create a very simple async wrapper around these CPU intensive tasks which simply blocks the async thread if dropped. Since under normal usage dropping futures is uncommon, I am willing to pay the performance price in the cancellation scenario in exchange for normally non-blocking code flows.
I spent a day trying to figure this out, including asking for help on the rust discord, I wasn't able to reach a solution that worked, basically everything still seemed to still have a non-static bound or else not work with generics. I do agree that that would be an ideal solution, but I am not aware of a way to do this.
Am happy to add this as well |
|
I think the proposed solutions by @adamreichold showed that there are acceptable workarounds to this scenario. I personally went with the box approach as the cleanest. Thank you for both of your time, it seems like the PR is unnecessary. |
| /// # Safety | ||
| /// The caller must ensure that the spawned closure does not outlive | ||
| /// any references it captures. |
There was a problem hiding this comment.
My final thought is that this safety requirement is a lot harder than it appears from this simple statement. The code which owns the borrowed data has to guard against all forms of invalidation, including panic unwinding, which essentially means the lifetime of the data needs to come from outside a panic::catch_unwind call (unless your whole app is panic=abort). Drop guards may also work if you're absolutely sure there's no way for that to be leaked or otherwise forgotten. It is also hard to figure out when it is safe to release things, since any action in the closure to invalidate itself can be a nasty race -- we had our own issues like #740 and #1011.
This allows downstream crates to implement scoped tasks. While these implementations would have to be unsafe, it's an incredibly useful potential pattern if it could take advantage of rayon's threadpool.