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 MKL as julia 1.7 dependency + minor style cleanup #26

Merged
merged 1 commit into from
Nov 29, 2021

Conversation

Seelengrab
Copy link
Contributor

@Seelengrab Seelengrab commented Nov 18, 2021

Hi!

Julia 1.7 will feature a mechanism called "BLAS trampoline", which allows for loading a BLAS library at runtime (see here). This PR makes use of that and adds MKL as a dependency (plus project management/instantiation stuff). Someone running the benchmark doesn't need to have MKL installed already, it's installed during the activation of the julia project before the benchmarks are run. This will only work with julia 1.7+ though (which should be released relatively soon), so if that's a concern I can add a note somewhere saying that the benchmarks require it.

I've also fixed the benchmarking script, since the existing version in the repo tried to run a test_riccati.jl.in file, which didn't exist. I've also made it so that the NM and NREP parameter is passed on the command line instead of hardcoding via a file.

With this and the removal of some unnecessary (I think) copies, I get these results locally:

riccati_benchmark

Note that I've only run python3 run_benchmark_julia.py as well as python3 run_benchmark_numpy.py, as I don't know how to run your BLASFEO benchmarks. I suspect the remaining difference in benchmarking times is due to this, as well as my laptop not being as fast as the machine you've run your benchmarks on. Nevertheless, since Julia+MKL can match performance of prometeo/BLASFEO once the matrix size becomes large enough i.e. we're memory bound, I suspect the speedup is real.

Cheers!


# Riccat recursion

function riccati_trf(N, nx, nu, BAt, RSQ, L, LN, BAtL, M)

LN = copy(RSQ[nu+1:nu+nx, nu+1:nu+nx]);
LN = RSQ[nu+1:nu+nx, nu+1:nu+nx];

Choose a reason for hiding this comment

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

Maybe remove the semicolon(s) here, too, while you are at it?

Suggested change
LN = RSQ[nu+1:nu+nx, nu+1:nu+nx];
LN = RSQ[nu+1:nu+nx, nu+1:nu+nx]

@oscardssmith
Copy link

@zanellia from your perspective, is this ready to merge?

@zanellia
Copy link
Owner

Hey @Seelengrab thanks a lot for the PR and sorry for the loose response.

as well as my laptop not being as fast as the machine you've run your benchmarks on
It could also be the other way around though. Just to have a rough estimate which processor did you use for the benchmarks? Will merge anyway ;)

@zanellia zanellia merged commit dee5c66 into zanellia:master Nov 29, 2021
@oscardssmith
Copy link

IMO, the best answer would be for you to re-run the benchmark as that will ensure consistency.

@Seelengrab
Copy link
Contributor Author

I've run this on a Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz, which by now is a somewhat dated laptop CPU :)

I do agree with @oscardssmith though, a uniform rerun so you can update your graphics (julia 1.7 will be released relatively soon, as far as I know) is probably best.

@zanellia
Copy link
Owner

IMO, the best answer would be for you to re-run the benchmark as that will ensure consistency.

@oscardssmith Sure. I was not going to patch the data points together XD.

I've run this on a Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz, which by now is a somewhat dated laptop CPU :)

I do agree with @oscardssmith though, a uniform rerun so you can update your graphics (julia 1.7 will be released relatively soon, as far as I know) is probably best.

@Seelengrab awesome, thanks! I will rerun everything on my laptop (FYI, back then the benchmark was run on an i7-7560U CPU running at 2.30 GHz)

@oscardssmith
Copy link

Have you run the benchmarks yet? I'm really interested in seeing how well this works when all the benchmarks are on the same machine.

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