hi, i wrote this code a few months ago, and im not sure if every unsafe in this code is justified, so it would nice to have some experts review them...
I've not looked in great depth at the code, but it looks to me like your Wrapper is a reimplementation of UnsafeCell, and that what you're actually after is RefCell - in particular, RefCell::borrow_mut implements what you're trying to do with Wrapper, but says that it'll panic if you get the safety guarantees wrong.
To reimplement around RefCell, you'd change pub fn buffer<U, D, B, R1, R2>(bufs: &mut [B], mut update: U, mut draw: D) -> Error<R1, R2> to pub fn buffer<U, D, B, R1, R2>(bufs: &[RefCell<B>], mut update: U, mut draw: D) -> Error<R1, R2>, wrapping the buffers in RefCell, and then use normal shared slice indexing to get a RefCell<B>, which you could then call .borrow_mut() on to get a &mut B. Something like:
// With Wrapper:
(update)(unsafe { &mut *bufs.add(ui_val) })
// With RefCell
(update)(bufs[ui_val].borrow_mut())
This pushes all the safety requirements onto the authors of the Standard Library, and hopefully they've got them completely right.
1 Like
i thought RefCell is !Send?
sorry, i was indeed wrong, thank you for your precious eyes & time!
sorry to disturb you again, but i think unless im missing something, won't that require RefCell<B>: Sync?
Try it - I believe that because RefCell is Send if B is Send, and because you're using it like a reference type, you do not need RefCell to be Sync.
It is pretty much a straightforward implementation of the runtime borrowing your comments described, so if it's not usable, your safety comments are probably incomplete.
but how? I don't get how a &[RefCell<B>] can be shared between threads without requiring it to be Sync
tried it, and unfortunately did not work:
error[E0277]: `RefCell<B>` cannot be shared between threads safely
--> src/lib.rs:67:43
|
67 | let f: Box<dyn FnOnce() + Send> = Box::new(closure);
| ^^^^^^^^^^^^^^^^^ `RefCell<B>` cannot be shared between threads safely
|
= help: within `[RefCell<B>]`, the trait `Sync` is not implemented for `RefCell<B>`
= note: required because it appears within the type `[RefCell<B>]`
= note: required because of the requirements on the impl of `Send` for `&[RefCell<B>]`
note: required because it's used within this closure
--> src/lib.rs:34:23
|
34 | let closure = move || {
| _______________________^
35 | | // SAFETY: we are only accessing different locations from different threads at any
36 | | // time, so this, in fact is safe to be Send
37 | | //let bufs = wrapper.0;
... |
64 | | }
65 | | };
| |_________^
= note: required for the cast to the object type `dyn FnOnce() + Send`
For more information about this error, try `rustc --explain E0277`.
error: could not compile `buf` due to previous error
The safe way to fix this is to replace the RefCell with Mutex, and lock/unlock as needed (using Mutex::try_lock if you don't want to wait for the lock to be free). The core of all of the errors you're getting is that you need synchronization between the two threads to confirm that there's no data race, or you need to be personally confident in your use of unsafe - in this case, verifying that there are no circumstances under which you could have two threads accessing the same buffer.
i think i am pretty confident with the use of unsafe. i believe that there is no way a data race could happen given my setup, and that was what i wanted to confirm here (plus confirming if the requirements were complete and not missing something like a lifetime or a Send somewhere)
I can see a data race, since you're not using memory barriers between the two threads; if they run on different cores of the system, or if the compiler optimizes on this basis, you can have writes completed by one thread that are not going to be picked up by reads on the other thread.
This is why I'd advocate for using Mutex::try_lock().unwrap() in preference to trying to do this with unsafe; memory ordering between threads can be deeply non-intuitive, and the standard library is written to get it right or trigger an error if it can't.
1 Like
oh yeah, but can i avoid using mutex while also ensuring writes are picked up by reads from the other thread?
what do you think about using UnsafeCell<&mut [B]>?
UnsafeCell pushes the responsibility for generating the correct memory ordering onto you. You could use a Mutex to generate the correct memory ordering state, or implement a Mutex using atomics (but be very careful here - experts make mistakes doing this), but that doesn't reduce the overhead.
I'm assuming this is for a game or similar thing where performance matters; start with a Mutex wrapping the buffers, and watch for Mutex operations showing up when you profile it. Many experts in the field care deeply about the performance of Mutex in the uncontended case (the case where no thread ever has to wait for another thread to release the Mutex) and this is thus often very cheap. For example, on modern Windows and Linux on current x86 and most ARM processors, locking and unlocking in the uncontended case is an atomic operation followed by a branch that will be taken if the Mutex turned out to be contended.
the library is designed to (atleast intended to), at any point in time, access two different locations in the same buffer. im not sure if a mutex wrapping the whole buffer will allow me to do that
You are walking through a minefield if you're trying to do that (as opposed to double-buffering type scenarios, where only one thread at a time accesses a given buffer). Taking and releasing locks is slow but safe (take a lock, read the buffer, release the lock); RwLocks help if the issue is lots of readers, few writers, by allowing multiple parallel readers.
Otherwise, you're deep into messy territory; the semantics of C and Rust allow the compiler to not care about memory accesses from other threads until you hit an appropriate barrier operation (atomic op, or something like a lock built atop an atomic op), and to thus not access the buffer at the times you expect. At the machine level, you still have to deal with cache coherency rules, store queues, load buffers and more that mean that a write can be indefinitely delayed with respect to reads from other CPU cores in the same machine.
sorry, i should have been more clear, so:
i have two functions, update and draw.
the loops that call them use AtomicUsize to synchronize their accesses annd make sure they are using different buffers in the same list of buffers (called bufs).
You also need to make sure that you're using the correct memory orderings with AtomicUsize. Using SeqCst or Relaxed is almost certainly wrong, since Relaxed is too weak, while SeqCst is so strong that it'll cost you a lot more than a Mutex around each buffer in the list will cost you.
In other words, instead of having bufs: &mut [B] and relying on unsafe to get things right, have bufs: &[Mutex<B>], and rely on the Mutex being implemented correctly so that you can lock and unlock the buffers. As a nice bonus, this should permit the two threads to avoid needing strongly ordered atomics - Relaxed will be good enough if the two threads need to co-ordinate buffer numbers, or no atomic if they don't.
1 Like
yes i know SeqCst is a lot more costly, the thing is: does the cost outweigh less allocations and less cache misses?
but i guess even if mine is faster, Mutex is only way forward, since i should not trust myself to write unsafe code better than the stdlib authors enough to justify the better performance.
1 Like