feedback

[BurntSushi]

It was easier for me to just write feedback in one issue, but feel free to split this out.

Overall, nice work, but I have some concerns.

Firstly, the use of unsafe in this repository concerns me. I'm pretty sure that almost all of it can be avoided. Many of the uses of unsafe seem to use transmute to convert between integer types and arrays. The standard library has safe primitives for these, but that does unfortunately require a much more recent version of Rust. 1.32 is probably good enough to be honest, but short of that, I think it would be better to just write safe versions in this crate. (i.e., Use bit shifting.)

There is also some incorrect use of unsafe. For example, the FromRawPtr::from_ptr method needs to be marked unsafe, because it's presumably up to the caller to guarantee that the raw pointer given is valid.

You might also consider using NonNull instead of *mut in your ThreadFastRng implementation.

Also, why use local_rng instead of thread_rng? The latter is a reasonable name, and it's what rand uses, so it might be good to stay consistent.

I think there are some documentation issues too. For example, I don't understand what the parameters to FastRng::seed mean.

Is there a name better than FastRng? It might be better to be a bit more specific? Not sure. Or just call it DefaultRng or something. It might be good to note stability characteristics. e.g., Is it permissible for the rng algorithm to change in a non-semver breaking release?

Finally, could you consider combining the crates? I think it would be fine to have one crate with both the traits and a common sense implementation that comes with it. This does make the API evolution of the implementation coupled with the trait, but for the most part, the API of the implementation is the API of the trait.

[elichai]

First of all thank you for your feedback and for looking into these crates :)

1. Yeah, so these safe functions are only from 1.32 and I want to support 1.13+ (like serde does), and in practice these functions also just use transmute, https://doc.rust-lang.org/src/core/num/mod.rs.html#4023-4025 I feel pretty comfortable with transmuting interger primitives, especially that transmute still gives a compile time error for transmuting incorrect sizes. But I still think that unsafe should be accompanied by benchmarks, this might not really matter at all, so i'll invest some time writing the same functions with bit shifting and benchmark both.

2. you're right, i'll fix that, I didn't worry about it too much because this isn't part of the public API and that trait is only to compensate for the missing pub(restricted) in older compilers (which I probably would've used to construct the ThreadFastRng struct)

3. Sadly NonNull is rust 1.25+.

4. I didn't want to use the same names so it won't look like i'm "stealing code" from them (I actually did implemented everything myself without looking at their code, I just know their public API). but this is probably a stupid reason.

5. I'll document the FastRng::seed better. although it's mostly for advanced users.

6. What's wrong with the name? it represents what I want this Rng to be, it might not be the same algorithm and might not be the simplest one, but my API promise that i'll always make it the fastest RNG I know of.

7. Could you expand on why the algorithm change can matter for you? maybe you're looking for a different Rng? (i.e. deterministic Rng).

8. Combining them, hmm. I wanted to give the option for people to implement their own Rng so the way I envisioned it was that I have the trait only crate, and I have the Rng crates, and they re-export the Random crate, so that using the trait crate directly is only needed if you're using your own Rng or you want to be abstract over any kind of Rng.

Does these makes sense to you? if not please tell me, i'm open to changing my mind :)

[BurntSushi]

Thanks for the quick response!

I want to support 1.13+ (like serde does)

I think there is probably very little value in doing this. When serde was first released, it required a very recent version of the Rust compiler at the time. I don't think Serde has any official MSRV policy, but from its track record, it seems like it considers an MSRV bump to be a semver incompatible change. This is a very conservative stance, and I personally don't think it's worth emulating. regex is also very widely used for example, and I bump the MSRV in minor version releases. Very few people have complained. (However, I try to keep the MSRV itself fairly conservative, usually around a year old.)

I feel pretty comfortable with transmuting interger primitives, especially that transmute still gives a compile time error for transmuting incorrect sizes.

Your point about performance is a good one. Basically, I don't think the juice is worth the squeeze here, even if it is slightly faster. (Or, conversely, even if it does require a newer version of Rust.) For example, in your implementation of try_fill_bytes, rand will have different values depending on the target's endianness: https://github.com/elichai/random-rs/blob/bd98b95580422fe6c6dccf2f4fcc7c76e40301a7/random-fast-rng/src/lib.rs#L122

This means, for example---I think---that you'll get different random values for the same seed depending on the target's endianness. That seems bad to me, and the only reason why your code has this bug is because transmute is very difficult to use correctly.

Sadly NonNull is rust 1.25+.

Again, I don't think it's worth supporting ancient compilers for a new project. You might want to be conservative going forward, but for a new project, I don't see much reason to extend support to such ancient versions of Rust. Especially when this is preventing you from removing uses of unsafe, which are pretty compelling reasons to target a newer version of Rust.

I didn't want to use the same names so it won't look like i'm "stealing code" from them (I actually did implemented everything myself without looking at their code, I just know their public API). but this is probably a stupid reason.

rand is permissively licensed, and this project uses the same license. Just be up front about where you got the code and given credit and attribution where it's due. You'll be fine.

I'll document the FastRng::seed better. although it's mostly for advanced users.

Thanks! Also, it or something like it is incredibly useful during testing to make things deterministic, which I personally don't consider particularly advanced. Also, even if it were just for advanced users, I don't think that means it should get less attention paid to its documentation. Even advanced users need to know what the parameters mean.

What's wrong with the name? it represents what I want this Rng to be, it might not be the same algorithm and might not be the simplest one, but my API promise that i'll always make it the fastest RNG I know of.

Yes, that's what I was getting at with respect to whether changing the RNG implementation is legal in a semver compatible release.

The problem with names like "fast" is that they tend to become strained over time. For example, the algorithm you have right now might be "fast," but it might also have some other desirable properties that may be unrelated to it being "fast." Therefore, if you swap it out with something else later that's faster, your users might complain that they've lost the ability to access those other desirable properties, whatever they may be. In that scenario, you're kind of stuck with naming. Where as if you just name it Default or something more specific, you have a bit more flexibility.

This is strictly an abstract argument. I don't know whether said desirable properties actually exist or not.

Could you expand on why the algorithm change can matter for you? maybe you're looking for a different Rng? (i.e. deterministic Rng).

I don't think I personally care, but it sounds like something worth knowing. Even something as simple as, "the specific RNG algorithm used here may change in a patch release" is good enough.

Combining them, hmm. I wanted to give the option for people to implement their own Rng so the way I envisioned it was that I have the trait only crate, and I have the Rng crates, and they re-export the Random crate, so that using the trait crate directly is only needed if you're using your own Rng or you want to be abstract over any kind of Rng.

I don't see why combining the crates means you can't let others implement their own RNG. The combined crate would still have traits.

[elichai]

Thanks. I think FastRng and DeterministicRng should be 2 separate ideas. especially for stability, I would like to be able to change the algorithm of FastRng pretty frequently (whenever I read a paper proposing something faster, or if there's some new instruction that can do things faster). and a DeterministicRng should be really conservative about changing algorithm, even in major bumps.

About 1.25, I am a regular contributor to the rust-bitcoin ecosystem and might want to integrate these crates there, and rust-bitcoin requires 1.22+.

again about endianess, FastRng doesn't promise to be deterministic in any way, it might be somewhat deterministic now (same seed on the same endianess) but in the future this might change, an Rng that promises to be deterministic deserves it's own struct.

about the crates and people implementing their own, is that the amounts of Rng grows pretty fast. today it's just one, but then i'll want deterministic, OS based, cryptographically secure deterministic, DRAND based, and who knows what else, so i'd rather keep every Rng to his own crate while re-exporting the trait.

Unless you can point me to a good reason why this makes the usage not nice.

[BurntSushi]

It sounds like we probably have different ideas about what this crate should look like unfortunately. Not sure I have the time to really dig into this debate, but we're definitely not on the same page. Which is totally fine. It's good to have disagreements like this sometimes!

[elichai]

Disagreements are the only way we can learn new things :) I really hope this crate can be useful for what it is made for, so any reasons for why it is not useful are usually more helpful then "Nice work" :)

(i.e. the rand crate is pretty darn big and provides a lot of different Rng sources)

Thanks for your time and feedback.

[elichai]

I started experimenting with the idea, will wait and hear other peoples feedback (on IRC and reddit) and next week i'll make a decision on this. Meanwhile i'll start implementing more random number generators.

https://github.com/elichai/random-rs/tree/single-crate