Cherry-pick changes to PRNG algorithms #209

dhardy · 2017-12-15T12:28:41Z

Notable changes:

Rng::next_u64 and Rng::fill_bytes no longer have default implementations
impls module added (private; the idea is that this gets exported as rand_core::impls later)
Isaac code massively improved and performance improved by @pitdicker
ChaCha code also improved to a lesser extent

Note that this is based on core (PR #208) which accounts for the changes to Cargo.toml and the README.

Also note: yes, I merged these 17 commits by hand (ugh) and ran the tests for each. Reviewing in detail from this PR will be horrible; it may be easier to review the original commits if you want to do that. But everything has already been well reviewed by @pitdicker and myself and there are a bunch of tests, so I'm not too worried.

dhardy · 2017-12-15T12:38:15Z

Edit: fixed for Rust 1.15

pitdicker · 2017-12-16T14:13:04Z

I have read over the changes, and it looks like a lot of work backporting! It looks good, there is nothing I could comment on.

The change to make IsaacWordRng a wrapper instead of a 'use' is missing. But the only purpose of that change is for better documentation, and not that interesting for now.

dhardy · 2017-12-16T17:01:23Z

Yes, I deliberately missed out IsaacWordRng; it's not exposed and I'm not sure it's useful.

dhardy · 2017-12-17T17:58:29Z

Rebased (first commit moving Issac64 to a new module merged to make diff easier to read)

dhardy · 2017-12-18T14:20:44Z

Breaking changes (see also here):

Rng::next_u64 and Rng::fill_bytes no longer have default implementations
PRNGs no longer implement Copy
new_unseeded fns removed from ISAAC variants [replaced by new_from_u64; easy to provide a deprecated wrapper fn]
Isaac64Rng::next_u32 produces different results
Endianness differs in default next_u64 implementation (also fill_bytes?)
Debug implementations no longer show internals [unimportant]

Not sure at the moment whether to try to work around the above (not so easy) or make this wait until 0.5.

dhardy · 2017-12-18T17:16:50Z

Ok, I reverted all the above breaking changes (except to Debug) so that this PR can land as 0.4.2.

The commits affecting Isaac64Rng::next_u32 were removed entirely and will be re-added later.

dhardy · 2017-12-20T15:07:38Z

src/lib.rs

    fn next_u64(&mut self) -> u64 {
-        ((self.next_u32() as u64) << 32) | (self.next_u32() as u64)
+        impls::next_u64_via_u32(self)


This is also a breaking change (was BE, or possibly undefined order)

No, it turns out the old code also produced LE values (for me at least).

Hmm, having that as a breaking change does not make me happy.

I double-checked your results, and if I add the test on the master branch it fails...

Thanks for picking that up. I forgot IsaacRng already has it's own impl of next_u64! Now I get BE results from the old code too. Ok, will revert. Does this apply to fill_bytes too?

dhardy · 2017-12-20T17:05:16Z

Actually, this is more complicated: the next_u64 impls for ChaCha and Xorshift also need to be removed, as do the new fill_bytes implementations. That means some of your optimisations are not applicable, unfortunately. So I'm not sure whether it's worth merging this in 0.4.x at all? To add insult to injury most of the tests I'd like to check (e.g. true_bytes) didn't exist before.

pitdicker · 2017-12-20T17:12:16Z

Are you sure fill_bytes has changed? But I was just thinking the same thing, how much is left that can land? The code for isaac::next_u64 can be adjusted by just swapping x and y though.

dhardy · 2017-12-21T11:37:49Z

Well, there was change in output (I tried running the new true_bytes test with old code).

The problem here is that these changes affect output that was not previously tested, and that we want to change anyway (standardise to LE). I don't want to write new tests to confirm we didn't break output that was untested before and will soon be obsolete. So unless someone else does that, I think I'll just merge all breaking changes except for the removal of default implementations in Rng (which is an API change likely to hit many Rng impls) for 0.5.

pitdicker · 2017-12-21T14:01:05Z

I was asking more because I was careful to preserve the order of fill_bytes in the "Fill isaac64 backwards" commit. But that was after the switch to LE. Now that I see the old code, it was first combining two u32 in a big-endian u64, and then using that to return bytes in what I think is little endian.

I think I'll just merge all breaking changes except for the removal of default implementations in Rng (which is an API change likely to hit many Rng impls) for 0.5.

Seems like a good plan to me. As you say we want those changes in the end, so why go through all the hoops just to remain in the 0.4.* version.

I am not sure what you want to wait for to remove the default implementations in Rng. If there where many users of 0.4.* I would wait. And if there was a big upcoming version to collect these breaking changes. But that is not the release model we ended up with?

This removes the unsafe blocks from the main isaac and isaac64 routines. Some macro's are replaced by functions. Over time the implementation of ISAAC and ISAAC-64 have diverged a bit, now they are as similar as possible (for easier comparison). The code is tested against the previous implementation, and the reference implementation. IsaacRng now does 34% better in the benchmark. [Cherry-picked from 4747665]

for next_u64 and fill_bytes This is based on dd1241a but heavily modified

[Cherry-picked from 14d0561]

Cherry-picked from adcd8e5 [by Paul Dicker, 19th October] I have implemented it as a function instead of a trait, as that makes it easy to add it to every RNG ont at a time. I split the `init` function in two instead of the current version that uses a bool to select between two paths. This makes it more clear how the seed is used. The current `mix` macro has to be defined in the function, and would have to be duplicated. Therefore I converted it to a seperate function. I precalculated the values a...h, but am not sure this is a good idea. It makes the resulting code smaller, and gives a small performance win. Because it are 'magic' values anyway, I thought why not?

[Cherry-picked from 130b64c]

So the internal state is never exposed (may be security-sensitive) [Cherry-picked from e513aaa]

…mpls [Cherry-picked from ae365ef]

Includes both the values output now and the values which should be output by #36. [Cherry-picked from fd2660b]

This also replaces `core::num::Wrapping` with a few `wrapping_add`'s. There were about 30 conversions to and from `Wrapping`, while there are only 9 wrapping operations. Because `fill_via_u32_chunks` expects a `[u32]`, converting away was just easier. [Cherry-picked from d7b014c]

Also uses a different solution to index without bounds checking, to recover a very little bit of lost performance. [Cherry-picked from f92a347]

[Cherry-picked from 707c3e1]

This is 45% faster. We are no longer throwing away half of the results. [Cherry-picked from 415ef6f and 0bdb1c3]

[Cherry-picked from 5f4bedf]

Restore default implementations of next_u64 and fill_bytes Restore new_unseeded functions (as wrappers)

dhardy · 2017-12-27T12:03:34Z

Updated (adjusted the "revert breaking changes" commit and squashed a couple of commits together to simplify). Due to breaking changes, this will land in 0.5.

This means that breaking changes are those above minus the removal of default implementations and of new_unseeded.

pitdicker · 2018-01-02T14:48:42Z

🎉

dhardy requested a review from alexcrichton December 15, 2017 12:28

dhardy mentioned this pull request Dec 15, 2017

Add HC-128 RNG dhardy/rand#74

Merged

dhardy force-pushed the prng branch from 8c2d345 to 08deed6 Compare December 15, 2017 13:52

pitdicker mentioned this pull request Dec 16, 2017

Add HC-128 #210

Merged

dhardy mentioned this pull request Dec 17, 2017

Release strategy #205

Closed

dhardy force-pushed the prng branch from 08deed6 to 3eac806 Compare December 17, 2017 17:57

pitdicker mentioned this pull request Dec 18, 2017

Add Serde #189

Merged

dhardy mentioned this pull request Dec 18, 2017

Replace HTTP links with HTTPS #179

Merged

dhardy force-pushed the prng branch from 3eac806 to a720e5f Compare December 18, 2017 17:14

dhardy commented Dec 20, 2017

View reviewed changes

Paul Dicker and others added 10 commits December 27, 2017 11:43

Add impls module; replace custom impls; remove default impls

9c99933

for next_u64 and fill_bytes This is based on dd1241a but heavily modified

Fix: allow compiling with Rust 1.15

78912da

Add some extra documentation

5485199

[Cherry-picked from 14d0561]

Simplify isaac init code

e8dbf2e

[Cherry-picked from 130b64c]

Remove Copy trait from ChaCha

5c300f6

Custom Debug implementation for ChaCha and Xorshift

d16b9cb

So the internal state is never exposed (may be security-sensitive) [Cherry-picked from e513aaa]

Add true_bytes tests for ChaCha and Isaac; fix 2 bugs in fill_bytes i…

164f289

…mpls [Cherry-picked from ae365ef]

Isaac64: add test for true 32-bit values

a7e9d77

Includes both the values output now and the values which should be output by #36. [Cherry-picked from fd2660b]

pitdicker and others added 6 commits December 27, 2017 11:49

Fill isaac backwards, and use fill_via_u32_chunks

74660c2

Also uses a different solution to index without bounds checking, to recover a very little bit of lost performance. [Cherry-picked from f92a347]

Fill isaac64 backwards, and use fill_via_u32_chunks

62b5722

[Cherry-picked from 707c3e1]

Improve performance of isaac64::next_u32.

865ed54

This is 45% faster. We are no longer throwing away half of the results. [Cherry-picked from 415ef6f and 0bdb1c3]

Add test for alternating between next_u64 and next_u32

31dd85d

[Cherry-picked from 5f4bedf]

Test next_u64 on 32-bit generator to check LE conversion

bf9f905

dhardy force-pushed the prng branch from 42532e5 to bf9f905 Compare December 27, 2017 11:50

Revert some breaking changes

8ad6e2a

Restore default implementations of next_u64 and fill_bytes Restore new_unseeded functions (as wrappers)

Merge branch 'master' into prng

1d3439a

pitdicker mentioned this pull request Jan 2, 2018

To provide (or not) specific PRNGs? dhardy/rand#58

Open

Improve Rng method documentation

99a3b7c

dhardy merged commit 99a3b7c into rust-random:master Jan 2, 2018

dhardy deleted the prng branch January 3, 2018 11:56

dhardy mentioned this pull request Jan 12, 2018

Tracker: planned changes for 0.5 #232

Closed

33 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cherry-pick changes to PRNG algorithms #209

Cherry-pick changes to PRNG algorithms #209

dhardy commented Dec 15, 2017 •

edited

Loading

dhardy commented Dec 15, 2017 •

edited

Loading

pitdicker commented Dec 16, 2017

dhardy commented Dec 16, 2017

dhardy commented Dec 17, 2017

dhardy commented Dec 18, 2017 •

edited

Loading

dhardy commented Dec 18, 2017

dhardy Dec 20, 2017

dhardy Dec 20, 2017

pitdicker Dec 20, 2017

dhardy Dec 20, 2017 •

edited

Loading

dhardy commented Dec 20, 2017 •

edited

Loading

pitdicker commented Dec 20, 2017

dhardy commented Dec 21, 2017

pitdicker commented Dec 21, 2017

dhardy commented Dec 27, 2017

pitdicker commented Jan 2, 2018

Cherry-pick changes to PRNG algorithms #209

Cherry-pick changes to PRNG algorithms #209

Conversation

dhardy commented Dec 15, 2017 • edited Loading

dhardy commented Dec 15, 2017 • edited Loading

pitdicker commented Dec 16, 2017

dhardy commented Dec 16, 2017

dhardy commented Dec 17, 2017

dhardy commented Dec 18, 2017 • edited Loading

dhardy commented Dec 18, 2017

dhardy Dec 20, 2017

Choose a reason for hiding this comment

dhardy Dec 20, 2017

Choose a reason for hiding this comment

pitdicker Dec 20, 2017

Choose a reason for hiding this comment

dhardy Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

dhardy commented Dec 20, 2017 • edited Loading

pitdicker commented Dec 20, 2017

dhardy commented Dec 21, 2017

pitdicker commented Dec 21, 2017

dhardy commented Dec 27, 2017

pitdicker commented Jan 2, 2018

dhardy commented Dec 15, 2017 •

edited

Loading

dhardy commented Dec 15, 2017 •

edited

Loading

dhardy commented Dec 18, 2017 •

edited

Loading

dhardy Dec 20, 2017 •

edited

Loading

dhardy commented Dec 20, 2017 •

edited

Loading