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

libcore: rand: Use a pure Rust implementation of ISAAC RNG #6073

Closed
wants to merge 2 commits into from

Conversation

huonw
Copy link
Member

@huonw huonw commented Apr 26, 2013

This replaces the wrapper around the runtime RNG with a pure Rust implementation of the same algorithm. This is much faster (up to 5x), and is hopefully safer.

There is still (a little) room for optimisation: testing by summing 100,000,000 random u32s indicates this is about 40-50% 10% slower than the pure C implementation (running as standalone executable, not in the runtime).

(Only 6d50d55 is part of this PR, the first two are from #6058, but are required for the rt rng to be correct to compare against in the tests.)

@ghost ghost assigned graydon Apr 26, 2013
@huonw
Copy link
Member Author

huonw commented Apr 26, 2013

I can't test to see if this fixes #6061 since I can't reproduce it on any of the machines I have access to, it would be helpful for someone who sees that bug to test it.

@thestinger
Copy link
Contributor

I mentioned on IRC that I'm curious how it compares to the C implementation when there aren't stack switches (#[fast_ffi] on the extern fns).

@huonw
Copy link
Member Author

huonw commented Apr 26, 2013

As a permanent record of the answer to @thestinger's curiousity:

Current implementation (perf results)

real    0m3.279s
user    0m3.256s
sys     0m0.008s

With fast_ffi, default stack size (perf results)

real    0m4.188s
user    0m4.172s
sys     0m0.004s

With fast_ffi, RUST_STACK_MIN=50000000 (perf results)

real    0m1.545s
user    0m1.520s
sys     0m0.012s

This patch (perf results)

real    0m0.778s
user    0m0.768s
sys     0m0.008s

@pcwalton
Copy link
Contributor

Oops, sorry, reviewed this. Wondering why this isn't ready for merging yet.

@pnkfelix
Copy link
Member

As noted on #6061, this (commit 6d50d55 does not fix #6061. See that bug for more details.)

@huonw
Copy link
Member Author

huonw commented Apr 26, 2013

@pcwalton mainly because there are at least 2 other pull requests touching this area of the code, and there are a few other commits I want to add to this. (A review on #6058 would be nice though.)

@pnkfelix thanks.

@huonw
Copy link
Member Author

huonw commented Apr 27, 2013

@thestinger I did some more experimentation, and by adding #[inline] to the isaac fn, I get

real    0m0.506s
user    0m0.492s
sys     0m0.004s

The C version is

real    0m0.458s
user    0m0.452s
sys 0m0.000s

@huonw
Copy link
Member Author

huonw commented Apr 28, 2013

@pcwalton or @graydon, this is ready for a review now.

@Thiez
Copy link
Contributor

Thiez commented Apr 29, 2013

I couldn't help but notice you use mut-fields in the implementation. I suppose the compiler won't complain because core.rc has #[allow(deprecated_mutable_fields)], but eventually we'll want to get rid of that. I think the options are either changing the Rng and Rand to use next(&mut self) and rand(rng: &mut R) or adding unsafe code + transmute_mut in your implementation. To me the former makes more sense; all PRNGs are going to have some kind of internal state that is modified when generating numbers, why have a Rand trait that pretends otherwise?

@thestinger
Copy link
Contributor

I just want to point out that working around mutability issues with transmutes is incorrect and will break when Rust communicates aliasing/mutability information to LLVM. It shouldn't be considered as an option.

This replaces the wrapper around the runtime RNG with a pure Rust
implementation of the same algorithm. This is faster (up to 5x), and
is hopefully safer.

There is still much room for optimisation: testing by summing 100,000,000
random `u32`s indicates this is about 40-50% slower than the pure C
implementation (running as standalone executable, not in the runtime).
@huonw
Copy link
Member Author

huonw commented Apr 29, 2013

@Thiez I totally agree that things should take &mut self, however I was going to do that as part of the larger changes to rand that are in progress.

bors added a commit that referenced this pull request Apr 30, 2013
This replaces the wrapper around the runtime RNG with a pure Rust implementation of the same algorithm. This is much faster (up to 5x), and is hopefully safer.

There is still (a little) room for optimisation: testing by summing 100,000,000 random `u32`s indicates this is about ~~40-50%~~ 10% slower than the pure C implementation (running as standalone executable, not in the runtime).

(Only 6d50d55 is part of this PR, the first two are from #6058, but are required for the rt rng to be correct to compare against in the tests.)
@bors bors closed this Apr 30, 2013
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Jun 22, 2024
Fixes 6069

Calling `item.ident.as_str()` returns an NFC normalized ident, which
might not be what's written in the source code. To avoid panics when
calling `snippet_provider.span_after` use the ident from the source.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants