Add `new_from_u64` to `IsaacRng` and `Isaac64Rng` #5

pitdicker · 2017-09-23T19:47:52Z

This PR includes some clean-up around the init code of ISAAC, support for initializing from an u64, benchmarks for RNG initialization, and some cleanup of the tests.

I have implemented new_from_u64 for IsaacRng and Isaac64Rng as a function instead of a trait for now, as that makes it easy to add it to every RNG one at a time.

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?

pitdicker · 2017-10-20T06:32:57Z

Rebased.

I also removed the Copy trait. This turned out to a bit difficult, because initialisation relied on copying an empty static struct. Also Clone relied on Copy. Initialisation was a bit strange anyway. It followed the same strategy as the C implementation, that expects the initialisation key to be results array of an empty struct. I now made init take the key as an argument, and have it create the empty struct itself.

This does move the code around a lot. This makes reviewing more difficult, my apologies. If it helps, it still passes all test and matches the reference implementation (tested).

Once new_from_u64 is part of SeedableRng everything that has to do with initialisation should be in one part of the file.

pitdicker · 2017-10-20T06:36:21Z

One thing I am thinking about is the advise of Bob Jenkins to only use a seed that is half the size of the state. It should be more than enough, but he did not make the change because the reference implementation was already published.

We can leave from_seed as it is, so it can match the reference implementation. But for from_rng it should not be observable whether we used a seed of only half the state. It should be a free performance win.

dhardy

Great work! Sorry to be so picky, I know some of what I've commented on isn't your code anyway.

Main things I'm concerned about are the obvious mistake in the comments and potential adjustments to new_from_u64. I'm not certain my points are correct, however.

Preferably add new commits rather than rebase now, as this is already complex.

dhardy · 2017-10-20T09:18:26Z

src/prng/isaac.rs

-                g ^= h << 8;  b += g; h += a;
-                h ^= a >> 9;  c += h; a += b;
-            }}
+// Cannot be derived because [u32; 256] does not implement Clone


I think this should be fixed once RFC 2000 gets implemented, but for now this makes sense

I will add a note

dhardy · 2017-10-20T09:24:59Z

src/prng/isaac.rs

+        };
+
+        // Prepare the first set of results
+        rng.isaac();


cnt == 0 so isaac() should be run anyway when next_u32() is called. Or is this done to try to match results when seed == 0 or to improve initialisation quality? That's not what the comment says though.

Yes true. There is no reason to do this step except that it mirrors the reference implementation. It does not matter much whether we pay the cost of the first round during initialization or on the first next_u32.

It's not about where the cost is paid, it's that this gets run twice. If that's by design, that's fine, but the comment is wrong.

O, that would not be by design. After the first round cnt == RAND_SIZE. Where would it run for the second time?

Note there is one version on the authors site that calls isaac() twice, the one used for the challenge. But the normal version only calls it once.

Ok, you're right — isaac() modifies cnt of course.

dhardy · 2017-10-20T09:27:54Z

src/prng/isaac.rs

    fn try_fill(&mut self, dest: &mut [u8]) -> Result<()> {
        ::rand_core::impls::try_fill_via_u32(self, dest)
    }
 }

+/// Creates a new ISAAC-64 random number generator.


This isn't the 64-bit variant, right? Did you copy + paste?

Yes, a copy + paste error

dhardy · 2017-10-20T09:30:32Z

src/prng/isaac.rs

+///
+/// And his answer to the question "For my code, would repeating the key over
+/// and over to fill 256 integers be a better solution than zero-filling, or
+/// would they essentially be the same?":


Does this have implications for seed_from_u64 — i.e. adjust all the state instead of just the first bit? (I have no idea myself; maybe it's not significant.)

I don't know all the reasoning behind the initialization code. But with the authors comment

If the seed is under 32 bytes, they're essentially the same, otherwise repeating the seed would be stronger

I think seed_from_u64 is fine.

Darn, bits != bytes! Sorry.

dhardy · 2017-10-20T09:33:47Z

src/prng/isaac.rs

+
+    let mut mem = [w(0); RAND_SIZE];
+
+    macro_rules! memloop {


Is there a real reason for this obfuscated code (generic wrt source array) — shouldn't it be the same to just use key directly (i.e. change fn to init(mut mem: [w32; RAND_SIZE]))?

With the old code it was not possible, but maybe now with a separate key it is. I am going for the simple solution: just try it, and if the result is the same, I will remove the macro. That would be a good improvement!

dhardy · 2017-10-20T09:34:57Z

src/prng/isaac.rs

+    /// Creates an ISAAC random number generator using an u64 as seed.
+    /// If `seed == 0` this will produce the same stream of random numbers as
+    /// the reference implementation when used unseeded.
+    pub fn new_from_u64(seed: u64) -> IsaacRng {


I get why you don't want to share code between init and new_from_u64 but it might be worth looking into at some point — well, maybe better to leave it until another PR.

It is not that I don't want to share code. Every time you duplicate something there is an extra place to make mistakes. But it took a long read for me to understand how a seed ran trough the old code. The code path is different in 3 separate places, and I didn't find a nice readable way to combine them. I will think about it some more.

It worked :-). Much better now.

pitdicker · 2017-10-20T11:52:16Z

Sorry to be so picky

Thanks for looking so closely!

Preferably add new commits rather than rebase now, as this is already complex.

Completely understand. Otherwise you almost have to start again.

dhardy

Definitely like the new init code

dhardy · 2017-10-20T13:25:41Z

src/prng/isaac.rs

+        };
+
+        // Prepare the first set of results
+        rng.isaac();


Ok, you're right — isaac() modifies cnt of course.

dhardy · 2017-10-21T09:35:58Z

src/prng/isaac.rs

-        }
+        let mut key = [w(0); RAND_SIZE];
+        key[0] = w(seed as u32);
+        key[1] += w((seed >> 32) as u32);


= then +=

Doesn't matter I guess but preferably be consistent

I should be more careful, this are sloppy mistakes...

dhardy · 2017-10-21T09:44:09Z

src/prng/isaac.rs

+        let mut key = [w(0); RAND_SIZE];
+        key[0] = w(seed as u32);
+        key[1] += w((seed >> 32) as u32);
+        // Initialize with only one pass.
        // A second pass does not improve the quality here, because all of
        // the seed was already available in the first round.


I don't really get this — I mean, all of the seed gets used in the first round when using big keys too, right?

In reality I suppose it shouldn't matter since a 64-bit seed is not really sufficient for cryptography and this method should still be sufficient for non-crypto uses I guess (unless there's any detectable bias in initial output). Speaking very much as a non-expert here...

For a short overview: in blocks of 8 words the seed is mixed with the temporary array a...h. Each block of 8 words then becomes part of the new state. After one full pass every word of the new state is effected by the first block. But only the last block of 8 words is effected by the last block of the seed.

After the first pass a...h has all of the seed mixed into it. Doing another pass will now mix bits from the middle and last blocks of the seed into the entire state.

The 64-bit seed is part of the first block, and therefore available to a...h from the first round. At the end of the first full pass the 'random quality' of a...h is not any higher than at the start. A second pass does not distribute the randomness from the seed more evenly than the first.

Ah, I get you. But the 32-bit generator has two blocks set; b will not affect a until the second round if I understand correctly. I don't see why it matters though (provided a is initially set to a good seed, which it should be, since it gets added to the golden ratio).

pitdicker · 2017-10-21T14:15:13Z

I have tried a merge commit, hope I did it right...

pitdicker added 2 commits October 19, 2017 21:09

Add initialization benchmarks

a6528a3

Clean up ISAAC tests

08ac750

pitdicker force-pushed the isaac_init branch from 89fb24b to 6a34697 Compare October 20, 2017 06:23

pitdicker force-pushed the isaac_init branch from 6a34697 to adcd8e5 Compare October 20, 2017 06:25

dhardy requested changes Oct 20, 2017

View reviewed changes

pitdicker force-pushed the isaac_init branch 2 times, most recently from 21cec59 to b31a663 Compare October 20, 2017 16:28

dhardy reviewed Oct 21, 2017

View reviewed changes

Simplify isaac init code

130b64c

pitdicker force-pushed the isaac_init branch from b31a663 to 130b64c Compare October 21, 2017 13:55

Merge branch 'master' into isaac_init

58350a9

pitdicker force-pushed the isaac_init branch from a12ef45 to 58350a9 Compare October 21, 2017 14:14

dhardy merged commit bf53631 into dhardy:master Oct 21, 2017

pitdicker deleted the isaac_init branch October 21, 2017 14:39

dhardy mentioned this pull request Oct 22, 2017

Seeding PRNGs (traits and bit size) #18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `new_from_u64` to `IsaacRng` and `Isaac64Rng` #5

Add `new_from_u64` to `IsaacRng` and `Isaac64Rng` #5

pitdicker commented Sep 23, 2017

pitdicker commented Oct 20, 2017

pitdicker commented Oct 20, 2017

dhardy left a comment

dhardy Oct 20, 2017

pitdicker Oct 20, 2017

dhardy Oct 20, 2017

pitdicker Oct 20, 2017

dhardy Oct 20, 2017

pitdicker Oct 20, 2017

pitdicker Oct 20, 2017

dhardy Oct 20, 2017

dhardy Oct 20, 2017

pitdicker Oct 20, 2017

dhardy Oct 20, 2017

pitdicker Oct 20, 2017

dhardy Oct 20, 2017

dhardy Oct 20, 2017

pitdicker Oct 20, 2017

dhardy Oct 20, 2017

pitdicker Oct 20, 2017

pitdicker Oct 20, 2017

pitdicker commented Oct 20, 2017

dhardy left a comment

dhardy Oct 20, 2017

dhardy Oct 21, 2017

pitdicker Oct 21, 2017

dhardy Oct 21, 2017

pitdicker Oct 21, 2017

dhardy Oct 21, 2017

pitdicker commented Oct 21, 2017

Add new_from_u64 to IsaacRng and Isaac64Rng #5

Add new_from_u64 to IsaacRng and Isaac64Rng #5

Conversation

pitdicker commented Sep 23, 2017

pitdicker commented Oct 20, 2017

pitdicker commented Oct 20, 2017

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker commented Oct 20, 2017

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker commented Oct 21, 2017

Add `new_from_u64` to `IsaacRng` and `Isaac64Rng` #5

Add `new_from_u64` to `IsaacRng` and `Isaac64Rng` #5