Skip to content

Conversation

rylev
Copy link
Member

@rylev rylev commented Mar 22, 2022

The difference in compile times between the 2 benchmarks is substantial:

  • piston-image:
    • check: 4.01s
    • build: 5.59s
    • build --release: 10.20s
  • image-0.24.1
    • check: 11.59s
    • build: 16.50s
    • build --release: 24.03s

The new benchmark does seem like it will measure interesting performance changes. Unfortunately, my local version of perf does not support instruction counts (because I'm running in WSL).

@nnethercote should I remove some of the files that are clearly not relevant for benchmarking (e.g., GitHub Actions yamls, git attributes, etc.)?

@rylev rylev requested a review from nnethercote March 22, 2022 10:58
The difference in compile times between the 2 benchmarks is substantial:

* piston-image:
  * check: 4.01s
  * build: 5.59s
  * build --release: 10.20s
* image-0.24.1
  * check: 11.59s
  * build: 16.50s
  * build --release: 24.03s

The new benchmark does seem like it will measure interesting performance
changes. Unfortunately, my local version of `perf` does not support
instruction counts (because I'm running in WSL).
@rylev rylev force-pushed the add-image-0.24.1-benchmark branch from c0ff2c6 to 334c40d Compare March 22, 2022 17:05
@nnethercote
Copy link
Contributor

Thanks for this, it looks good.

@nnethercote should I remove some of the files that are clearly not relevant for benchmarking (e.g., GitHub Actions yamls, git attributes, etc.)?

I haven't done that. I think it's best to keep things simple, and have the benchmark code match the released code as closely as possible.

Thanks for measuring the overall time. It would be good to check the time for the final crate as well. I added an extra step to the instructions yesterday:

Then compare the final crate time with these commands:

target/release/collector bench_local +nightly --id Test --profiles=Check,Debug,Opt --scenarios=Full --include=$OLD,$NEW,helloworld
target/release/site results.db

Then compare Test against itself, and toggle the “Show only significant changes”/“Filter out very small changes”/“Display raw data” check boxes to make sure it hasn’t changed drastically. (E.g. futures was changed so it’s just a facade for a bunch of sub-crates, and the final crate time became very similar to helloworld, which wasn’t interesting).

Also, there's currently a test failure.

@rylev
Copy link
Member Author

rylev commented Mar 23, 2022

It would be good to check the time for the final crate as well. I added an extra step to the instructions yesterday:

As I noted above, I don't have the ability to measure instruction counts on my machine, but here's the result for cpu-clock:u. I think it's clear from this that the new benchmark makes the compiler work harder than the old benchmark.
image

Also, there's currently a test failure.

The failure was caused by there not being a Cargo.lock file which was not included because the image crate included a .gitignore file which had Cargo.lock in it. Not sure if including this in the how-to doc is worth it, but this seems like a possible failure scenario for other crates. Hopefully, the tests pass now.

@rylev rylev changed the title Add image-0.24.1 as a benchmark Add image-0.24.1 as a benchmark Mar 23, 2022
@rylev rylev merged commit 6037bc0 into rust-lang:master Mar 23, 2022
@rylev rylev deleted the add-image-0.24.1-benchmark branch March 23, 2022 12:39
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.

2 participants