Reduce JitterRng size #251

pitdicker · 2018-02-01T20:54:02Z

This PR contains various changes to JitterRng to get it's size down from 2112 bytes to 12 bytes (rounded up to 16).

The largest change is splitting the state in a part that is preserved across runs, and a part that is reinitialized with every call to gen_entropy. The memory for the memaccess noise source does not need to be preserved. Also prev_time was already initialized again for every round of gen_entropy. And the two delta's for the stuck test are questionable to preserve across runs.

Various fields could be reduced in size, index is less than 64*32=2048 so it fits in an u16. rounds as at most 128, JitterRng does work if the entropy we can collect per round is less than half a bit. And we can expect the delta's to be of sub-second size, so they always fit in an i32.

If JitterRng was used with next_u32, we would preserve the unused 32 bits in an Option. This duplicates the bits in data. Now I just used a bool to indicate half of it is still available. This makes the case where someone alternately uses next_u32 and next_u64 with JitterRng slightly less efficient, but I don't imagine that is a realistic scenario.

Finally I have replaced the pointer to a timer function with a zero-sized type with a trait that implements the timer. It feels a little bit more like the Rust way of doing things, but it doesn't look great to me, because JitterRng gets an extra parameter JitterTimer.

Back when JitterRng was first merged there were some problems with how I tried to avoid rounding errors in the estimation of the number of rounds in test_timer. An alternative I did not use at the time (going for simplicity) was to use a lookup table for small values. The last commit implements that.

pitdicker · 2018-02-02T07:35:54Z

I have to think a little more about the stuck test (which is also the cause of the CI failure).

Edit: I think it is enough to just set prev_time to the current time while initializing EcState here and here.

pitdicker · 2018-02-02T20:16:46Z

I updated the first commit and the last two to make the CI pass (glad there exists something like CI to catch my mistakes)

dhardy

I'm uneasy about this. For a start, it's supposed to be a crypto-strength entropy generator, but we're straying further from the reference implementation, so I'm less happy implementing CryptoRng — but this is required for e.g. ChaChaRng::new() to be secure.

I think I'd prefer to just put everything inside a static mutex, similar to os::ReadRng. This might require making JitterRng a shim around inner data and make it difficult for JitterRng::new and JitterRng::new_with_timer to return the same type. Let me know if you want me to try this, but I'd rather hear your opinion before trying anything.

dhardy · 2018-02-03T13:05:08Z

src/jitter.rs

+    // Memory for the Memory Access noise source FIXME
+    mem_prev_index: u16,
+    // Make `next_u32` not waste 32 bits
+    data_half_used: bool,


I'm not sure whether Rustc does in fact re-order fields for better packing yet. If that's the reason for using small integers, it's probably worth re-ordering them here?

I believe it reorders for a couple of versions now, but I measured that the size actually decreased with size_of::JitterRng<Timer>.

dhardy · 2018-02-03T13:06:23Z

src/jitter.rs

-    last_delta: i64,
-    last_delta2: i64,
+    last_delta: i32,
+    last_delta2: i32,


Why make these smaller if they're not being saved anyway?

Good point. I made this change while exploring various idea's, and saw no harm in keeping them as i32.

dhardy · 2018-02-03T14:35:18Z

src/jitter.rs

+        state.set_rounds(rounds);
+
+        // Fill `data` with a non-zero value.
+        state.gen_entropy();


Why bother? Doesn't look like it gets used.

It gets used, see #251 (comment)

dhardy · 2018-02-03T14:36:36Z

src/jitter.rs

@@ -48,25 +48,53 @@ const MEMORY_SIZE: usize = MEMORY_BLOCKS * MEMORY_BLOCKSIZE;
 // Note: the C implementation relies on being compiled without optimizations.
 // This implementation goes through lengths to make the compiler not optimise
 // out what is technically dead code, but that does influence timing jitter.
-pub struct JitterRng {
+pub struct JitterRng<T: JitterTimer> {
    data: u64, // Actual random number


As I understand it, outside of gen_entropy calls, this is either unused or the more significant half is used. So would it be better to move data to EcState and store an Option<u32> here?

No, this is what is called the 'entropy pool' in previous versions of jitter_entropy. I wondered if it was necessary to preserve across runs. Suppose we initialize it with some constant with each round of gen_entropy. In theory, if the timer does not collect the expected number of bits of entropy per round once in a while, the result will be a little closer to that constant. Preserving data across runs will instead make it just a little closer to the previously generated value.

dhardy · 2018-02-03T14:40:10Z

src/jitter.rs

+    // Timer used by `measure_jitter`
+    timer: T,
+    // Memory for the Memory Access noise source FIXME
+    mem_prev_index: u16,


So, is it necessary to use a different index every time when the whole block gets reallocated? I'm not really sure about the memory access bit — e.g. if the block is freshly allocated each time, what's to stop the compiler just working out the result in advance?

Using a different index each time will keep the code working as before, I did not want to change the algorithm. Preserving the index across runs is the reason the compiler can not work out the result. But I should have updated the comment (I use a lot of FIXMEs while making changes 😄)

dhardy · 2018-02-03T14:42:07Z

src/jitter.rs

-            prev_time: 0,
-            last_delta: 0,
-            last_delta2: 0,
+            timer: timer.clone(),


shouldn't be necessary to clone

👍, leftover from a previous version.

dhardy · 2018-02-03T14:46:19Z

src/jitter.rs

+            prev_time: self.timer.get_nstime(),
+            last_delta: 0,
+            last_delta2: 0,
+            mem: [0; MEMORY_SIZE],


If this is statically initialised to zero each time, what's to stop the compiler optimising the memory access out? Even if it doesn't, this will be in the lowest-level cache it fits in already.

If this is statically initialised to zero each time, what's to stop the compiler optimising the memory access out?

The black_box(ec.mem[0]); ~~sprinkled around~~ placed strategically.

Even if it doesn't, this will be in the lowest-level cache it fits in already.

Good point, I will do some measurements.

I collected measurements with timer_stats for 100_000 rounds, with the CPU timer source in timer_stats disabled. So I only measured the mem_access noise source.

The result of the NIST Entropy Estimation test on the current version is: 0.449745 (on my PC).
And with new version it is: 0.465139. So no difference.

dhardy · 2018-02-03T14:47:11Z

src/jitter.rs

+        let mut ec = EcState {
+            prev_time: self.timer.get_nstime(),
+            last_delta: 0,
+            last_delta2: 0,


Not really sure about these deltas — you're sure they don't need to be preserved?

They are only necessary for the stuck test. Now we set prev_time when initializing EcState, and set it again together with last_delta in the first dummy round of measure_jitter. That dummy round was already part of the algorithm.

We are only missing the previous value of last_delta2, making the stuck test on the first real round of measure_jitter slightly less reliable. But using a last_delta2 that could have been set seconds or minutes earlier was not all that reliable already anyway.

So I see no loss here, but it takes some thinking through...

pitdicker · 2018-02-03T15:33:22Z

I'm uneasy about this. For a start, it's supposed to be a crypto-strength entropy generator, but we're straying further from the reference implementation.

Yes, good to be careful! What has changed is that the last_delta2 variable is now initialized to 0 in gen_entropy, while it had some (possibly not completely meaning) value before. And that only influences the 'quality measuring' stuck_test a little, for the first round of measure_jitter. Seems not a great loss to me.

The second change is the location of the memory area. In jitter_entropy it was on the heap. We had it as part of the type, and now it becomes an array on the stack. I will take some measurements here to test if it changes anything.

I think I'd prefer to just put everything inside a static mutex

Let me know if you want me to try this

This was one of the variants I made, but though this was the nicer option.

pitdicker · 2018-02-03T15:33:41Z

Thanks of the detailed review!

dhardy · 2018-02-04T12:04:55Z

Well, I guess your replies are enough to convince me this is probably the way to go. I'll leave this open to further comments for a few days before merging.

dhardy · 2018-02-07T15:03:40Z

src/jitter.rs

@@ -48,12 +48,12 @@ const MEMORY_SIZE: usize = MEMORY_BLOCKS * MEMORY_BLOCKSIZE;
 // Note: the C implementation relies on being compiled without optimizations.
 // This implementation goes through lengths to make the compiler not optimise
 // out what is technically dead code, but that does influence timing jitter.
-pub struct JitterRng {
+pub struct JitterRng<T: JitterTimer> {


Turns out, this is a problem: JitterRng is no longer a concrete type, so we can't use it in the EntropyRng enum. I'm going to revert this commit: 3aa580a

Reduce JitterRng size

pitdicker changed the title ~~Reduce JitterRng reduce size~~ Reduce JitterRng size Feb 1, 2018

pitdicker force-pushed the jitter_reduce_mem branch from 368648a to 3964474 Compare February 2, 2018 20:04

pitdicker added 7 commits February 2, 2018 21:11

Replace timer function with trait

3aa580a

Replace Option with bool

16a9993

Make index u16

4603c09

Make rounds u8

d8394a1

Reduce deltas to i32

40241ab

Split JitterRng state in a permanent and temporary state

0d052bd

Use a lookup-table when estimating the number of JitterRng rounds

36093a7

pitdicker force-pushed the jitter_reduce_mem branch from 3964474 to 36093a7 Compare February 2, 2018 20:12

dhardy reviewed Feb 3, 2018

View reviewed changes

dhardy mentioned this pull request Feb 5, 2018

Add EntropySource wrapper #235

Merged

dhardy merged commit f6773b9 into rust-random:master Feb 7, 2018

dhardy reviewed Feb 7, 2018

View reviewed changes

pitdicker deleted the jitter_reduce_mem branch March 1, 2018 19:36

pitdicker pushed a commit that referenced this pull request Apr 4, 2018

Merge pull request #251 from pitdicker/jitter_reduce_mem

45ec7f4

Reduce JitterRng size

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce JitterRng size #251

Reduce JitterRng size #251

pitdicker commented Feb 1, 2018

pitdicker commented Feb 2, 2018 •

edited

Loading

pitdicker commented Feb 2, 2018

dhardy left a comment

dhardy Feb 3, 2018

pitdicker Feb 3, 2018

dhardy Feb 3, 2018

pitdicker Feb 3, 2018

dhardy Feb 3, 2018

pitdicker Feb 3, 2018

dhardy Feb 3, 2018

pitdicker Feb 3, 2018

dhardy Feb 3, 2018

pitdicker Feb 3, 2018

dhardy Feb 3, 2018

pitdicker Feb 3, 2018

dhardy Feb 3, 2018

pitdicker Feb 3, 2018

pitdicker Feb 3, 2018

dhardy Feb 3, 2018

pitdicker Feb 3, 2018

pitdicker commented Feb 3, 2018

pitdicker commented Feb 3, 2018

dhardy commented Feb 4, 2018

dhardy Feb 7, 2018

Reduce JitterRng size #251

Reduce JitterRng size #251

Conversation

pitdicker commented Feb 1, 2018

pitdicker commented Feb 2, 2018 • edited Loading

pitdicker commented Feb 2, 2018

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

pitdicker commented Feb 3, 2018

pitdicker commented Feb 3, 2018

dhardy commented Feb 4, 2018

Choose a reason for hiding this comment

pitdicker commented Feb 2, 2018 •

edited

Loading