From 396829f69d31a2e22107cdaa3daddab8cd296b54 Mon Sep 17 00:00:00 2001 From: Almog Gavra Date: Wed, 28 May 2025 17:15:23 -0700 Subject: [PATCH 1/6] add benchmark to CI/CD --- .github/workflows/bench.yml | 69 +++++++++++++++++++++++++++++++++++++ scripts/ci_bench.sh | 44 +++++++++++++++++++++++ scripts/format_critcmp.sh | 21 +++++++++++ 3 files changed, 134 insertions(+) create mode 100644 .github/workflows/bench.yml create mode 100755 scripts/ci_bench.sh create mode 100755 scripts/format_critcmp.sh diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml new file mode 100644 index 0000000..8396ed8 --- /dev/null +++ b/.github/workflows/bench.yml @@ -0,0 +1,69 @@ +name: Benchmark CI + +on: + pull_request: + branches: [main] + +jobs: + benchmark: + runs-on: ubuntu-latest + steps: + - name: Checkout PR code + uses: actions/checkout@v3 + with: + fetch-depth: 0 + + - name: Install Rust + uses: actions-rs/toolchain@v1 + with: + toolchain: stable + override: true + + - name: Install critcmp + run: cargo install critcmp + + - name: Run comparison script + run: | + ./scripts/ci_bench.sh + + - name: Post or update benchmark comment + uses: actions/github-script@v7 + with: + github-token: ${{ secrets.CI_CD_TOKEN }} + script: | + const fs = require('fs'); + const marker = ''; + const body = marker + '\n' + fs.readFileSync('benchmark-report.md', 'utf8'); + + // Get all comments on the PR + const { data: comments } = await github.rest.issues.listComments({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + }); + + // Look for a comment containing our marker + const existing = comments.find(comment => + comment.body.includes(marker) && + comment.user.login === context.actor // Ensure it's ours + ); + + if (existing) { + // Update the existing comment + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, + }); + console.log('โœ… Updated existing benchmark comment'); + } else { + // Create a new comment + await github.rest.issues.createComment({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + body, + }); + console.log('๐Ÿ†• Created new benchmark comment'); + } diff --git a/scripts/ci_bench.sh b/scripts/ci_bench.sh new file mode 100755 index 0000000..ea80f6d --- /dev/null +++ b/scripts/ci_bench.sh @@ -0,0 +1,44 @@ +#!/bin/bash +set -euo pipefail + +# Configuration +BENCH_NAME="serde_bench" +WORKTREE_DIR="/tmp/bench-main" +REPORT_FILE="benchmark-report.md" + +# Cleanup trap +cleanup() { + echo "๐Ÿงน Cleaning up..." + git worktree remove --force "$WORKTREE_DIR" 2>/dev/null || true +} +trap cleanup EXIT + +# Check if critcmp is installed +if ! command -v critcmp >/dev/null 2>&1; then + echo "โŒ critcmp is not installed. Please run: cargo install critcmp" + exit 1 +fi + +# Fetch and prepare main branch in a clean worktree +echo "๐Ÿ“ฆ Checking out 'main' into temporary worktree..." +git fetch origin main +git worktree add --force "$WORKTREE_DIR" origin/main + +# Run benchmarks on main branch +echo "๐Ÿš€ Running benchmarks on 'main'..." +( + cd "$WORKTREE_DIR" + cargo bench --bench "$BENCH_NAME" -- --save-baseline main +) +mkdir -p ./target/criterion +cp -r "$WORKTREE_DIR/target/criterion" ./target/criterion + +# Run benchmarks on current branch +echo "๐Ÿš€ Running benchmarks on PR branch..." +cargo bench --bench "$BENCH_NAME" -- --save-baseline pr + +# Compare with critcmp +echo "๐Ÿ“Š Comparing benchmarks..." +critcmp main pr | ./scripts/format_critcmp.sh > "$REPORT_FILE" + +echo "โœ… Benchmark comparison saved to $REPORT_FILE" diff --git a/scripts/format_critcmp.sh b/scripts/format_critcmp.sh new file mode 100755 index 0000000..13c3fe2 --- /dev/null +++ b/scripts/format_critcmp.sh @@ -0,0 +1,21 @@ +#!/bin/bash +set -euo pipefail + +echo '| Group | main | pr |' +echo '|-------|------|----|' + +while IFS= read -r line; do + # Skip empty lines and headers + [[ -z "$line" || "$line" =~ ^group || "$line" =~ ^[-[:space:]]+$ ]] && continue + + # Split by 2+ spaces into group, main, pr + IFS='|' read -r group rest <<< "$(echo "$line" | sed -E 's/ +/|/g')" + IFS='|' read -r main pr <<< "$rest" + + # Trim whitespace + group="$(echo "$group" | xargs)" + main="$(echo "$main" | xargs)" + pr="$(echo "$pr" | xargs)" + + echo "| \`$group\` | \`$main\` | \`$pr\` |" +done From 9c3897cbee90c385589d511a826d837d43475d6c Mon Sep 17 00:00:00 2001 From: Almog Gavra Date: Wed, 28 May 2025 19:36:31 -0700 Subject: [PATCH 2/6] change access token --- .github/workflows/bench.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 8396ed8..2aedeee 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -29,7 +29,7 @@ jobs: - name: Post or update benchmark comment uses: actions/github-script@v7 with: - github-token: ${{ secrets.CI_CD_TOKEN }} + github-token: ${{ secrets.GITHUB_TOKEN }} script: | const fs = require('fs'); const marker = ''; From 7066b404455c145b9249db8fc7348754989f8785 Mon Sep 17 00:00:00 2001 From: Almog Gavra Date: Wed, 28 May 2025 19:41:58 -0700 Subject: [PATCH 3/6] add bot permissions --- .github/workflows/bench.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 2aedeee..57a1f16 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -4,6 +4,10 @@ on: pull_request: branches: [main] +permissions: + issues: write + pull-requests: write + jobs: benchmark: runs-on: ubuntu-latest From 5e05b00fe7e8c1f4ebd3ce8976444957d97f540e Mon Sep 17 00:00:00 2001 From: Almog Gavra Date: Wed, 28 May 2025 21:10:52 -0700 Subject: [PATCH 4/6] cleanup some stuff and fix the comment --- .github/workflows/bench.yml | 4 ++-- scripts/ci_bench.sh | 29 ++++++++++++++++++++++------- scripts/format_critcmp.sh | 21 --------------------- 3 files changed, 24 insertions(+), 30 deletions(-) delete mode 100755 scripts/format_critcmp.sh diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 57a1f16..b957ebd 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -60,7 +60,7 @@ jobs: comment_id: existing.id, body, }); - console.log('โœ… Updated existing benchmark comment'); + console.log('Updated existing benchmark comment'); } else { // Create a new comment await github.rest.issues.createComment({ @@ -69,5 +69,5 @@ jobs: repo: context.repo.repo, body, }); - console.log('๐Ÿ†• Created new benchmark comment'); + console.log('Created new benchmark comment'); } diff --git a/scripts/ci_bench.sh b/scripts/ci_bench.sh index ea80f6d..9eb6c57 100755 --- a/scripts/ci_bench.sh +++ b/scripts/ci_bench.sh @@ -13,12 +13,6 @@ cleanup() { } trap cleanup EXIT -# Check if critcmp is installed -if ! command -v critcmp >/dev/null 2>&1; then - echo "โŒ critcmp is not installed. Please run: cargo install critcmp" - exit 1 -fi - # Fetch and prepare main branch in a clean worktree echo "๐Ÿ“ฆ Checking out 'main' into temporary worktree..." git fetch origin main @@ -39,6 +33,27 @@ cargo bench --bench "$BENCH_NAME" -- --save-baseline pr # Compare with critcmp echo "๐Ÿ“Š Comparing benchmarks..." -critcmp main pr | ./scripts/format_critcmp.sh > "$REPORT_FILE" + +cat < "$REPORT_FILE" +## ๐Ÿ“Š Benchmark Comparison Report + +This pull request includes Criterion benchmarks comparing performance to the \`main\` branch. + +The table below shows **relative ratios** and **timing stats**, and **throughput estimates** for each benchmark group. +A ratio above \`1.00\` means this PR is **slower**, while below \`1.00\` means it is **faster**. + +\`\`\` +$(critcmp main pr) +\`\`\` + +โœ… Benchmarks completed successfully. + +๐Ÿง  **Notes**: +- These benchmarks are not a pass/fail gate. +- Use this as a signal to review performance-sensitive changes. +- Results may vary significantly due to GHA runner hardware variance. + +_Reported by the benchmark CI bot_ +EOF echo "โœ… Benchmark comparison saved to $REPORT_FILE" diff --git a/scripts/format_critcmp.sh b/scripts/format_critcmp.sh deleted file mode 100755 index 13c3fe2..0000000 --- a/scripts/format_critcmp.sh +++ /dev/null @@ -1,21 +0,0 @@ -#!/bin/bash -set -euo pipefail - -echo '| Group | main | pr |' -echo '|-------|------|----|' - -while IFS= read -r line; do - # Skip empty lines and headers - [[ -z "$line" || "$line" =~ ^group || "$line" =~ ^[-[:space:]]+$ ]] && continue - - # Split by 2+ spaces into group, main, pr - IFS='|' read -r group rest <<< "$(echo "$line" | sed -E 's/ +/|/g')" - IFS='|' read -r main pr <<< "$rest" - - # Trim whitespace - group="$(echo "$group" | xargs)" - main="$(echo "$main" | xargs)" - pr="$(echo "$pr" | xargs)" - - echo "| \`$group\` | \`$main\` | \`$pr\` |" -done From 0a9fb785563efb5a22836f59386078f58e68493f Mon Sep 17 00:00:00 2001 From: Almog Gavra Date: Wed, 28 May 2025 21:18:17 -0700 Subject: [PATCH 5/6] fix comment update --- .github/workflows/bench.yml | 3 +-- scripts/ci_bench.sh | 9 +++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index b957ebd..690f219 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -48,8 +48,7 @@ jobs: // Look for a comment containing our marker const existing = comments.find(comment => - comment.body.includes(marker) && - comment.user.login === context.actor // Ensure it's ours + comment.body.includes(marker) ); if (existing) { diff --git a/scripts/ci_bench.sh b/scripts/ci_bench.sh index 9eb6c57..9224765 100755 --- a/scripts/ci_bench.sh +++ b/scripts/ci_bench.sh @@ -39,8 +39,8 @@ cat < "$REPORT_FILE" This pull request includes Criterion benchmarks comparing performance to the \`main\` branch. -The table below shows **relative ratios** and **timing stats**, and **throughput estimates** for each benchmark group. -A ratio above \`1.00\` means this PR is **slower**, while below \`1.00\` means it is **faster**. +The table below shows **relative ratios** and **timing stats** for each benchmark group. +A ratio above \`1.00\` means this PR is **slower**. \`\`\` $(critcmp main pr) @@ -49,9 +49,10 @@ $(critcmp main pr) โœ… Benchmarks completed successfully. ๐Ÿง  **Notes**: -- These benchmarks are not a pass/fail gate. +- These benchmarks are not a pass/fail gate and are informative only. - Use this as a signal to review performance-sensitive changes. -- Results may vary significantly due to GHA runner hardware variance. +- Results may be unreliable due to GHA runner hardware variance. +- If results indicate a significant performance regression, run the benchmarks locally to confirm. _Reported by the benchmark CI bot_ EOF From 269f9995c89cd7c3a007a6515500f00557575357 Mon Sep 17 00:00:00 2001 From: Almog Gavra Date: Wed, 28 May 2025 21:32:32 -0700 Subject: [PATCH 6/6] clarify some of the wording on the comment --- scripts/ci_bench.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/ci_bench.sh b/scripts/ci_bench.sh index 9224765..4da01ad 100755 --- a/scripts/ci_bench.sh +++ b/scripts/ci_bench.sh @@ -38,9 +38,9 @@ cat < "$REPORT_FILE" ## ๐Ÿ“Š Benchmark Comparison Report This pull request includes Criterion benchmarks comparing performance to the \`main\` branch. +This comment will automatically update as the benchmarks are re-run on each commit. -The table below shows **relative ratios** and **timing stats** for each benchmark group. -A ratio above \`1.00\` means this PR is **slower**. +The table below shows **relative ratios** and **timing stats** for each benchmark group: \`\`\` $(critcmp main pr)