add RandomSource::next_u{32,64} methods#157193
Conversation
|
|
This comment was marked as outdated.
This comment was marked as outdated.
| impl_primitive!(u8); | ||
| impl_primitive!(i8); | ||
| impl_primitive!(u16); | ||
| impl_primitive!(i16); | ||
| impl_primitive!(u32); | ||
| impl_primitive!(i32); | ||
| impl_primitive!(u64); | ||
| impl_primitive!(i64); | ||
| impl_primitive!(u32, next_u32); | ||
| impl_primitive!(i32, next_u32); | ||
| impl_primitive!(u64, next_u64); | ||
| impl_primitive!(i64, next_u64); | ||
| impl_primitive!(u128); | ||
| impl_primitive!(i128); | ||
| impl_primitive!(usize); | ||
| impl_primitive!(isize); |
There was a problem hiding this comment.
Not directly a question about this addition, but if we want to stabilize Distribution and be serious about its reproducibility, we may need to consider if we want to add next_uN for other N and/or conditionally use read_uN for usize/isize as well.
|
LGTM, but as I wrote in the issue, I believe |
| fn next_u32(&mut self) -> u32 { | ||
| let mut buf = [0; size_of::<u32>()]; | ||
| self.fill_bytes(&mut buf); | ||
| u32::from_ne_bytes(buf) |
There was a problem hiding this comment.
Considering that most PRNGs generate either u32 or u64, IMO it's better to remove these blanket impls.
There was a problem hiding this comment.
That would gratuitously make more work for implementers of RNGs that just read out bytes, which includes hardware RNGs. Default impls add zero overhead for implementations that don't use them.
There was a problem hiding this comment.
My point is that most RNG implementations will override those blanket impls. If we are to exclude the system source, then IO-based RNGs represent an extremely niche RNG class. If you want to reduce downstream work, then it would make more sense to provide blanket impls in terms of next_u32.
Ideally, we would introduce a WordRng trait, but unfortunately it would not work on current stable Rust (see the linked rand_core issue).
There was a problem hiding this comment.
My point is that most RNG implementations will override those blanket impls.
@newpavlov I'm unclear on what problem you're trying to solve, here. A default impl doesn't have to be useful to everyone to be useful. What problem does the default impl cause?
There was a problem hiding this comment.
My problem is that the impls are virtually never useful. For non-deterministic IO-based RNGs you want to use from_ne_bytes instead of from_le_bytes suggested below, while for word-based RNGs they will be always overwritten. It also arguably creates a false guidance, users may see fill_bytes as the "fundamental" method even if their RNG produces u32 or u64.
|
Now that I accidentally nerd-sniped @joshtriplett into optimizing my RNG's Results on my machinerunning 24 tests test cc8r::next_u32_direct ... bench: 2.04 ns/iter (+/- 0.10) test cc8r::next_u32_direct_dyn ... bench: 1.88 ns/iter (+/- 0.07) test cc8r::next_u32_via_bytes ... bench: 1.98 ns/iter (+/- 0.04) test cc8r::next_u32_via_bytes_dyn ... bench: 4.42 ns/iter (+/- 0.38) test cc8r::next_u64_direct ... bench: 2.71 ns/iter (+/- 0.08) test cc8r::next_u64_direct_dyn ... bench: 2.69 ns/iter (+/- 0.11) test cc8r::next_u64_via_bytes ... bench: 2.56 ns/iter (+/- 0.03) test cc8r::next_u64_via_bytes_dyn ... bench: 4.41 ns/iter (+/- 0.10) test xoshiro128::next_u32_direct ... bench: 1.35 ns/iter (+/- 0.03) test xoshiro128::next_u32_direct_dyn ... bench: 2.05 ns/iter (+/- 0.04) test xoshiro128::next_u32_via_bytes ... bench: 1.35 ns/iter (+/- 0.03) test xoshiro128::next_u32_via_bytes_dyn ... bench: 3.39 ns/iter (+/- 0.12) test xoshiro128::next_u64_direct ... bench: 1.94 ns/iter (+/- 0.01) test xoshiro128::next_u64_direct_dyn ... bench: 3.07 ns/iter (+/- 0.06) test xoshiro128::next_u64_via_bytes ... bench: 1.94 ns/iter (+/- 0.02) test xoshiro128::next_u64_via_bytes_dyn ... bench: 6.79 ns/iter (+/- 0.19) test xoshiro256::next_u32_direct ... bench: 1.37 ns/iter (+/- 0.02) test xoshiro256::next_u32_direct_dyn ... bench: 2.07 ns/iter (+/- 0.22) test xoshiro256::next_u32_via_bytes ... bench: 1.35 ns/iter (+/- 0.03) test xoshiro256::next_u32_via_bytes_dyn ... bench: 4.32 ns/iter (+/- 0.44) test xoshiro256::next_u64_direct ... bench: 1.36 ns/iter (+/- 0.32) test xoshiro256::next_u64_direct_dyn ... bench: 2.04 ns/iter (+/- 0.04) test xoshiro256::next_u64_via_bytes ... bench: 1.35 ns/iter (+/- 0.01) test xoshiro256::next_u64_via_bytes_dyn ... bench: 3.42 ns/iter (+/- 0.21) |
| fn next_u32(&mut self) -> u32 { | ||
| let mut buf = [0; size_of::<u32>()]; | ||
| self.fill_bytes(&mut buf); | ||
| u32::from_ne_bytes(buf) |
There was a problem hiding this comment.
| u32::from_ne_bytes(buf) | |
| u32::from_le_bytes(buf) |
AFAICT this would be necessary for reproducibility, right?
There was a problem hiding this comment.
It's necessary if you care about reproducibility across LE and BE platforms, which is a reasonable position that I agree with. I used from_ne_bytes because that's what the Distribution impls do, but we should probably just update everything to use LE consistently.
There was a problem hiding this comment.
I used from_ne_bytes because that's what the Distribution impls do
Ah, oops. Yes, those should be fixed too.
There was a problem hiding this comment.
I added a commit here changing this (let me know if you'd rather have a separate PR)
There was a problem hiding this comment.
On second though, I decided to split this out into #157430 because it's independent of adding next_uN methods and I don't want it to be forgotten if we end up not adding the methods.
If we're going to add these methods at any point, it's better to add them before stabilization. Since they should be allowed to behave differently from `fill_bytes` calls, existing code that calls `fill_bytes` (including the standard library's `Distribution` impls) would break reproducibility by switching to `next_uN` later. Similarly, `RandomSource` implementations that want to guarantee reproducibility either have to override these methods from the start or never override them. The main reason why we should add these methods is, of course, performance. The existing contract of `fill_bytes` helps when an `impl RandomSource` generates one word at a time since `fill_bytes` calls that generate exactly one word's worth of data can be inlined and simplified to avoid the general loop. However, for `dyn RandomSource`, the `fill_bytes` call can't be inlined, so the desired optimization doesn't kick in. In contrast, `next_uN` methods in the vtable make direct, inlinable calls to `fill_bytes`.
7e1f1d4 to
1f4b26c
Compare
|
|
||
| /// Returns a random 32-bit integer. | ||
| /// | ||
| /// The default implementation uses `fill_bytes` and interprets those bytes as integer, but this |
There was a problem hiding this comment.
| /// The default implementation uses `fill_bytes` and interprets those bytes as integer, but this | |
| /// The default implementation uses `fill_bytes` and interprets those bytes as a little-endian integer, but this |
There was a problem hiding this comment.
I guess the question is whether we want to promise this, or reserve the right to change it (at least before stabilization, but maybe even later). Personally I think native-endian isn't worth the portability hazard and little-endian is a good default, but I've already seen one comment above advocating for native-endian.
There was a problem hiding this comment.
For context, rand uses LE conversions and I don't recall ever receiving any criticism/complaints regarding this choice. Possibly because there isn't much BE hardware about today (or at least, not much intersection of Rust and BE systems).
|
|
||
| /// Returns a random 64-bit integer. | ||
| /// | ||
| /// The default implementation uses `fill_bytes` and interprets those bytes as integer, but this |
There was a problem hiding this comment.
| /// The default implementation uses `fill_bytes` and interprets those bytes as integer, but this | |
| /// The default implementation uses `fill_bytes` and interprets those bytes as a little-endian integer, but this |
I agree. That said, we seem to be pretty happy with the |
If we're going to add these methods at any point, it's better to add them before stabilization. Since they should be allowed to behave differently from
fill_bytescalls, existing code that callsfill_bytes(including the standard library'sDistributionimpls) would break reproducibility by switching tonext_uNlater. Similarly,RandomSourceimplementations that want to guarantee reproducibility either have to override these methods from the start or never override them.The main reason why we should add these methods is, of course, performance. The existing contract of
fill_byteshelps when animpl RandomSourcegenerates one word at a time sincefill_bytescalls that generate exactly one word's worth of data can be inlined and simplified to avoid the general loop. However, withdyn RandomSource, thefill_bytescall can't be inlined, so the desired optimization doesn't kick in. In contrast,next_uNmethods in the vtable make direct, inlinable calls tofill_bytes.Tracking issue: #130703
r? @joshtriplett