Add fill and try_fill methods to Rng #247

dhardy · 2018-01-29T12:49:44Z

This was suggested a couple of times. It's perhaps more useful with #244 since then fill_bytes and try_fill_bytes are not available in SampleRng.

I'm not entirely happy with the code organisation; this adds a generic-sounding trait name, but perhaps it should be renamed AsByteSliceMut and not have fn to_le to meet what would be expected from the name. From our point of view those are needless complications however.

This also adds more unsafe code but I don't think that's avoidable.

pitdicker · 2018-01-29T18:39:42Z

I have been working on something similar, but with a different approach. pitdicker@bd44cd2 is the latest try. Every type there has a default implementation that uses the same method as gen, and integer slices have a custom implementation that uses fill_bytes directly. Worth comparing?

dhardy · 2018-01-29T18:59:41Z

Interesting; you push more of the implementation down into the trait. Your version also supports any type supporting the Default distribution, at the cost of requiring per-element copy, then specialises for integer slices; however doesn't support error-forwarding via try_fill_bytes (which would require a separate trait since this is part of the trait implementation).

I'm not sure which I prefer; I'm also not sure how useful the error forwarding would be in practice (with many generators being infallible, and for the rest a single "try generating a random word" check after initialisation would suffice to catch most errors).

pitdicker · 2018-01-29T20:24:20Z

Good summary!

For some things like distributions returning Results just doesn't make sense. I wish we had some solution for the few cases where distributions are combined with a fallible RNG. Just an idea, but what do you think about this?

To add a wrapper like FallibleRng that reimplements next_u32, next_u64 and fill_bytes by wrapping try_fill bytes. If there is some error, store the error in an extra field. And to add a functions like is_ok() and get_result().

Sorry for bringing up a different issue though...

dhardy · 2018-01-30T09:19:17Z

No, I don't think much of that. If the wrapper is implementing the same next_u32 etc. then it can only handle errors from its source by (a) panicking (roughly the same as now) or (b) by returning some non-random data and expecting a user to call is_ok() afterwards. Also you miss the implicit error handling we have now in OsRng::fill_bytes.

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

Are you ready to make a choice between the two options? I like mine more 😄. But my variant does nothing that can not be achieved in some other way just as easy and fast. While yours can also pass on errors, so that seems like the better choice.

dhardy · 2018-02-01T14:42:34Z

I added a second commit inspired by @pitdicker's version.

There is however a significant difference: @pitdicker's code also supports any type supported by the default distribution, e.g. arrays, slices and floating-point numbers, so for example it could fill an array with FP numbers in the range [0, 1). My code in this PR only supports byte-copy to slices of integers (it can be extended to arrays and tuples similar to Rand).

So, what should this do?

let mut array = [0f32; 100];
rng.fill(&mut array);  // or &mut array[..]

Create an array of 100 × f32 each in range [0, 1)
Fill with random bytes, allowing all f32 values including NaN
Not be supported

Pretty obviously, option (2) has very little use (and would not be safe with all types). Option (1) is well-defined but appears counter-intuitive (or maybe that's just me) — it requires accessing elements individually and using a distribution, instead of just a block-copy. I prefer (3) but others may prefer (1)?

pitdicker · 2018-02-01T15:42:06Z

src/lib.rs

+/// Trait for casting types to byte slices
+pub trait AsByteSlice {
+    /// Return a mutable reference to self as a byte slice
+    fn as_byte_slice<'a>(&'a mut self) -> &'a mut [u8];


I don't know for sure (still a novice and no computer right now), but is it possible to end up with two mutable references to the same memory after this function?

The reference passed in will be locked until after the result is destroyed thanks to Rust's lifetime analysis. It's actually hard to get this wrong in Rust.

pitdicker · 2018-02-01T15:44:39Z

src/lib.rs

+    /// Return a mutable reference to self as a byte slice
+    fn as_byte_slice<'a>(&'a mut self) -> &'a mut [u8];
+    /// Perform byte-swapping on BE platforms
+    fn to_le(&mut self);


This does not really need to be part of the trait. Wouldn't it be easier to just write the conversion loop in fill and try_fill?

Okay, then we can also standardise the naming.

It will require to use more complex trait bounds, not only you'll need trait for iteration, but also for to_le method as well.

pitdicker · 2018-02-01T16:14:35Z

Looks good to me!

dhardy · 2018-02-06T15:17:33Z

@burdges @newpavlov you commented on this topic before; any thoughts on this PR or @pitdicker's version? I'm not sure whether to add this, some other variant of fill, or nothing like this.

newpavlov

I don't mind this addition, but I think we better to list primitive types (i.e. u16, u32, etc.) in the methods documentation and add to AsByteSliceMut note that its main usage is for fill and try_fill methods.

newpavlov · 2018-02-06T15:25:55Z

src/lib.rs

+        let slice = dest.as_byte_slice_mut();
+        self.fill_bytes(slice);
+        for mut x in slice {
+            x.to_le();


If I am reading this correctly this line does not do anything, because to_le for u8 should be no-op. We'll need to add to_le method to AsByteSliceMut trait.

You're right. Actually, that's what I had originally.

Good catch, but shouldn't it just loop over dest?

I don't see a trait supporting to_le though so it has to be done per implementation.

Then you'll need to add another bound to allow you iterate mutably over dest. I think it's easier to add a method to AsByteSliceMut.

I wonder if eventually we'll get a suitable trait; for now it doesn't look like it (also nothing in the num crate I can see).

newpavlov · 2018-02-06T15:59:59Z

src/lib.rs

+            fn to_le(&mut self) {
+                for mut x in self {
+                    x.to_le();
+                }


I think this will not work. You need to write:

for x in self { *x = x.to_le(); }

Well spotted. There's a unit test already but I we need to run CI on a BE platform...

dhardy · 2018-02-07T12:24:24Z

@newpavlov I added some documentation.

Because this works by filling dest with random bytes, I wonder if fill_bytes is more appropriate — we could replace the current fill_bytes / try_fill_bytes with these functions instead. That should be fully compatible with users of fill_bytes but not existing Rng implementations (though those will require updating anyway with the next rand release). It requires that each implementation calls dest.as_byte_slice_mut() and (if appropriate) dest.to_le(), i.e. a tiny increase in implementation code (with more flexibility for Endianness handling).

An advantage of doing it that way would be that we could still add a more generic fn fill similar to @pitdicker's version later if we want. Thoughts?

vks · 2018-02-16T10:33:42Z

An advantage of doing it that way would be that we could still add a more generic fn fill similar to @pitdicker's version later if we want. Thoughts?

Yes, I think this reasonable. Also, fill for floats can be implemented more efficiently, because less than 32/64 bits are needed per float (see the dSFMT random number generator).

dhardy · 2018-02-16T10:45:41Z

Regarding floats, see this discussion. I'd prefer to leave them out of this discussion since there are far too many possible trade-offs.

vks · 2018-02-16T11:14:32Z

I agree, which is why I think it is a good idea to only have fill_bytes without support for floats.

dhardy · 2018-02-19T15:19:40Z

My previous idea of replacing fill_bytes and try_fill_bytes with these functions doesn't work, and the new extension trait split Rng: RngCore emphasises why: the base functions used in RngCore must not be generic so that RngCore can be object-safe (and without removing any functionality).

We could still use those names for the new functions and rename or shadow the old functions, but I don't think either is a good idea, so fill and try_fill will have to do (unless we prefer fill_slice etc.).

dhardy · 2018-02-19T15:31:46Z

Rebased on top of master (with new RngCore).

vks · 2018-02-19T15:59:47Z

Looks good to me! I would consider explicitly mentioning that the bytes are uniformly distributed.

dhardy · 2018-02-19T16:59:38Z

I have a commit adding support for arrays (up to length 32) using recursive expansion like Rand, but I noticed it doubles the compile time — from:

real    0m2.180s
user    0m2.666s
sys     0m0.116s

to:

real    0m3.654s
user    0m4.637s
sys     0m0.144s

but with some re-thinking, I got this down to:

real    0m2.278s
user    0m2.791s
sys     0m0.100s

Once support for generic constants lands we should be able to support arrays properly (without all the macro recursion nonsense).

Usage of recursive macros appears to have some compile time hit, but with a single macro and generic impl it's not too much (directly recursing for each type is far worse).

dhardy · 2018-02-20T11:09:59Z

Rebase after merge of #256.

Add fill and try_fill methods to Rng

pitdicker reviewed Feb 1, 2018

View reviewed changes

dhardy force-pushed the fill_rng branch from fc14449 to 93a80bd Compare February 4, 2018 12:02

newpavlov reviewed Feb 6, 2018

View reviewed changes

dhardy force-pushed the fill_rng branch 2 times, most recently from 80696ec to 746a7aa Compare February 7, 2018 12:19

dhardy force-pushed the fill_rng branch from 746a7aa to 48f076c Compare February 7, 2018 15:19

dhardy added the E-question Participation: opinions wanted label Feb 7, 2018

dhardy force-pushed the fill_rng branch from 48f076c to e9d0b06 Compare February 19, 2018 15:32

dhardy added 2 commits February 20, 2018 11:08

Add fill and try_fill methods to Rng

cbd2cc7

AsMutByteSlice: support arrays up to length 32

5da66c3

Usage of recursive macros appears to have some compile time hit, but with a single macro and generic impl it's not too much (directly recursing for each type is far worse).

dhardy force-pushed the fill_rng branch from 2b36486 to 5da66c3 Compare February 20, 2018 11:09

dhardy merged commit d9bde36 into rust-random:master Feb 21, 2018

dhardy deleted the fill_rng branch March 14, 2018 16:39

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

Merge pull request #247 from dhardy/fill_rng

9b04136

Add fill and try_fill methods to Rng

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fill and try_fill methods to Rng #247

Add fill and try_fill methods to Rng #247

dhardy commented Jan 29, 2018

pitdicker commented Jan 29, 2018

dhardy commented Jan 29, 2018

pitdicker commented Jan 29, 2018 •

edited by dhardy

Loading

dhardy commented Jan 30, 2018

pitdicker commented Feb 1, 2018

dhardy commented Feb 1, 2018

pitdicker Feb 1, 2018

dhardy Feb 1, 2018

pitdicker Feb 1, 2018

dhardy Feb 1, 2018

newpavlov Feb 6, 2018

pitdicker commented Feb 1, 2018

dhardy commented Feb 6, 2018

newpavlov left a comment

newpavlov Feb 6, 2018

dhardy Feb 6, 2018

pitdicker Feb 6, 2018

dhardy Feb 6, 2018

newpavlov Feb 6, 2018

dhardy Feb 6, 2018

newpavlov Feb 6, 2018 •

edited

Loading

dhardy Feb 6, 2018

dhardy commented Feb 7, 2018

vks commented Feb 16, 2018

dhardy commented Feb 16, 2018

vks commented Feb 16, 2018

dhardy commented Feb 19, 2018

dhardy commented Feb 19, 2018

vks commented Feb 19, 2018

dhardy commented Feb 19, 2018

dhardy commented Feb 20, 2018

Add fill and try_fill methods to Rng #247

Add fill and try_fill methods to Rng #247

Conversation

dhardy commented Jan 29, 2018

pitdicker commented Jan 29, 2018

dhardy commented Jan 29, 2018

pitdicker commented Jan 29, 2018 • edited by dhardy Loading

dhardy commented Jan 30, 2018

pitdicker commented Feb 1, 2018

dhardy commented Feb 1, 2018

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 1, 2018

dhardy commented Feb 6, 2018

newpavlov 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

newpavlov Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy commented Feb 7, 2018

vks commented Feb 16, 2018

dhardy commented Feb 16, 2018

vks commented Feb 16, 2018

dhardy commented Feb 19, 2018

dhardy commented Feb 19, 2018

vks commented Feb 19, 2018

dhardy commented Feb 19, 2018

dhardy commented Feb 20, 2018

pitdicker commented Jan 29, 2018 •

edited by dhardy

Loading

newpavlov Feb 6, 2018 •

edited

Loading