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.

Sign up for GitHub

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

Jump to bottom

Use cargo-binstall to fast install critcmp for benchmark-compare #3637

Merged
merged 2 commits into from Mar 21, 2023
Merged

Use cargo-binstall to fast install critcmp for benchmark-compare #3637

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa46579
Add Swatinem/rust-cache to benchmark-compare job
JonathanPlasse Mar 20, 2023
e2cf163
Use cargo-binstall to install critcmp
JonathanPlasse Mar 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
116 changes: 59 additions & 57 deletions .github/workflows/benchmark.yaml
View file
Open in desktop
Expand Up @@ -26,7 +26,7 @@ jobs:
- name: "PR - Install Rust toolchain"
run: rustup show

- uses: Swatinem/rust-cache@v1
- uses: Swatinem/rust-cache@v2
Copy link
Member

Choose a reason for hiding this comment

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

whoops, thx


- name: "PR - Build benchmarks"
uses: actions-rs/cargo@v1
Expand Down Expand Up @@ -75,59 +75,61 @@ jobs:
- run-benchmark

steps:
- name: "Install Rust toolchain"
run: rustup show

- name: "Install critcmp"
# Use debug build: Building takes much longer than the "slowness" of using the debug build.
run: cargo install --debug critcmp

- name: "Linux | Download PR benchmark results"
uses: actions/download-artifact@v3
with:
name: benchmark-results-ubuntu-latest
path: ./target/criterion

- name: "Linux | Compare benchmark results"
shell: bash
run: |
echo "### Benchmark" >> summary.md
echo "#### Linux" >> summary.md
echo "\`\`\`" >> summary.md
critcmp main pr >> summary.md
echo "\`\`\`" >> summary.md
echo "" >> summary.md

- name: "Linux | Cleanup benchmark results"
run: rm -rf ./target/criterion

- name: "Windows | Download PR benchmark results"
uses: actions/download-artifact@v3
with:
name: benchmark-results-windows-latest
path: ./target/criterion

- name: "Windows | Compare benchmark results"
shell: bash
run: |
echo "#### Windows" >> summary.md
echo "\`\`\`" >> summary.md
critcmp main pr >> summary.md
echo "\`\`\`" >> summary.md
echo "" >> summary.md

echo ${{ github.event.pull_request.number }} > pr-number

cat summary.md > $GITHUB_STEP_SUMMARY

- uses: actions/upload-artifact@v3
name: Upload PR Number
with:
name: pr-number
path: pr-number

- uses: actions/upload-artifact@v3
name: Upload Summary
with:
name: summary
path: summary.md
- name: "Install Rust toolchain"
run: rustup show

- uses: Swatinem/rust-cache@v2
Copy link
Member

Choose a reason for hiding this comment

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

I considered adding rust-cache to the critcmp step but decided against it because GitHub limits the cache per repository to 10GB. That means GitHub starts evicting cached artifacts once we exceed the 10GB limit. You can see all our cache artifacts here.

Now, the reason I excluded critcmp was to avoid adding yet another cache (where each cache is around 600MB) that results in the eviction of other cached artifacts (e.g., for the clippy process) because I thought having warm caches for clippy is more important than for the benchmark results.

I don't know if that's the right call, nor have I verified my assumption that caching the critcmp build causes any build time regression for other jobs. So happy to give it a try. But do we need to built critcmp in the first place...

I researched more to find ways to avoid building critcmp altogether. I was excited when I read that criterion 0.4 has built-in tabulating. No need to use critcmp, we can generate the comparison directly in the benchmark step. Unfortunately, they removed tabulating before the release in favor of adding it to cargo-criterion. I tried cargo criterion locally, but the feature doesn't seem to exist, and even if it does, we would only be switching one dependency for another.

So what if we could avoid running cargo install altogether? Can we download the critcmp binary or cache it somehow? That's when I discovered cargo-binstall. It installs binaries rather than building the crates from source... yay! Uhm, okay, but how do we install cargo-binstall?

cargo-binstall recommends using install-development-tools: uses: taiki-e/install-action@cargo-binstall. All that's left to do is to run cargo binstall critcmp (maybe figure out the right options)

Luckily cargo-binstall publishes prebuilt binaries on GitHub and there are even a github actions to download binaries from the release page (release-downloader or dtonlay/install). Alternatively, we could just use curl or wget to get the binary. Do you want to give this at try?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, critcmp does not seem to have prebuilt binaries on GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary as far as I understand. You can use cargo binstall to install critcmp. It uses the quickinstall registry under the hood that provides pre-built binaries (at least it works on my linux machine)

❯ cargo binstall critcmp -y
 INFO resolve: Resolving package: 'critcmp'
 WARN The package critcmp v0.1.7 will be downloaded from third-party source QuickInstall
 INFO This will install the following binaries:
 INFO   - critcmp (critcmp -> /home/micha/.cargo/bin/critcmp-v0.1.7)
 INFO And create (or update) the following symlinks:
 INFO   - critcmp (/home/micha/.cargo/bin/critcmp -> critcmp-v0.1.7)
 INFO Installing binaries...
 INFO Done in 2.070975169s


- name: "Install critcmp"
# Use debug build: Building takes much longer than the "slowness" of using the debug build.
run: cargo install --debug critcmp

- name: "Linux | Download PR benchmark results"
uses: actions/download-artifact@v3
with:
name: benchmark-results-ubuntu-latest
path: ./target/criterion

- name: "Linux | Compare benchmark results"
shell: bash
run: |
echo "### Benchmark" >> summary.md
echo "#### Linux" >> summary.md
echo "\`\`\`" >> summary.md
critcmp main pr >> summary.md
echo "\`\`\`" >> summary.md
echo "" >> summary.md

- name: "Linux | Cleanup benchmark results"
run: rm -rf ./target/criterion

- name: "Windows | Download PR benchmark results"
uses: actions/download-artifact@v3
with:
name: benchmark-results-windows-latest
path: ./target/criterion

- name: "Windows | Compare benchmark results"
shell: bash
run: |
echo "#### Windows" >> summary.md
echo "\`\`\`" >> summary.md
critcmp main pr >> summary.md
echo "\`\`\`" >> summary.md
echo "" >> summary.md

echo ${{ github.event.pull_request.number }} > pr-number

cat summary.md > $GITHUB_STEP_SUMMARY

- uses: actions/upload-artifact@v3
name: Upload PR Number
with:
name: pr-number
path: pr-number

- uses: actions/upload-artifact@v3
name: Upload Summary
with:
name: summary
path: summary.md