Skip to content

CHANGE: Make <SmallRng as SeedableRng>::Seed the same type on 32 and 64 bit platforms #1285

Closed
@thomcc

Description

Summary

Change SmallRng's implementation of SeedableRng so that the Seed is the same type and size on all targets (currently, <SmallRng as SeedableRng>::Seed is a [u8; 32] if cfg(target_pointer_width = "64") and a [u8; 16] if cfg(target_pointer_width = "32")).

This would be a breaking change, which will need to wait for the next semver-major release (currently that would be 0.9)

Details

There are a few ways this may be implemented that would satisfy me.

  1. Use the same underlying RNGs as are currently used, and increase the size of the seed used on 32 bit platforms to [u8; 32]. This is double the size the RNG actually needs, so for use it would need to be combined somehow (or have a portion discarded). There are many possible ways to do this, and the details do not seem particularly important.
  2. Use the same underlying RNGs as are currently used, and decrease the size of the seed used on 64 bit platforms to [u8; 16]. This is half the size the RNG actually needs, so it would need to be stretched into a [u8; 32] before use. As before, there are ways to do this,
  3. Switch the underlying RNG used under either (or both) target configurations to something that is consistent on both, or at least uses the same seed size/argument on both.

Of these I think 1 or 3 is the best, but do not feel particularly strongly.

The first two have the drawback that the seed would no longer corresponds directly to the state (or to the seed of the wrapped RNG). This seems to be allowed by the documentation of SeedableRng::from_seed already, and SmallRng being an abstraction probably gives you leeway to do things like it, but I suppose it is a downside.

The last one has a downside that it may be difficult to find algorithms that run well on 32 and 64 bit platforms (although I don't know that I believe this).

Motivation

rand::rngs::SmallRng's public API currently contains a portability hazard. Specifically, if you use SmallRng::from_seed, you need to pass a [u8; 32] on 64 bit targets and a [u8; 16] on 32 bit ones. Users who are writing portable code must notice this difference and either avoid the function or change argument passed into from_seed based on a #[cfg].

The fact that a different algorithm is used on different platforms is mentioned in the documentation, but it is not made clear that this impacts the API in a way other than the sequence of values output by the RNG. To me (and I could have misread the intent), the documentation seemed to indicate that the underlying RNG should considered an implementation detail (which is good, if I wanted a specific RNG implementation, I'd use that directly), implying I shouldn't have to worry about it (except beyond not expecting identical results across platforms).

I hit this as a compilation failure on 32 bit platforms in rust-lang/rust#104658 when updating the Rust standard library's version of rand used in benchmarks and tests, and found it quite surprising. I ended up switching to rand_xorshift to provide the RNG implementation, because it was less of a portability headache (even though I didn't care about the underlying algorithm).

I'd have preferred to continue using SmallRng (and had I noticed seed_from_u64 perhaps I'd have used that), but it still strikes me as pretty odd to have something like this in the public API on what's already intended to be a wrapper that abstracts away from the specific underlying algorithm — IOW, it feels like an abstraction leak.

Given how popular rand is, and the fact that most developer machines are 64 bit these days... I suspect there's some amount of rand users who have code which does not compile on 32 bit platforms as a result, which seems unfortunate.

Alternatives

Several alternatives for how to fix this are given under "Details", above. Those all seem pretty good to me, and relatively simple.

Aside from those, another option would be to overhaul the SeedableRng API to help avoid this sort of issue. That may help, but in general this kind of issue can crop up anywhere cfg is used unless care is taken to avoid it... so I don't know that it's an issue with SeedableRng in particular.

Finally, some alternatives which I don't find particularly compelling:

  1. Try to fix it with documentation.
  2. Do nothing. Users concerned with portability should use a different API or match the seed types with cfg.
  3. Come up with a way to move the error to runtime, but change nothing else. (I'm not sure what this one would look like specifically, but it seems disappointing)

P.S. Sorry if this has been discussed, I did a search and it did not come up.

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions