Implementation of the Xorwow pseudorandom number generator

code review
1

For some reason there is no rand compatible crate that implements the Xorwow algorithm. So I decided to give it a try, and now I am here to kindly ask for a code review.

Repo: https://github.com/elkasztano/xorwow

Some questions:

  • I am not sure where to put the #[inline] attribute. Is it even necessary or is the compiler smart enough to figure it out itself?

  • Is there anything missing in the documentation? Should I add more examples?

  • Currently, the project is only available on Github. Should I publish it on crates io?

Any suggestions for improvement are highly appreciated. Thank you in advance!

2

You should measure that. Compilers are often better at optimizing code than humans, and inlining is a very basic optimization. It's not as simple as "slapping inline on every function magically makes code fast". I would not add it by default.

If you want it to be widely available, yes – people generally don't want to depend on git repositories.

Another comment I have is about the surface syntax – you are using way too much vertical whitespace, you don't need to add empty lines after every closing brace. The way you are placing closing parentheses/brackets/braces is also non-idiomatic, try running your code through rustfmt.

You also seem to be extensively using for_each where it's not really warranted. This:

                (2..($nr-1)).rev().for_each(|i| {
                    self.s[i] = self.s[i-1];
                });

would be clearer and less noisy like this:

                for i in (2..($nr - 1)).rev() {
                    self.s[i] = self.s[i - 1];
                }

Actually, thinking of it, this is just <[T]>::fill().

3 Likes
3

And make sure to measure it from another crate — like a benchmark target, not a test you write inside the library crate. Cross-crate inlining is more conditional than same-crate inlining, so in order to understand the behavior that your library’s users will see, you have to use a separate crate.

3 Likes
4

Thank you very much for your comment!

I have refactored the code and I also gave rustfmt a try. The code looks a lot cleaner now.

A quick benchmark without external dependencies didn't show much difference with or without #[inline]. I think I will let the compiler decide, at least for now.

5

Testing from another crate is a good idea, I will try that out. Might criterion be a good choice?

I totally forgot how fast those generators are, btw. Generating one billion (!) pseudorandom numbers took about 3.5 seconds on my machine.

6

Yes, I use Criterion. Divan is another notable benchmark library, and I've heard it's lighter-weight than Criterion.

2 Likes
7

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.