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 Rust implementation of the micro benchmark #22433

Closed
wants to merge 2 commits into from

Conversation

Enet4
Copy link
Contributor

@Enet4 Enet4 commented Jun 19, 2017

I wrote a Rust implementation of the micro benchmark not long ago (repo), and was recommended to include it here with the others.
Although the program is based on the C implementation in "perf.c", it's composed of idiomatic Rust and does not have unsafe code blocks. As for relevant implementation details:

  • The randmatstat and randmatmul benchmarks have two modes. By default, they will use ndarray, a library analogous to numpy, which provides experimental support for BLAS in some operations. The program can be compiled to use the blas crate instead, which may be slightly faster but is not as idiomatic and requires unsafe code blocks.
  • The pseudorandom number generator is the 64-bit implementation of Mersenne Twister 19937 from the mersenne_twister crate.

The necessary preparation steps for the benchmark machine:

  1. Install Rustup
  2. Add the toolchain mentioned in the rust-toolchain file: rustup toolchain add nightly-2017-10-28

At this point, make benchmark/rust.csv should work fine.

The performance measurements from my machine can be seen on this graph. Still, it would be great to have it measured alongside the other languages in your system. Please let me know of necessary changes or questions about the code.

@ararslan ararslan added the potential benchmark Could make a good benchmark in BaseBenchmarks label Jun 19, 2017
@Enet4 Enet4 force-pushed the rust-micro-benchmark branch from c48a90b to 2025fb2 Compare June 19, 2017 14:03
@ViralBShah
Copy link
Member

This is great. But I don't think it should require a nightly release of rust. That is not going to be reproducible. Can we wait until we have a release version that can be used for this purpose?

@Enet4
Copy link
Contributor Author

Enet4 commented Jun 19, 2017

@ViralBShah Hmm... The unstable test crate is needed here for the black_box function, which informs the compiler not to assume anything about a value. In particular, fib would trigger LLVM's invariant code motion without it, making the last 4 iterations (out of 5) use a hoisted value instead of recalculating the Fibonacci numbers. And by the looks of it, black_box won't be stabilized any time soon (rust-lang/rfcs#1484).

There may be a way around this, however. Although it wouldn't behave exactly the same, I could wrap certain values in the code around a Volatile type, which would make stable+safe Rust. Doing this for recursive calls seems to prevent invariant code motion, at least. If you find using a stable compiler important, I believe this can be done without changing the benchmarks' fairness too much.

@Enet4
Copy link
Contributor Author

Enet4 commented Jun 19, 2017

That is not going to be reproducible.

Now that I think of it, this wouldn't be a problem either, since we can instruct rustup to use a specific nightly toolchain (even by release date).

Enet4 added 2 commits October 29, 2017 18:08
- use ndarray crate for randmatstat and randmatmul by default
- enable direct BLAS use with a cargo feature
- provide rust-toolchain override file
- fix trace calculation in randmatstat
- other code refactors
@Enet4 Enet4 force-pushed the rust-micro-benchmark branch from 2025fb2 to a8ef082 Compare October 29, 2017 18:37
@Enet4
Copy link
Contributor Author

Enet4 commented Oct 29, 2017

Here's an update:

Recent developments over the blas crate have revealed that its functions were actually unsafe, since they do not check whether the input slice sizes are appropriate. Their use in this benchmark was ok, since we were (fairly) sure that the matrix vectors had the right sizes for the matrix multiplications. However, the use of unsafe code is not taken lightly in Rust development, and alternatives should be considered unless when absolutely necessary.

With that in mind, I have updated the randmatstat and randmatmul benchmark to use 2-dimensional arrays from the ndarray crate (this would be analogous to using numpy instead). ndarray is a pure Rust implementation which contains experimental support for BLAS in some operations. This was also an opportunity to make the code more idiomatic and readable, at the expense of a bit more overhead at run time. The same benchmark can be compiled to use the previous implementation (with direct use of the BLAS API) by adding the direct_blas feature. The bug found in #24371 was also fixed here.

Furthermore, since Rustup supports toolchain override files, I've added a file pointing to a recent nightly toolchain. So, the full process would be:

  1. Install Rustup
  2. Add the toolchain mentioned in the rust-toolchain file: rustup toolchain add nightly-2017-10-28
  3. cargo run --release (or cargo run --release --features direct_blas for direct BLAS implementation)

@ViralBShah
Copy link
Member

We generally want to move the performance benchmarks out of Julia Base. Thus closing this for now.

@ViralBShah ViralBShah closed this Mar 4, 2018
@ViralBShah
Copy link
Member

ViralBShah commented Mar 4, 2018

If this was ready to merge, please let me know and I am happy to merge it. The hope is that we will move the whole test/perf directory out of Base, and if this PR is merged, the rust implementation would also move out then.

cc @ararslan

@Enet4
Copy link
Contributor Author

Enet4 commented Mar 4, 2018

I have been keeping a fresh version of the implementation in its own repository, with updated dependencies and corrected output to the latest benchmark names. This branch might not have all changes yet, but refreshing it is a fairly easy task.
I have rebased and updated the branch, which would make it ready to be merged. If this migration to a new repository is going to happen soon, we can also postpone the merge to that time.

@ViralBShah
Copy link
Member

IIRC, it was also using nightly versions of some packages. I'd say that if all the dependent packages are released versions, we can merge it.

Maybe a new PR is required, since the Reopen button is greyed out here.

@Enet4
Copy link
Contributor Author

Enet4 commented Mar 4, 2018

Not really nightly versions of packages, just a nightly version of the compiler. 🙂 It is currently fixed to the version stated in the rust-toolchain file. All dependencies were also fetched from the official crate registry (which is crates.io), and their respective versions locked with the Cargo.lock file. All of this combined should ensure reproducibility in the long run.

I can make another PR, but please let me know if you'd still like the code to be compatible with a stable Rust toolchain.

@KristofferC KristofferC removed the potential benchmark Could make a good benchmark in BaseBenchmarks label Oct 8, 2018
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.

4 participants