core: try to use small integers for uniform sampling

[joboet]

Tracking issue: #130703

When sampling from e.g. 0u128..=3u128 we can avoid the expensive 128-bit widening multiplications required for a full 128-bit bound and perform arithmetic using 32-bit integers instead. I've applied this truncation to power-of-two cases, too, for consistency reasons: sampling an 8-bit or 16-bit number will now always draw a 32-bit number from the RNG, except in the case where the range is equivalent to the full range of the type.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, nia-e

[Mark-Simulacrum]

nit: Should this use RangeFull or the new code use the (..) syntax?

View changes since the review

[joboet]

Mmmh, yeah, I wasn't too careful about consistency when I added this. I don't have a preference here, what's yours?

[Mark-Simulacrum]

I don't have a strong opinion either way, I think I could see either being nicer.

[Mark-Simulacrum]

I think this implementation is increasing the bytes we sample from the underlying RNG for u8 and u16 to 4 (from 1 and 2 respectively). Are we OK with that?

It looks like that is not done for RangeInclusive where the start/end overflow (there we hit RangeFull.sample under the original type, e.g., u8/u16.

View changes since the review

[joboet]

Yes, exactly. As I tried to explain in the PR description, I think this makes sense for consistency reasons (and likely won't affect performance in any significant way) – but I'm happy to be convinced of the contrary!

[Mark-Simulacrum]

I guess I'm trying to understand what you mean by "consistency". Why is <= 32 bit more important to keep 'consistent' rather than 64/128 bits? Is it just that u32 is 'fast enough'?

[joboet]

Non-power-of-two range lengths always use 32-bit sampling (for 8- and 16-bit outputs) to reduce the chance of rejections. So with the current code there is a difference in the amount of data drawn from the RNG depending on whether the range length is a power of two or not, and that seems a bit inconsistent to me.

[Mark-Simulacrum]

                let Some(bound) = (self.last - self.start).checked_add(1) else {
                    // Overflow can only occur for Self::MIN..=Self::MAX, meaning
                    // the range is effectively unbounded.
                    return RangeFull.sample(source);
                };

I think that case still samples from the smaller integer type (u8/u16), right? So this isn't actually always using 32-bit sampling?

My suspicion (maybe wrong) is that depending on the underlying RNG's implementation -- not sure what we landed on / will land on for std -- if you use 32-bit integers to sample (say) 0..10, it'll be ~4x slower than sampling from u8 as the major cost is (probably) the chacha or whatever RNG you're using, right?

If that claim is true, then it seems like we ought to use the smallest possible number of bytes pulled from the underlying RNG (maybe rounded to powers of two, i.e., not trying to pull 3/6/7 bytes)?

Maybe the underlying machine instructions aren't as efficient so it ends up slower, but it feels a bit surprising to me that we don't mask off bounded.next_power_of_two() to increase probability of a hit in the sampling algorithm...

[Mark-Simulacrum]

Why the wrapping_add here? Shouldn't offset always fit in the integer type when added to start? Or is it off by one?

View changes since the review

[joboet]

Oh, just because the signed code uses wrapping too (there is no version of add_unsigned that only checks for overflow in the debug case). This can never overflow, the result is guaranteed to fit in the range after all.