switch gen_biguint to fill_bytes #53
Conversation
| // Swap bytes per the `Rng::fill` source. This might be | ||
| // unnecessary if reproducibility across architectures is not | ||
| // desired. | ||
| data.to_le(); |
There was a problem hiding this comment.
This is an interesting question -- what guarantees should we make about reproducibility? Is this PR a breaking change altogether, since seeded RNGs may now produce a different BigUint? Maybe that's going too far to consider this across crate versions, but if we want it across architectures, we'll also need to be careful in the future about conditional BigDigit sizes, for instance. (Right now this is using the public BigUint::new though, which should always be u32-based even if the internals change.)
There was a problem hiding this comment.
This may be a value-breaking change (depending on the PRNG used), but I think will usually not be. (Rand does not specify that fill_bytes must output the same bits in the same order as repeated calls to next_u32, though I think this is the case for all current generators.)
|
I don't know why the |
|
No, it's just waiting for me to approve it. Did you have any more thoughts about what (if anything) we should guarantee, when producing numbers from a seeded rng? Also, yes, adding the benches would be nice. |
|
I don't really use the crate, I just happened to see this implementation could be improved. So I'm not sure. |
|
You could add a deprecated function using the old method. I’m not sure how useful that would be without implementing all the surrounding code though. |
|
OK, I've added some tests of value stability. I generated the expected values on the current master branch, and they do still match on this branch. I think we should go ahead and make this change, and mention the caveat about possible effects on reproducibility (depending on PRNG) in the release notes. Before I merge, I'm going to see if I can run this on Fedora's infrastructure with all architectures, but I need to update a few dependencies there first -- including |
|
All arches passed, including big-endian ppc64 and s390x. Thanks! bors r+ |
53: switch gen_biguint to fill_bytes r=cuviper a=TheIronBorn Changes `gen_biguint` from a `push(gen::<u32>)` method to rand's [`fill_bytes`](https://docs.rs/rand/0.5.0/rand/trait.RngCore.html#tymethod.fill_bytes). This should improve performance in most cases. - For small PRNGs which only natively generate 64 bits (like Xorshift64 or [`splitmix64.c`](http://prng.di.unimi.it/splitmix64.c)), this will no longer throw away half the bits generated. - For block PRNGs like `StdRng`, this should reduce overhead. - For an SIMD PRNG (rust-random/rand#377), this would be a significant improvement. ```diff,ignore name no_fill ns/iter fill ns/iter diff ns/iter diff % speedup +rand_1009 256 222 -34 -13.28% x 1.15 +rand_131072 27,366 14,715 -12,651 -46.23% x 1.86 +rand_2048 459 357 -102 -22.22% x 1.29 -rand_256 93 130 37 39.78% x 0.72 +rand_4096 842 557 -285 -33.85% x 1.51 -rand_64 69 92 23 33.33% x 0.75 +rand_65536 13,625 7,382 -6,243 -45.82% x 1.85 +rand_8192 1,836 869 -967 -52.67% x 2.11 ``` (i.e. `rand_1009` does `gen_biguint(1009)`. All benches are powers of two except `rand_1009`) (Let me know if you want the `rand_` benches added) Co-authored-by: TheIronBorn <> Co-authored-by: Josh Stone <[email protected]>
Build succeeded |
Changes
gen_biguintfrom apush(gen::<u32>)method to rand'sfill_bytes. This should improve performance in most cases.splitmix64.c), this will no longer throw away half the bits generated.StdRng, this should reduce overhead.(i.e.
rand_1009doesgen_biguint(1009). All benches are powers of two exceptrand_1009)(Let me know if you want the
rand_benches added)