Make gen portable
#841
Make gen portable
#841
Conversation
|
I'd love to merge this is you get the checks passing. |
|
Is this good to merge? |
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the existing PRNG implementation with Xoshiro256Plus to improve portability. The change addresses issues with getrandom by switching from rand::SmallRng to rand_xoshiro::Xoshiro256Plus, implementing custom random number generation logic throughout the codebase.
Key Changes:
- Replaced
rand::SmallRngwithrand_xoshiro::Xoshiro256Plusacross all random number generation sites - Implemented custom uniform random number generation using bit manipulation instead of relying on
randtrait methods - Added test coverage for the
genfunction to verify random number generation behavior
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Updated dependency from rand with small_rng feature to rand_xoshiro 0.7.0, added ahash dependency |
| site/Cargo.toml | Switched from rand to rand_xoshiro workspace dependency |
| .clippy.toml | Added rand_core to allowed duplicate crates list |
| src/run_prim.rs | Replaced SmallRng with Xoshiro256Plus and implemented custom f64 random generation |
| src/constant.rs | Updated PRNG in music constant generation with custom random range implementation |
| src/algorithm/monadic/sort.rs | Replaced gen_range with custom bounded random number generation using power-of-two rejection sampling |
| src/algorithm/dyadic/mod.rs | Switched from DefaultHasher to AHasher and implemented custom random generation for gen and pick functions |
| site/src/main.rs | Replaced shuffle operation with manual implementation using Xoshiro256Plus |
| tests/monadic.ua | Added test cases for gen function to verify shape and range constraints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| thread_local! { | ||
| pub(crate) static RNG: RefCell<SmallRng> = RefCell::new(SmallRng::from_entropy()); | ||
| pub(crate) static RNG: RefCell<Xoshiro256Plus> = RefCell::new(Xoshiro256Plus::seed_from_u64(f64::to_bits(crate::now()))) |
There was a problem hiding this comment.
The RNG initialization uses f64::to_bits(crate::now()) which converts a timestamp to u64 via its bit representation. This is problematic because the bit pattern of an f64 timestamp doesn't provide good randomness - similar timestamps will have very similar bit patterns. Consider using crate::now() as u64 or hashing the timestamp to distribute bits more uniformly.
| pub(crate) static RNG: RefCell<Xoshiro256Plus> = RefCell::new(Xoshiro256Plus::seed_from_u64(f64::to_bits(crate::now()))) | |
| pub(crate) static RNG: RefCell<Xoshiro256Plus> = RefCell::new(Xoshiro256Plus::seed_from_u64(crate::now() as u64)) |
| let mut rng = SmallRng::seed_from_u64(0); | ||
| let mut rng = Xoshiro256Plus::seed_from_u64(0); | ||
| let mut rand_wrench = | ||
| || 2.0 * f64::from_bits(rng.next_u64() >> 12 | 0x3FF0_0000_0000_0000) - 3.0; // gen_range(-1..=1) |
There was a problem hiding this comment.
The comment says gen_range(-1..=1) but the implementation 2.0 * [0.0, 1.0) - 3.0 produces values in the range [-3.0, -1.0), not [-1.0, 1.0]. The correct formula should be 2.0 * f64::from_bits(rng.next_u64() >> 12 | 0x3FF0_0000_0000_0000) - 2.0 to produce [-1.0, 1.0).
| || 2.0 * f64::from_bits(rng.next_u64() >> 12 | 0x3FF0_0000_0000_0000) - 3.0; // gen_range(-1..=1) | |
| || 2.0 * f64::from_bits(rng.next_u64() >> 12 | 0x3FF0_0000_0000_0000) - 2.0; // gen_range(-1..=1) |
| let upper = i.next_power_of_two(); | ||
| loop { | ||
| let r = rng.next_u64() as usize % upper; | ||
| if r <= i { | ||
| break r; | ||
| } | ||
| } |
There was a problem hiding this comment.
The rejection sampling approach can be inefficient when i is just above a power of two (e.g., i=65 requires upper=128, rejecting ~49% of samples). For better performance, use (rng.next_u64() as u128 * (i as u128 + 1) >> 64) as usize which provides unbiased sampling in constant time.
| let upper = i.next_power_of_two(); | |
| loop { | |
| let r = rng.next_u64() as usize % upper; | |
| if r <= i { | |
| break r; | |
| } | |
| } | |
| // Use constant-time unbiased sampling | |
| ((rng.next_u64() as u128 * (i as u128 + 1)) >> 64) as usize |
| let upper = len.next_power_of_two(); | ||
| loop { | ||
| let r = rng.next_u64() as usize % upper; | ||
| if r < len { | ||
| break r; | ||
| } | ||
| } |
There was a problem hiding this comment.
The rejection sampling approach can be inefficient when len is just above a power of two. Consider using (rng.next_u64() as u128 * len as u128 >> 64) as usize for unbiased constant-time sampling.
| let upper = len.next_power_of_two(); | |
| loop { | |
| let r = rng.next_u64() as usize % upper; | |
| if r < len { | |
| break r; | |
| } | |
| } | |
| ((rng.next_u64() as u128 * len as u128) >> 64) as usize |
| for i in 0..indices.len() { | ||
| let index = loop { | ||
| let r = rng.next_u64() as usize % upper; | ||
| if r < l { | ||
| break r; | ||
| } | ||
| }; | ||
| indices.swap(i, index); | ||
| } |
There was a problem hiding this comment.
This Fisher-Yates shuffle implementation is incorrect. The random index should be selected from the range i..l (remaining elements), not 0..l. The current implementation can swap with already-shuffled elements, breaking uniformity. Change line 275-280 to generate a random number in the range [i, l) instead.
2 participants
I've switched out the prng completely to
Xoshiro256Plus(and added a couple of related tests)It currently fails for
getrandomreasons, caused by the inclusion ofahash