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

Initial implementation benchmarks for the rune cli #240

Merged
merged 10 commits into from Aug 6, 2021
Merged

Conversation

saidinesh5
Copy link
Contributor

@saidinesh5 saidinesh5 commented Aug 5, 2021

We use the criterion benchmark framework for this task, as the built in rust benchmarks aren't yet available in rust stable.
Each of the benchmarks use a sample size of 10 instead of the default 100.

These benchmarks measure the time taken for:

  1. build - sine rune
  2. inference - sine rune, microspeech, style transfer.

The results on my Linux Ryzen 7 1700 machine look like:

image

TODO: Try to enable Github Actions for these benchmarks These benchmarks can be run manually before each release, so we aren't adding them to github actions for now

Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan left a comment

Choose a reason for hiding this comment

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

Overall I'm really happy with this PR!

As discussed on Slack we might either choose to ignore build times or only run benchmarks locally before a release. I'm lazy so I'd prefer to just have CI run our benchmarks every time something gets merged into master, but including it as a manual step in our release process might work too. What do you think will be the best way forward?

There are a couple stylistic comments (e.g. functions take PathBuf by value or &Path by reference, but not &PathBuf because then you've got double indirection), but that's something a quick clippy run would point out so I'm not overly concerned.

crates/rune-cli/benches/rune_benchmark.rs Outdated Show resolved Hide resolved
crates/rune-cli/benches/rune_benchmark.rs Outdated Show resolved Hide resolved
crates/rune-cli/benches/rune_benchmark.rs Outdated Show resolved Hide resolved
crates/rune-cli/benches/rune_benchmark.rs Outdated Show resolved Hide resolved

assert!(blob.len() != 0);

rune_build_dir.close().expect("Unable to close the rune build directory");
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't call it then the destructor will make sure our temporary directory is cleaned up so it's up to you whether you want to write an explicit close() here and blow up if it failed or skip an extra line of code.

From their docs:

Although TempDir removes the directory on drop, in the destructor any errors are ignored. To detect errors cleaning up the temporary directory, call close instead.

Comment on lines 88 to 89
let base = example_dir().join("sine");
let rune = parse_runefile(base.join("Runefile.yml").as_path()).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more personal preference than anything else, but I'd typically write it like this:

Suggested change
let base = example_dir().join("sine");
let rune = parse_runefile(base.join("Runefile.yml").as_path()).unwrap();
let base = example_dir().join("sine");
let runefile = base.join("Runefile.yml");
let rune = parse_runefile(&runefile).unwrap();

Or alternatively you could make parse_runefile() accept anything which is impl AsRef<Path>. That would also let us accept string literals if we were using it for other tests.

crates/rune-cli/benches/rune_benchmark.rs Outdated Show resolved Hide resolved
crates/rune-cli/benches/rune_benchmark.rs Outdated Show resolved Hide resolved
@saidinesh5 saidinesh5 marked this pull request as ready for review August 5, 2021 11:27
saidinesh5 and others added 10 commits August 6, 2021 13:40
TODO: Refactor out the project_root() code into utils
Since Just this one benchmark is taking an average of 3 minutes on my
machine, I haven't added benchmarks that test out building other runes.
Co-authored-by: Michael Bryan <michael@hotg.ai>
Co-authored-by: Michael Bryan <michael@hotg.ai>
Co-authored-by: Michael Bryan <michael@hotg.ai>
@saidinesh5 saidinesh5 merged commit b920303 into master Aug 6, 2021
@saidinesh5 saidinesh5 deleted the benchmarks branch August 6, 2021 09:12
@f0rodo
Copy link
Contributor

f0rodo commented Aug 7, 2021

This is a long time coming!!

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.

None yet

3 participants