Rayon like library

I'm kind of new to rust and been blown away with Rayon. So to try and understand it better, I found a similar project to work from called Rayoff https://github.com/aeyakovenko/rayoff. I used this as a base and experimented with different ways to increase the speed.

The current result looks something like this: https://github.com/JackThomson2/Threadpooler excuse the name. I had no idea what to call it.

Any feedback / tips would be greatly appreciated.

@JackThomson2, welcome aboard :slightly_smiling_face:

Well, the repo you are basing your project off, https://github.com/aeyakovenko/rayoff, is already making mistakes:

  • some patterns are code smells (such as using extern "C" callbacks when no FFI is happening),

  • and some are actually unsound :warning: (such as misusing @Michael-F-Bryan's FFI closure unpacking to obtain a dangling pointer).

    • Aside: @Michael-F-Bryan, it seems that the issues I pointed out about your API being a bit error-prone (no lifetime in the returned value) are indeed biting readers of your blog post; what do you think of trying to refactor your API into a scoped API, with an example of misuage? (using the pointer outside the scope)

So it's a pretty bad start for your project, @JackThomson2 :sweat_smile:

A general tip when starting with Rust, is to try and avoid doing unsafe code until the language is mastered, or at least, doing it alone: do ask for reviews! Indeed, unsafe code is subtle and error-prone, it is therefore quite hard; it is actually harder than just coding in C.

Trying to put in in "Socrates' paradox terms":

Although I do not suppose that either of us knows [how to write correct unsafe code], I am better off than they are – for they know nothing, and think that they know. I neither know nor think that I know.
– Socrustes


That being said, contrary to the original author, you have actually done something far better than them: you have actually asked the community for a code review / suggestions. And that's what's gonna lead to you having a better crate then they had :wink:

So, the exercise I recommend you undertake, is to go and try to rewrite their crate without unsafe, or at least, with as little unsafe as possible, asking as many questions as you wish within this thread, so that we can mentor you to achieve this.

This can lead to a quite educational experience, one that will benefit other readers of this thread :slightly_smiling_face:

4 Likes

Hmm... I'm not a massive fan of the reasoning provided when using unsafe.

/// Safe because ctx/elems are only touched until job is complete.
/// Job::wait must be called to complete the job
unsafe impl<F, T> Send for Job<F, T> where F: Fn(*mut T) {}

/// Safe because data access either atomic or read only
/// elems atomic access is guarded by the work_index
/// Job::wait must be called to complete the job
unsafe impl<F, T> Sync for Job<F, T> where F: Fn(*mut T) {}

For example we implement Send+Sync for Job with the justification that access is synchronised, but what if some of the data inside our elements/closure can only ever be used from a single thread ("thread affinity")? This happens quite often in GUI libraries and often leads to an exception (in managed languages) or triggering an assert/abort.

Really, we should only implement Sync when the item pointed to by a Job's elems is also Sync (i.e. unsafe impl<T, F> Sync for Job<T, F> where T: Sync, F: Sync).


Your Pool::dispatch_mut() method has a problem related to Exception Safety.

pub struct Job<F, T>
where
    F: Fn(*mut T),
{
    complete: *const AtomicU64,
    method: *const F,
    data: *mut T,
}

impl Pool {
    #[inline]
    pub fn dispatch_mut<F, A>(&self, elems: &mut [A], func: F)
    where
        F: Fn(&mut A) + Send + Sync,
    {
        // Job must wait to completion before this frame returns
        let job = unsafe { Job::new(elems, func, self.num_threads) };
        let job = Arc::new(job);
        self.notify_all(job.clone());
        job.wait();
    }
}

You're probably gonna have a bad time if Job::notify_all() or Job::wait() ever panic (e.g. because of the unwrap() in notify_all())... If a call panics while background threads are still running then the original thread will unwind its stack and destroy the closed-over data. This triggers a use-after-free if the background thread tries to access this data (which it will, because that's all it ever does).


@Yandros, the resource mentioned in unpack_closure()'s doc-comment is about 3 years old. I've since created a much better (and less footgun-friendly!) article which should appear much higher up in search results than that undocumented S3 bucket.

You'll write something like this:

pub fn get_trampoline<F>(_closure: &F) -> AddCallback
where
    F: FnMut(c_int),
{
    trampoline::<F>
}

unsafe extern "C" fn trampoline<F>(result: c_int, user_data: *mut c_void)
where
    F: FnMut(c_int),
{
    let user_data = &mut *(user_data as *mut F);
    user_data(result);
}

And then use it like this:

let mut closure = |result: c_int| got = result;
let trampoline = get_trampoline(&closure);

unsafe {
    better_add_two_numbers(
        1,
        2,
        trampoline,
        &mut closure as *mut _ as *mut c_void,
    );
}

This formulation puts the onus on the caller to ensure &mut closure as *mut _ as *mut c_void is done correctly.

3 Likes

As someone who dived into unsafe quite early (perhaps too early), I can say with certainty, that it's a terrible idea to go beyond simple-to-proof-correct unsafe without sufficient Rust experience.

Anything that involves &-to-&mut conversions or manual heap memory management via raw pointers and custom Drop implementation is beyond fun, because there are so many things to keep in mind, even the standard library authors have introduced several unsound bugs over the years and those people are quite experienced with Rust.

If you come from a language like C/C++, you may be experienced with manual memory management, but what you're not familiar with are Rust's aliasing rules, that are powerful when it comes to optimization, but also make writing correct unsafe code more difficult, because they're very strict.

Do yourself a favor and stick to safe Rust, at least in the beginning. The pitfall of UB is, that it may appear to work just fine, but suddenly breaks your program in the future or under specific runtime conditions you haven't tested, initially.

2 Likes

I wouldn't say writing unsafe code is as horribly unpleasant as @Phlopsi makes it out to be, but it's a good idea to first become familiar with safe Rust first. That way you get an intuition for how safe abstractions work and understand the core concepts behind how Send, Sync, and lifetimes let you write correct code.

By all means, write unsafe code if you want. It's a really good way to learn how computers and memory work, and you can have a lot of fun doing it. Just make sure to ask for code reviews and see how other people have developed safe abstractions over fundamentally unsafe operations. Projects like rayon, the standard library, once_cell and crossbeam are pretty good examples.

2 Likes

Not all unsafe Rust is the same, either: I’m relatively confident about transmuting repr(transparent) types or inserting unreachable_unchecked, but will still ask here for verification if I need to do anything that might interact with ownership in odd ways, like using raw pointers.

1 Like

Thanks for all this great feedback, I do appreciate it. The unsafe-ness of this code is definitely apparent, it all came around with me getting ahead of myself in micro-benchmarks and trying to squeeze those numbers down. (In some/few workloads it can pull ahead of rayon)

I will look at converting this to use less unsafety and keep you posted.