How `unsafe` this code block is?

code review
1 
struct XorShift128Plus { x: u64,  y: u64 }
impl XorShift128Plus {
    fn next(&mut self) -> u64 {
        let Self { x, y } = *self;
        let t = x ^ (x << 23);
        self.x = y;
        self.y = t ^ y ^ (t >> 17) ^ (y >> 26);
        self.y.wrapping_add(y)
    }
}
static mut RNG: XorShift128Plus = XorShift128Plus { x: 0x_C01D, y: 0x_C0F1, };
pub fn rand_num() -> u64 {
    unsafe { RNG.next() }
}

My little knowledge say, "In this context, Its OK, to be unsafe"
Its feel like more random!!!

I say, Its good...
But other say, It's UB

So, How unsafe it is ?

2

If two threads call rand_num at the same time, it's instant undefined behavior since the function creates an exclusive reference to RNG.

10 Likes
3

It breaks promises that safe Rust makes. Because rand_num is not marked as unsafe, Rust allows calling it at any time from any thread and expects that to remain safe.

This function will cause a data race if it's called from multiple threads at the same time, which is unsafe by Rust standards. It will also create multiple exclusive &mut references to the same data at the same time, which breaks Rust's aliasing rules.

Note that even if you are not actually calling rand_num from multiple threads, the function definition is still wrong according to Rust's rules. Rust's safety rules are about what code is allowed to do in the worst case, not just what code is currently doing.

If you don't care about this function misbehaving when called from multiple threads, you could use AtomicU64 for x and y. Or at least wrap everything in UnsafeCell which tells the compiler it may be accessed from multiple threads at the same time.

If you want this function to work safely and reliably, you will need to wrap the state in a Mutex, or use thread-local storage instead.

4 Likes
4

UnsafeCell by itself doesn't prevent data races; the unsafe code must follow the same rules as safe code using the values directly. That's why it's marked !Sync. An AtomicU64, Mutex, or thread-local variable would be necessary for soundness here.

2 Likes
5

Also, even if this wasn't UB then one possible result of calling this function from multiple threads at the same time would be that they end up with the same random number, since nothing prevents them from coincidentally executing each step at the same time and thus end up with identical results and identical state updates. This is not a great property for the random number generator to have :slight_smile:

1 Like
6

How to write safe code that its runtime speed is faster than equivalent unsafe code?

7

If the unsafe code has UB, there's no "equivalence" and no way to reason on its speed, since there's no guarantee that it will do anything sensible at all. So the question is "how to write safe code which is faster than sound equivalent unsafe code?", and the answer is "not faster, but of the same speed - just by following the idiomatic approaches, unless benchmark says otherwise".

2 Likes
8

If you use AtomicU64 with Relaxed ordering, on most platforms it will have the same speed as your unsafe non-atomic code. It will be "safe" as far as Rust is concerned, although without locking it will still be incorrect in multi-threaded programs due to risk of returning the exact same number, or exposing inconsistent state, to different threads calling it at the same time.

The real safe solution is to use rand::thread_rng() and/or keeping that state in some local variable/function argument instead of having a shared global.

In terms of speed: if you're writing for multi-core CPUs, then for compute-intensive tasks taking advantage of multiple threads will likely be faster than a single-threaded program. When you have multiple threads, then globals are slow due to sharing of memory that requires synchronization of caches between cores. Global state also optimizes worse, because the optimizer has to be conservative about accessing a potentially shared state. In code that really needs to use lots of randomness, having the random state in a local variable would allow the optimizer to keep that state in registers, where it's fastest.

2 Likes
9

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.