diff --git a/.github/scripts/check-benchmarks.sh b/.github/scripts/check-benchmarks.sh new file mode 100755 index 0000000..d5bebb8 --- /dev/null +++ b/.github/scripts/check-benchmarks.sh @@ -0,0 +1,7 @@ +#!/bin/bash +# The benchmarks-ok check. Passes when every benchmark job passed, or when +# there was nothing to benchmark. Usage: check-benchmarks.sh +set -e + +echo "changes: $1, rake: $2" +[ "$1" = success ] && { [ "$2" = success ] || [ "$2" = skipped ]; } diff --git a/.github/scripts/pick-benchmarks.sh b/.github/scripts/pick-benchmarks.sh new file mode 100755 index 0000000..874030b --- /dev/null +++ b/.github/scripts/pick-benchmarks.sh @@ -0,0 +1,35 @@ +#!/bin/bash +# Decides what a CI run benchmarks, and writes `run` and `files` to +# $GITHUB_OUTPUT. Usage: pick-benchmarks.sh +# +# A pull request runs only the benchmark files it changes, unless it changes +# something every benchmark depends on. A push to main runs everything, if +# anything that affects benchmarks changed. +set -e + +base=$1 + +if ! git cat-file -e "$base^{commit}" 2>/dev/null; then + echo "Base $base not found, running every benchmark" + echo "run=true" >> "$GITHUB_OUTPUT" + exit 0 +fi + +changed=$(git diff --name-only "$base" HEAD) +files=$(git diff --name-only --diff-filter=d "$base" HEAD -- 'code/**/*.rb' | tr '\n' ' ') + +if echo "$changed" | grep -qE '^(Gemfile|Rakefile|compose\.yaml|docker/|\.github/workflows/benchmarks\.yml|\.github/scripts/)'; then + echo "Shared files changed, running every benchmark" + echo "run=true" >> "$GITHUB_OUTPUT" +elif [ -z "$files" ]; then + # Docs only, or a PR that only deletes a benchmark. + echo "No benchmark to run" + echo "run=false" >> "$GITHUB_OUTPUT" +elif [ "$GITHUB_EVENT_NAME" = pull_request ]; then + echo "Running: $files" + echo "run=true" >> "$GITHUB_OUTPUT" + echo "files=$files" >> "$GITHUB_OUTPUT" +else + echo "Benchmarks changed, running every benchmark" + echo "run=true" >> "$GITHUB_OUTPUT" +fi diff --git a/.github/scripts/report-failures.sh b/.github/scripts/report-failures.sh new file mode 100755 index 0000000..c52c97e --- /dev/null +++ b/.github/scripts/report-failures.sh @@ -0,0 +1,27 @@ +#!/bin/bash +# After a failed benchmark job: one annotation per failed benchmark, shown on +# the PR's changed files, and a list on the run's summary page. +# Usage: report-failures.sh +set -e + +log=$1 +ruby=$2 + +failed=$(sed -n '/^Failed benchmarks:/,$p' "$log" | grep -E '^code/.+\.rb$' || true) +[ -n "$failed" ] || exit 0 + +hint='If it needs a newer Ruby, make it skip older ones, see "Benchmarks that need a newer Ruby" in CONTRIBUTING.md.' + +{ + echo "### Benchmarks that failed on $ruby" + echo + echo "$failed" | while read -r file; do echo "- \`$file\`"; done + echo + echo "$hint" +} >> "$GITHUB_STEP_SUMMARY" + +echo "$failed" | while read -r file; do + # Annotation properties cannot contain raw % , or : + escaped=$(printf '%s' "$file" | sed 's/%/%25/g; s/,/%2C/g; s/:/%3A/g') + echo "::error file=$escaped,title=Fails on $ruby::Crashed on $ruby. $hint" +done diff --git a/.github/scripts/run-benchmarks.sh b/.github/scripts/run-benchmarks.sh new file mode 100755 index 0000000..74e4607 --- /dev/null +++ b/.github/scripts/run-benchmarks.sh @@ -0,0 +1,11 @@ +#!/bin/bash +# Runs one Ruby's benchmark job and keeps its output in benchmarks.log for +# report-failures.sh. Usage: FILES="..." run-benchmarks.sh +# Without FILES, every benchmark runs. +set -eo pipefail + +# Unquoted on purpose: one argument per file. set -f keeps names like +# dig-vs-[]-vs-fetch.rb from being read as glob patterns. +set -f +# shellcheck disable=SC2086 +docker compose run --rm -T "$1" $FILES 2>&1 | tee benchmarks.log diff --git a/.github/workflows/benchmarks.yml b/.github/workflows/benchmarks.yml index 7c90408..a6c9b80 100644 --- a/.github/workflows/benchmarks.yml +++ b/.github/workflows/benchmarks.yml @@ -1,22 +1,32 @@ name: Benchmarks -# Only changes that can affect a benchmark run it: README-only changes do not. +# Always runs, so the benchmarks-ok check at the end always reports and can be +# required. The changes job decides whether there is anything to benchmark. on: push: branches: [ main ] - paths: &benchmark_paths - - 'code/**/*.rb' - - 'Gemfile' - - 'Rakefile' - - 'compose.yaml' - - 'docker/**' - - '.github/workflows/benchmarks.yml' pull_request: branches: [ main ] - paths: *benchmark_paths jobs: + changes: + runs-on: ubuntu-latest + outputs: + run: ${{ steps.pick.outputs.run }} + files: ${{ steps.pick.outputs.files }} + + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Pick benchmarks + id: pick + # A pull_request checks out a merge commit: HEAD^1 is the base. + run: .github/scripts/pick-benchmarks.sh "${{ github.event_name == 'pull_request' && 'HEAD^1' || github.event.before }}" + rake: + needs: changes + if: needs.changes.outputs.run == 'true' runs-on: ubuntu-latest timeout-minutes: 60 @@ -38,36 +48,22 @@ jobs: run: | echo "SHARE=1" >> "$GITHUB_ENV" - uses: actions/checkout@v4 - with: - # A pull_request checks out a merge commit: HEAD^1 is the base. - fetch-depth: 2 - # On a pull request, run only the benchmark files it changes, unless it - # changes something every benchmark depends on. Pushes to main run all. - - name: Pick benchmarks - id: pick - if: github.event_name == 'pull_request' - run: | - changed=$(git diff --name-only HEAD^1 HEAD) - if echo "$changed" | grep -qE '^(Gemfile|Rakefile|compose\.yaml|docker/|\.github/workflows/benchmarks\.yml)'; then - echo "Shared files changed, running every benchmark" - else - files=$(git diff --name-only --diff-filter=d HEAD^1 HEAD -- 'code/**/*.rb' | tr '\n' ' ') - if [ -z "$files" ]; then - # For example a PR that only deletes a benchmark. - echo "No benchmark to run" - echo "skip=true" >> "$GITHUB_OUTPUT" - else - echo "Running: $files" - echo "files=$files" >> "$GITHUB_OUTPUT" - fi - fi - name: Run benchmarks on ${{ matrix.ruby }} - if: steps.pick.outputs.skip != 'true' env: - FILES: ${{ steps.pick.outputs.files }} - run: | - # Unquoted on purpose: one argument per file. set -f keeps names like - # dig-vs-[]-vs-fetch.rb from being read as glob patterns. - set -f - # shellcheck disable=SC2086 - docker compose run --rm -T ${{ matrix.ruby }} $FILES + FILES: ${{ needs.changes.outputs.files }} + run: .github/scripts/run-benchmarks.sh ${{ matrix.ruby }} + - name: Report failed benchmarks + if: failure() + run: .github/scripts/report-failures.sh benchmarks.log ${{ matrix.ruby }} + + # The check to require on main. Passes when every benchmark job passed, or + # when there was nothing to benchmark. + benchmarks-ok: + needs: [changes, rake] + if: always() + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v4 + - name: Check the benchmark jobs + run: .github/scripts/check-benchmarks.sh ${{ needs.changes.result }} ${{ needs.rake.result }} diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 843b17c..28ffaf5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -8,6 +8,13 @@ These idioms list here are trying to satisfy following goals: [![GOALS](/images/Goals.png)](https://speakerdeck.com/sferik/writing-fast-ruby?slide=11) +## Contents + +- [Note on entry](#note-on-entry) +- [Running it on other Rubies](#running-it-on-other-rubies) +- [Benchmarks that need a newer Ruby](#benchmarks-that-need-a-newer-ruby) +- [License](#license) + ## Note on entry Fast code first. @@ -31,9 +38,11 @@ end Run your result: ``` -$ ruby -v code/your-new/entry.rb +ruby -v code/your-new/entry.rb ``` +## Running it on other Rubies + To run it on a Ruby you don't have installed, use Docker. There is one service per Ruby in the CI matrix (see `compose.yaml`): @@ -51,12 +60,42 @@ the head builds go stale. To get the latest nightly build: docker compose build --no-cache ruby_head ``` +## Benchmarks that need a newer Ruby + +CI runs every benchmark on every Ruby in `compose.yaml`, back to Ruby 2.1, and +fails when one crashes. If your entry uses something older Rubies do not have, +make it skip them. + +Skip one report, so the rest still run everywhere: + +```ruby +Benchmark.ips do |x| + x.report('String#delete_suffix') { fast } if RUBY_VERSION >= '2.5.0' + x.report('String#sub') { slow } + x.compare! +end +``` + +Skip the whole file when nothing in it makes sense without the feature: + +```ruby +if RUBY_VERSION >= '2.5.0' + # everything, including Benchmark.ips +end +``` + +New syntax (for example `<<~` before 2.3) cannot be skipped this way: older +Rubies fail to parse the file before the `if` runs. Write it with syntax they +understand instead. + +To check an entry on an older Ruby, see [Running it on other Rubies](#running-it-on-other-rubies). + +## License + Thanks in advance!!! Look forward to learning more from you! <3 [JuanitoFatas](https://twitter.com/juanitofatas) -###### License - The documentation is [CC BY-SA 4.0 (International)](https://github.com/JuanitoFatas/fast-ruby#license). And code will be [CC0 1.0 Universal](https://github.com/JuanitoFatas/fast-ruby#code-license). diff --git a/Rakefile b/Rakefile index a040a1e..222a90e 100644 --- a/Rakefile +++ b/Rakefile @@ -12,7 +12,11 @@ task :run_benchmark do failed << benchmark unless system("ruby", "-v", "-W0", benchmark) end - abort "Failed benchmarks:\n#{failed.join("\n")}" unless failed.empty? + unless failed.empty? + abort "Failed benchmarks:\n#{failed.join("\n")}\n\n" \ + "If a benchmark needs a newer Ruby, make it skip older ones, see " \ + "\"Benchmarks that need a newer Ruby\" in CONTRIBUTING.md." + end end task default: :run_benchmark diff --git a/docker/run-benchmarks.sh b/docker/run-benchmarks.sh index 61cb1e8..514efac 100755 --- a/docker/run-benchmarks.sh +++ b/docker/run-benchmarks.sh @@ -23,5 +23,8 @@ done if [ -n "$failed" ]; then echo "Failed benchmarks:$failed" >&2 + echo >&2 + echo "If a benchmark needs a newer Ruby, make it skip older ones, see" \ + "\"Benchmarks that need a newer Ruby\" in CONTRIBUTING.md." >&2 exit 1 fi