zig icon indicating copy to clipboard operation
zig copied to clipboard

Random.shuffle produces different orderings on 32 and 64 bit systems

Open danielchasehooper opened this issue 1 year ago • 5 comments

Zig Version

0.10.0-dev.4475+5b9c8d1d6

Steps to Reproduce

I'm working on some code that targets arm64 and wasm32. Random.shuffle produces different orderings on the two systems, and this is causing headaches with reproducibility when comparing the two builds.

Expected Behavior

shuffle's signature doesn't give any indication that its behavior is platform dependent - So I'd expect it to behave the same regardless of target.

Actual Behavior

Random.shuffle produces different orderings on 32 and 64 bit systems due to using Random.intRangeLessThan(usize, ...) (usize is 32bits on wasm32 and 64bits on arm64)

I had to implement my own shuffle function in order to get consistent behavior across platforms

danielchasehooper avatar Oct 21 '22 13:10 danielchasehooper

Random.enumValue seems to suffer the same issue: https://github.com/ziglang/zig/blob/680d3cd1fc285a661bb32f90090a8ea2da986f75/lib/std/rand.zig#L70 So we should probably come up with a solution that fixes that too.

I'm not sure the fix is as simple as using either u32 or u64, or is it? Maybe it's fine to use u32 for both?

But if using usize is significant, we might have to kind of add an offset to the final result of the usize-involving calculation depending on the architecture's bits that makes sure that the final result is consistent across all architectures.

Finally we should probably add tests that check that an RNG seeded with 0 yields the same results on all architectures.

wooster0 avatar Oct 21 '22 14:10 wooster0

A possible solution would be to just optionally take the index type as a parameter, where null is the same as using usize, or mayhaps some fixed type like u32.

InKryption avatar Oct 21 '22 15:10 InKryption

It would be nice if we could keep the API as-is but if we end up doing that I would not make it optional though, like any other type parameter. We can explain in the docs why usize might be a good default and we can alternatively suggest a fixed size if it should be architecture-independent.

wooster0 avatar Oct 21 '22 15:10 wooster0

Another option: pick a uint width at runtime based on array.len, so an array with len <= 256 could use Random.intRangeLessThan(u8, ...), longer arrays could use u16/u32/u64 (or some subset of those).

danielchasehooper avatar Oct 21 '22 15:10 danielchasehooper

Or could uintLessThen be reimplemented so that it returns the same value regardless of the result type?

danielchasehooper avatar Oct 21 '22 17:10 danielchasehooper