Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RDRAND feature #109

Closed
wants to merge 8 commits into from
Closed

Add RDRAND feature #109

wants to merge 8 commits into from

Conversation

jethrogb
Copy link

@jethrogb jethrogb commented Jul 9, 2016

I expect this to be somewhat controversial.

This PR adds the rdrand feature to this crate. With this feature enabled, OsRng and ThreadRng are replaced by an implementation that just calls the x86 RDRAND instruction. This also makes those interfaces available again in no_std mode (see #108).

The benefits of using RDRAND are two-fold:

  • no external dependencies
  • very fast RNG

Only works on nightly because of the need for inline assembly

impl Rng for OsRng {
fn next_u32(&mut self) -> u32 {
let ret;
unsafe{asm!("rdrand $0":"=r"(ret):::"volatile")};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't check the carry bit to ensure that RDRAND succeeded. RDRAND can and does fail.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, good catch, thanks. I need to update some code elsewhere too... Fixed in 602551c

Jethro Beekman added 4 commits July 10, 2016 11:49
@nagisa
Copy link
Contributor

nagisa commented Jul 10, 2016

First and foremost, the design of the rand crate does not preclude having whatever random generation algorithms outside of rand crate itself. The availability of things like ThreadRng (with a notable exception of OsRng) is a historical mistake that was supposed to be fixed but wasn’t.

Second, this crate must compile with stable rustc, and asm! macro is an unstable feature. You might argue that the rdrand being behind a feature makes it fine, I argue that it is counter-intuitive to have a crate in rust-lang repo which cannot compile with the stable compiler for some particular configuration.

Finally, a shameless plug, the rdrand crate provides such implementation of Rng. While it does not replace OsRng or ThreadRng, I think they aren’t supposed to be replaced in the first place.

@jethrogb
Copy link
Author

First and foremost, the design of the rand crate does not preclude having whatever random generation algorithms outside of rand crate itself. The availability of things like ThreadRng (with a notable exception of OsRng) is a historical mistake that was supposed to be fixed but wasn’t.

Many parts of Rust features a lack of dependency injection though. There are other things that rely on ThreadRng or more specifically thread_rng, without any way for the user to configure the RNG. These things would otherwise work fine in a no_std situation when given a working ThreadRng implementation. This feature is intended to provide a solution for those situations.

You might argue that the rdrand being behind a feature makes it fine

And I do argue that. Another solution that does work on stable is to use the gcc crate with external assembly but that will probably be slower and IMO adds a lot of unnecessary dependencies.

@nagisa
Copy link
Contributor

nagisa commented Jul 11, 2016

Many parts of Rust features a lack of dependency injection though.

You can swap out the whole crates, though (via --extern), so if you really rdrand to be used for your generation purposes, you can just replace rand crate with your own, which uses rdrand

And I do argue that. Another solution that does work on stable is to use the gcc crate with external assembly but that will probably be slower and IMO adds a lot of unnecessary dependencies.

I have measurements:

test bench_u16_external ... bench:         106 ns/iter (+/- 3) = 18 MB/s
test bench_u32_external ... bench:         101 ns/iter (+/- 2) = 39 MB/s
test bench_u64_external ... bench:         101 ns/iter (+/- 3) = 79 MB/s
test bench_u16_inline   ... bench:         102 ns/iter (+/- 2) = 19 MB/s
test bench_u32_inline   ... bench:         104 ns/iter (+/- 3) = 38 MB/s
test bench_u64_inline   ... bench:         105 ns/iter (+/- 3) = 76 MB/s

As you can see overhead of a call-per-number is less than the variance (i.e. is invisible). The only fair point is that you have dependencies on more crates/external tools/etc.

@alexcrichton
Copy link
Contributor

I agree with @nagisa that this is best done as an external crate, and as to whether or not it replaces ThreadRng or not that also seems like a suitable question that should perhaps be deferred to a redesign of the rand crate rather than adding this in-tree for now.

@alexcrichton alexcrichton added the F-new-int Functionality: new, within Rand label Jun 14, 2017
@jethrogb jethrogb mentioned this pull request Aug 6, 2017
@pitdicker
Copy link
Contributor

The rdrand create by @nagisa seems to be a more developed variant to access RDRAND than included here. It also only works on nightly, but in the comment above (#109 (comment)) @nagisa added a solution.

@jethrogb Part of your motivation here is to have thread_rng available in no_std environments. Is RDRAND really a solution? I am still hopelessly ignorant about the core ecosystem, but the idea is that crates that happen to depend on tread_rng mostly run on embedded systems, and not in the situations that have no os, right? Then RDRAND would not be available there.

@jethrogb
Copy link
Author

jethrogb commented Jan 5, 2018

I'm fine if it's not implemented directly but instead depends on @nagisa's crate.

embedded systems, and not in the situations that have no os

I'm not sure what you mean by this. There are many different reasons why you'd be using no_std.

If the architecture you're running on doesn't have RDRAND, this PR is not going to help you, no. If on the other hand you're running no_std code on (recent enough) x86 processors, having thread_rng just work would be very helpful.

@dhardy
Copy link
Member

dhardy commented Jan 5, 2018

I'm not sure what you mean by this. There are many different reasons why you'd be using no_std.

I'm not very knowledgeable about this, and I don't believe pitdicker is either. Would you care to explain? WASM would be another possible no_std target, but I don't believe RDRAND would be available there.

More generally, it would be useful to be able to configure rand to use an entropy source provided by another crate, and have that affect all users of thread_rng etc. I don't see any good ways of doing it though really — maybe allowing an RNG to be set dynamically as a fallback in case OsRng fails.

@pitdicker
Copy link
Contributor

I don't believe pitdicker is either

True 😄, and also not very knowledgeable about many things.

More generally, it would be useful to be able to configure rand to use an entropy source provided by another crate, and have that affect all users of thread_rng etc.

This sounds like the same problem custom allocators have. Would a similar solution help?
If I understand it right, we would need to:

  • call on external function instead of thread_rng
  • make this available under a feature flag
  • link in the other library at compile
  • something like the rdrand crate would have to provide the functions.

Am I missing something big? Would this also help with embedded and WASM?

@pitdicker
Copy link
Contributor

I think we agree that this PR is not the way we want to go, but that there is a real problem with thread_rng() not being available with no_std. I have opened an issue for that in #313.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-new-int Functionality: new, within Rand P-low Priority: Low
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants