smol-rs / fastrand

A simple and fast random number generator
Apache License 2.0
386 stars 33 forks source link

Always use getrandom to seed RNG #86

Closed cod10129 closed 1 month ago

cod10129 commented 1 month ago

This PR makes the default random RNG seed always use getrandom. The original code looked like this:

fn random_seed() -> Option<u64> {
    use std::collections::hash_map::DefaultHasher;
    use std::hash::{Hash, Hasher};
    use std::thread;
    use std::time::Instant;

    let mut hasher = DefaultHasher::new();
    Instant::now().hash(&mut hasher);
    thread::current().id().hash(&mut hasher);
    Some(hasher.finish())
}

DefaultHasher doesn't actually have any randomness in it, so you're just hashing the current time and thread ID to seed RNG. This works, but it seemed wrong to me when I saw it. There's an implementation with getrandom right there.

A few other things I noticed: The #[cfg(target_pointer_width = "128")] at lib.rs line 622 is generating a warning, which has been failing CI since May, after the unexpected_cfgs lint was added in Rust 1.80. Unrelated but I also really like the cat logo.

notgull commented 1 month ago

This works, but it seemed wrong to me when I saw it. There's an implementation with getrandom right there.

It doesn't really matter, this is a pseudorandom number generator, so it doesn't matter if the seed has cryptographic level strength. It's just that some seed is nice to have here. Unfortunately the thread::id() and time tricks aren't available on WASM, hence the getrandom implementation.

I would also rather keep fastrand dependency free if possible.

The #[cfg(target_pointer_width = "128")] at lib.rs line 622 is generating a warning, which has been failing CI since May, after the unexpected_cfgs lint was added in Rust 1.80.

This is tracked in #85

Unrelated but I also really like the cat logo.

Thanks! It was drawn by NebulousStar on Twitter.

cod10129 commented 1 month ago

Alright, fair points. Seeding with the time is often used, but does it also need the thread ID? Note the main thread is always ThreadId(1).

I would also rather keep fastrand dependency free if possible.

I was actually wondering if the dependency was the reason. getrandom also pulls in cfg-if and (on Unix) libc. It is a nice cargo tree to just see a self-contained fastrand crate.

notgull commented 1 month ago

Note the main thread is always ThreadId(1).

Keep in mind GLOBAL_RNG is a thread-local variable, so it will be initialized with a different thread ID every time.

I was actually wondering if the dependency was the reason. getrandom also pulls in cfg-if and (on Unix) libc. It is a nice cargo tree to just see a self-contained fastrand crate.

Agreed, this is my problem with getrandom, in addition to the fact that it pulls libc instead of rustix on Linux.

cc https://github.com/rust-random/getrandom/issues/401

cod10129 commented 1 month ago

The RNG is thread-local, but two different threads aren't initializing their RNGs at the same nanosecond. I went through the performance implications:

Seems negligible, since there's also hashing and Instant::now happening. I'm thinking I'll just close this PR, unless there's anything you want to say?

notgull commented 1 month ago

The RNG is thread-local, but two different threads aren't initializing their RNGs at the same nanosecond.

This isn't guaranteed.

I'm thinking I'll just close this PR, unless there's anything you want to say?

Yeah, I'd agree with closing the PR.

notgull commented 1 month ago

Thanks anyways!