From f406bd3fd661437eee919d7018d4a802457e47c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Heres?= Date: Fri, 21 Aug 2026 15:18:30 +0200 Subject: [PATCH 01/10] ci: add opt-in TPC-H SF1 performance regression check for PRs Adds a `Benchmarks` workflow that runs TPC-H SF1 twice on one machine -- once for the base branch, once for the PR merged into it -- and fails when the PR is slower than the configured limits allow. The data is generated with tpchgen-cli, both sides are built with `--release` into separate target directories, and each side is measured with `benchmark_runner tpch --output`, so the JSON results feed straight into `compare.py`. `compare.py` grows the gate it needs for that: `--fail-threshold` and `--fail-total-threshold` make it exit non-zero on a per-query or total-time regression (both off by default, so existing usage is unchanged), a query that fails on only one side is reported instead of being silently dropped, and named query ids such as `tpch/Q01/sf1` are no longer rendered as `Qtpch/Q01/sf1`. Co-Authored-By: Claude Opus 5 --- .github/workflows/benchmark.yml | 194 ++++++++++++++++++++++++++++++++ benchmarks/README.md | 32 ++++++ benchmarks/compare.py | 123 +++++++++++++++++++- 3 files changed, 344 insertions(+), 5 deletions(-) create mode 100644 .github/workflows/benchmark.yml diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml new file mode 100644 index 0000000000000..c4d47ffe35d1e --- /dev/null +++ b/.github/workflows/benchmark.yml @@ -0,0 +1,194 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Catches performance regressions by running TPC-H SF1 twice on the same +# machine: once for the base branch, once for the PR merged into it. Only the +# ratio between the two runs is used, so the (shared, virtualized) runner does +# not have to be fast -- just consistent for the duration of the job. + +name: Benchmarks + +concurrency: + group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} + cancel-in-progress: true + +# Two release builds plus two benchmark runs take about an hour, and benchmarks +# on shared runners are noisy, so this is opt-in: add the `performance` label to +# a PR, or start it by hand from the Actions tab. +on: + pull_request: + types: [opened, synchronize, reopened, labeled] + workflow_dispatch: + inputs: + iterations: + description: 'Iterations per query (the fastest one is compared)' + type: string + default: '5' + query_regression: + description: 'Fail if a single query is more than this much slower' + type: string + default: '1.20' + total_regression: + description: 'Fail if the total time is more than this much slower' + type: string + default: '1.05' + +permissions: + contents: read + +jobs: + tpch-sf1: + name: TPC-H SF1 (PR vs base) + if: github.event_name == 'workflow_dispatch' || contains(github.event.pull_request.labels.*.name, 'performance') + # A single job, so both binaries are measured on the same machine. + runs-on: ${{ vars.USE_RUNS_ON == 'true' && format('runs-on={0},family=c8a+m8a,cpu=16,image=ubuntu24-full-x64,extras=s3-cache,disk=large,tag=datafusion', github.run_id) || 'ubuntu-latest' }} + # note: no rust container, so the two builds get plain `--release` + # binaries, comparable with what `benchmarks/bench.sh` produces locally + timeout-minutes: 150 + env: + ITERATIONS: ${{ inputs.iterations || '5' }} + QUERY_REGRESSION: ${{ inputs.query_regression || '1.20' }} + TOTAL_REGRESSION: ${{ inputs.total_regression || '1.05' }} + # Same cargo network settings as .github/actions/setup-rust-runtime, + # without its RUSTFLAGS: benchmark binaries are built with the defaults. + CARGO_HTTP_MULTIPLEXING: "false" + CARGO_NET_RETRY: "10" + CARGO_HTTP_RETRY: "10" + steps: + - uses: runs-on/action@46910bf61b41721b0579f237e186afb35477007a # v2.3.0 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # For `pull_request` this is the PR already merged into the base + # branch. Full history so the base commit can be built as well. + fetch-depth: 0 + - name: Free Disk Space (Ubuntu) + uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be # v1.3.1 + - name: Configure paths + run: | + echo "DATA_DIR=$RUNNER_TEMP/bench-data" >> "$GITHUB_ENV" + echo "RESULTS_DIR=$RUNNER_TEMP/results" >> "$GITHUB_ENV" + - name: Install Rust + run: | + if ! command -v rustup > /dev/null; then + curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --default-toolchain none + echo "$HOME/.cargo/bin" >> "$GITHUB_PATH" + export PATH="$HOME/.cargo/bin:$PATH" + fi + # installs the channel pinned by rust-toolchain.toml + rustup toolchain install + - name: Install uv + uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0 + - name: Install tpchgen-cli + run: cargo install tpchgen-cli --locked + - name: Resolve base commit + id: base + env: + BASE_REF: ${{ github.event.pull_request.base.ref || github.event.repository.default_branch }} + run: | + set -euo pipefail + if [ "$(git rev-list --parents -n 1 HEAD | wc -w)" -ge 3 ]; then + # HEAD is the PR merged into the base branch, so its first parent + # is the base branch tip that the merge used -- the commit this PR + # would actually land on. + base_sha=$(git rev-parse "HEAD^1") + else + # Manual run on a branch: compare it as-is against the base tip. + git fetch --no-tags origin "$BASE_REF" + base_sha=$(git rev-parse FETCH_HEAD) + echo "HEAD is not a merge commit, comparing it as-is against $BASE_REF" + fi + echo "sha=${base_sha}" >> "$GITHUB_OUTPUT" + echo "base: ${base_sha} $(git log -1 --format=%s "${base_sha}")" + echo "candidate: $(git rev-parse HEAD) $(git log -1 --format=%s HEAD)" + - name: Check out base commit + run: git worktree add --detach "$RUNNER_TEMP/base" "${{ steps.base.outputs.sha }}" + - name: Generate TPC-H SF1 data + # Same generator settings as `bench.sh data tpch`, inlined because that + # path also downloads the expected answers through `docker run -it`, + # which needs a TTY and is only used for result validation. + run: | + mkdir -p "$DATA_DIR/tpch_sf1" "$RESULTS_DIR/base" "$RESULTS_DIR/pr" + tpchgen-cli \ + --scale-factor 1 \ + --format parquet \ + --parquet-compression 'ZSTD(1)' \ + --parts=1 \ + --output-dir "$DATA_DIR/tpch_sf1" + # Separate target directories: the two source trees share crate names and + # versions, so one target directory would make each build evict the other. + - name: Build base benchmark runner + working-directory: ${{ runner.temp }}/base + env: + CARGO_TARGET_DIR: ${{ runner.temp }}/target-base + run: cargo build --release -p datafusion-benchmarks --bin benchmark_runner + - name: Build PR benchmark runner + env: + CARGO_TARGET_DIR: ${{ runner.temp }}/target-pr + run: cargo build --release -p datafusion-benchmarks --bin benchmark_runner + # Each runner reads the queries of the tree it was built from (they are + # resolved relative to its own `benchmarks` directory), and both read the + # one generated dataset. + - name: Benchmark base + working-directory: ${{ runner.temp }}/base/benchmarks + run: | + "$RUNNER_TEMP/target-base/release/benchmark_runner" tpch \ + --scale-factor 1 \ + --format parquet \ + --iterations "$ITERATIONS" \ + --path "$DATA_DIR" \ + --output "$RESULTS_DIR/base/tpch_sf1.json" + - name: Benchmark PR + working-directory: benchmarks + run: | + "$RUNNER_TEMP/target-pr/release/benchmark_runner" tpch \ + --scale-factor 1 \ + --format parquet \ + --iterations "$ITERATIONS" \ + --path "$DATA_DIR" \ + --output "$RESULTS_DIR/pr/tpch_sf1.json" + - name: Compare + run: | + set -uo pipefail + status=0 + uv run --no-project --with rich python3 benchmarks/compare.py \ + "$RESULTS_DIR/base/tpch_sf1.json" \ + "$RESULTS_DIR/pr/tpch_sf1.json" \ + --fail-threshold "$QUERY_REGRESSION" \ + --fail-total-threshold "$TOTAL_REGRESSION" \ + > "$RESULTS_DIR/comparison.txt" 2>&1 || status=$? + cat "$RESULTS_DIR/comparison.txt" + { + echo "### TPC-H SF1: base (${{ steps.base.outputs.sha }}) vs PR" + echo + echo "\`$ITERATIONS\` iterations per query; the fastest of each is compared, to keep runner noise out of the ratio." + echo "Fails above \`${QUERY_REGRESSION}x\` for a single query or \`${TOTAL_REGRESSION}x\` in total." + echo + echo '```' + cat "$RESULTS_DIR/comparison.txt" + echo '```' + } >> "$GITHUB_STEP_SUMMARY" + if [ "$status" -ne 0 ]; then + echo "::error::TPC-H SF1 got slower than the configured limits allow, see the job summary" + fi + exit "$status" + - name: Upload benchmark results + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: tpch-sf1-comparison + path: ${{ runner.temp }}/results + retention-days: 7 diff --git a/benchmarks/README.md b/benchmarks/README.md index f357ff4da58ce..94d9cea241f50 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -257,6 +257,27 @@ git pull Note: if `gh` is installed, you can also run `gh pr checkout $PR_NUMBER` instead of `git fetch upstream pull/$PR_NUMBER/head:pr-$PR_NUMBER` +### In CI + +The `Benchmarks` workflow (`.github/workflows/benchmark.yml`) runs TPC-H SF1 +against the base branch and against the PR merged into it, both on the same +runner, and fails when the PR is slower than the configured limits allow. It is +opt-in, because two release builds plus two benchmark runs take about an hour: + +- add the `performance` label to a PR, or +- start it from the Actions tab (`workflow_dispatch`), where the iteration + count and both regression limits can be overridden + +The comparison table is written to the job summary, and the two result JSON +files plus the table are uploaded as the `tpch-sf1-comparison` artifact. + +Runners are shared machines, so treat the numbers as a signal rather than a +measurement: the defaults (fail above `1.20x` for a single query or `1.05x` in +total, fastest of 5 iterations) are set to catch clear regressions without +flagging noise. Local runs on quiet hardware remain the way to confirm a +result, and a PR that changes the TPC-H benchmark files themselves is measured +against its own queries, which makes the comparison advisory. + ### Running Benchmarks Manually Assuming data is in the `data` directory, the `tpch` benchmark can be run with a command like this: @@ -297,6 +318,17 @@ $ cargo run --release --bin tpch -- benchmark datafusion --iterations 5 --path . ./compare.py /tmp/output_main/tpch.json /tmp/output_branch/tpch.json ``` +To use `compare.py` as a pass/fail gate (this is what CI does), pass one or both +of the regression limits. Without them it only prints the comparison and always +exits successfully. + +```shell +# exit non-zero if a single query is more than 20% slower, or if the total +# time is more than 5% slower +./compare.py /tmp/output_main/tpch.json /tmp/output_branch/tpch.json \ + --fail-threshold 1.20 --fail-total-threshold 1.05 +``` + This will produce output like: ``` diff --git a/benchmarks/compare.py b/benchmarks/compare.py index 9ad1de980abe8..8ccbe471eae74 100755 --- a/benchmarks/compare.py +++ b/benchmarks/compare.py @@ -46,7 +46,9 @@ def load_from(cls, data: Dict[str, Any]) -> QueryResult: @dataclass class QueryRun: - query: int + # A number for benchmarks that identify queries by index, a name such as + # "tpch/Q01/sf1" for the ones run by `benchmark_runner` + query: int | str iterations: List[QueryResult] start_time: int success: bool = True @@ -60,6 +62,13 @@ def load_from(cls, data: Dict[str, Any]) -> QueryRun: success=data.get("success", True), ) + @property + def label(self) -> str: + """Row label: "Q3" for numeric query ids, the id itself for named ones.""" + query = str(self.query) + + return f"Q{query}" if query.isdigit() else query + @property def min_execution_time(self) -> float: assert len(self.iterations) >= 1 @@ -150,7 +159,14 @@ def compare( comparison_path: Path, noise_threshold: float, detailed: bool, -) -> None: + fail_threshold: float | None = None, + fail_total_threshold: float | None = None, +) -> int: + """Print the comparison and return the process exit code. + + The exit code is non-zero only when a `fail_*_threshold` is given and the + comparison run is slower than it allows. + """ baseline = BenchmarkRun.load_from_file(baseline_path) comparison = BenchmarkRun.load_from_file(comparison_path) @@ -172,6 +188,10 @@ def compare( failure_count = 0 total_baseline_time = 0 total_comparison_time = 0 + # (label, comparison / baseline) for every query that ran on both sides, + # and the labels of queries that only the comparison run failed + changes: List[tuple[str, float]] = [] + new_failures: List[str] = [] for baseline_result, comparison_result in zip(baseline.queries, comparison.queries): assert baseline_result.query == comparison_result.query @@ -182,8 +202,10 @@ def compare( if base_failed or comp_failed: change_text = "incomparable" failure_count += 1 + if comp_failed and not base_failed: + new_failures.append(baseline_result.label) table.add_row( - f"Q{baseline_result.query}", + baseline_result.label, "FAIL" if base_failed else baseline_result.execution_time_report(detailed)[1], "FAIL" if comp_failed else comparison_result.execution_time_report(detailed)[1], change_text, @@ -197,6 +219,7 @@ def compare( total_comparison_time += comparison_value change = comparison_value / baseline_value + changes.append((baseline_result.label, change)) if (1.0 - noise_threshold) <= change <= (1.0 + noise_threshold): change_text = "no change" @@ -209,7 +232,7 @@ def compare( slower_count += 1 table.add_row( - f"Q{baseline_result.query}", + baseline_result.label, baseline_text, comparison_text, change_text, @@ -225,6 +248,10 @@ def compare( if len(comparison.queries) - failure_count > 0: avg_comparison_time = total_comparison_time / (len(comparison.queries) - failure_count) + total_change = ( + total_comparison_time / total_baseline_time if total_baseline_time else 1.0 + ) + # Summary table summary_table = Table(show_header=True, header_style="bold magenta") summary_table.add_column("Benchmark Summary", justify="left", style="dim") @@ -234,6 +261,7 @@ def compare( summary_table.add_row(f"Total Time ({comparison_header})", f"{total_comparison_time:.2f}ms") summary_table.add_row(f"Average Time ({baseline_header})", f"{avg_baseline_time:.2f}ms") summary_table.add_row(f"Average Time ({comparison_header})", f"{avg_comparison_time:.2f}ms") + summary_table.add_row("Total Change", f"{total_change:.2f}x") summary_table.add_row("Queries Faster", str(faster_count)) summary_table.add_row("Queries Slower", str(slower_count)) summary_table.add_row("Queries with No Change", str(no_change_count)) @@ -241,6 +269,68 @@ def compare( console.print(summary_table) + return report_regressions( + console, + baseline_header, + comparison_header, + changes, + new_failures, + total_change, + fail_threshold, + fail_total_threshold, + ) + + +def report_regressions( + console: Console, + baseline_header: str, + comparison_header: str, + changes: List[tuple[str, float]], + new_failures: List[str], + total_change: float, + fail_threshold: float | None, + fail_total_threshold: float | None, +) -> int: + """Report against the configured limits, returning the process exit code.""" + if fail_threshold is None and fail_total_threshold is None: + return 0 + + problems: List[str] = [] + + if fail_threshold is not None: + problems.extend( + f"{label} is {change:.2f}x slower (limit {fail_threshold:.2f}x)" + for label, change in changes + if change > fail_threshold + ) + + if fail_total_threshold is not None and total_change > fail_total_threshold: + problems.append( + f"total time is {total_change:.2f}x slower " + f"(limit {fail_total_threshold:.2f}x)" + ) + + # A query that only fails on one side is excluded from the timings above, + # so it would otherwise pass the gate unnoticed. + problems.extend( + f"{label} failed in {comparison_header} but not in {baseline_header}" + for label in new_failures + ) + + if not problems: + console.print( + f"No regression: {comparison_header} is within the configured limits " + f"of {baseline_header}." + ) + return 0 + + console.print(f"Regression: {comparison_header} is slower than {baseline_header}") + for problem in problems: + console.print(f" - {problem}") + + return 1 + + def main() -> None: parser = ArgumentParser() compare_parser = parser @@ -260,6 +350,20 @@ def main() -> None: default=0.05, help="The threshold for statistically insignificant results (+/- %5).", ) + compare_parser.add_argument( + "--fail-threshold", + type=float, + default=None, + help="Exit non-zero if any single query is slower than this ratio " + "(e.g. 1.2 for 20%% slower). Off by default.", + ) + compare_parser.add_argument( + "--fail-total-threshold", + type=float, + default=None, + help="Exit non-zero if the total time is slower than this ratio " + "(e.g. 1.05 for 5%% slower). Off by default.", + ) compare_parser.add_argument( "--detailed", action=argparse.BooleanOptionalAction, @@ -269,7 +373,16 @@ def main() -> None: options = parser.parse_args() - compare(options.baseline_path, options.comparison_path, options.noise_threshold, options.detailed) + raise SystemExit( + compare( + options.baseline_path, + options.comparison_path, + options.noise_threshold, + options.detailed, + options.fail_threshold, + options.fail_total_threshold, + ) + ) From 12aec6cc1d85f7ebbf5a3788ca34b8a297b4f424 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Heres?= Date: Fri, 21 Aug 2026 15:23:56 +0200 Subject: [PATCH 02/10] ci: install tpchgen-cli via taiki-e/install-action `cargo install` in a workflow is rejected by ci/scripts/check_no_cargo_install_in_workflows.sh. Pin 3.0.0 so the generated data stays fixed. Co-Authored-By: Claude Opus 5 --- .github/workflows/benchmark.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index c4d47ffe35d1e..286c89458b10b 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -93,7 +93,10 @@ jobs: - name: Install uv uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0 - name: Install tpchgen-cli - run: cargo install tpchgen-cli --locked + uses: taiki-e/install-action@7f4eb899022d8fe70b20c4f3de697aa85c309026 # v2.85.11 + with: + # pinned, so the generated data does not change under the benchmark + tool: tpchgen-cli@3.0.0 - name: Resolve base commit id: base env: From 2938436318b0eb12e24f58c40af27dc449f170f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Heres?= Date: Fri, 21 Aug 2026 15:28:38 +0200 Subject: [PATCH 03/10] ci: build both benchmark runners concurrently, with LTO off The two builds dominate this job. `release-nonlto` is `release` with `lto = false` and 16 codegen units, and fat LTO with a single codegen unit roughly doubles the build; both sides are built identically, so the ratio the gate looks at is unaffected. `workflow_dispatch` can still pick `release` for numbers comparable with locally posted bench.sh results. The builds also run concurrently now, since neither keeps every core busy on its own. They keep a target directory each, because cargo locks one exclusively and sharing it would serialize them; their logs are captured and replayed in groups so the two do not interleave line by line. Co-Authored-By: Claude Opus 5 --- .github/workflows/benchmark.yml | 64 +++++++++++++++++++++++++-------- benchmarks/README.md | 12 +++++-- 2 files changed, 59 insertions(+), 17 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 286c89458b10b..07f6c9042acfb 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -46,6 +46,13 @@ on: description: 'Fail if the total time is more than this much slower' type: string default: '1.05' + profile: + description: 'Cargo profile to build both sides with' + type: choice + options: + - release-nonlto + - release + default: release-nonlto permissions: contents: read @@ -56,10 +63,17 @@ jobs: if: github.event_name == 'workflow_dispatch' || contains(github.event.pull_request.labels.*.name, 'performance') # A single job, so both binaries are measured on the same machine. runs-on: ${{ vars.USE_RUNS_ON == 'true' && format('runs-on={0},family=c8a+m8a,cpu=16,image=ubuntu24-full-x64,extras=s3-cache,disk=large,tag=datafusion', github.run_id) || 'ubuntu-latest' }} - # note: no rust container, so the two builds get plain `--release` - # binaries, comparable with what `benchmarks/bench.sh` produces locally + # note: no rust container, so both binaries are built the way a + # developer's `benchmarks/bench.sh` run builds them timeout-minutes: 150 env: + # `release-nonlto` is `release` with `lto = false` and 16 codegen units. + # Building is what dominates this job, and fat LTO with one codegen unit + # roughly doubles it; both sides are built identically, so the ratio the + # gate looks at still holds. Dispatch with `release` when a change is + # expected to interact with cross-crate inlining, or to get numbers + # comparable with locally posted `bench.sh` results. + CARGO_PROFILE: ${{ inputs.profile || 'release-nonlto' }} ITERATIONS: ${{ inputs.iterations || '5' }} QUERY_REGRESSION: ${{ inputs.query_regression || '1.20' }} TOTAL_REGRESSION: ${{ inputs.total_regression || '1.05' }} @@ -131,24 +145,43 @@ jobs: --parquet-compression 'ZSTD(1)' \ --parts=1 \ --output-dir "$DATA_DIR/tpch_sf1" - # Separate target directories: the two source trees share crate names and - # versions, so one target directory would make each build evict the other. - - name: Build base benchmark runner - working-directory: ${{ runner.temp }}/base - env: - CARGO_TARGET_DIR: ${{ runner.temp }}/target-base - run: cargo build --release -p datafusion-benchmarks --bin benchmark_runner - - name: Build PR benchmark runner - env: - CARGO_TARGET_DIR: ${{ runner.temp }}/target-pr - run: cargo build --release -p datafusion-benchmarks --bin benchmark_runner + - name: Build both benchmark runners + # Concurrently, since neither build keeps every core busy on its own. + # They need a target directory each: cargo locks one exclusively, so + # sharing it would serialize the two. + run: | + build() { + (cd "$1" && CARGO_TARGET_DIR="$2" cargo build \ + --profile "$CARGO_PROFILE" \ + -p datafusion-benchmarks \ + --bin benchmark_runner) > "$3" 2>&1 + } + build "$RUNNER_TEMP/base" "$RUNNER_TEMP/target-base" "$RUNNER_TEMP/build-base.log" & + base_pid=$! + build "$GITHUB_WORKSPACE" "$RUNNER_TEMP/target-pr" "$RUNNER_TEMP/build-pr.log" & + pr_pid=$! + base_status=0 + wait "$base_pid" || base_status=$? + pr_status=0 + wait "$pr_pid" || pr_status=$? + # The two logs would be interleaved line by line without this. + echo "::group::base build" + cat "$RUNNER_TEMP/build-base.log" + echo "::endgroup::" + echo "::group::PR build" + cat "$RUNNER_TEMP/build-pr.log" + echo "::endgroup::" + if [ "$base_status" -ne 0 ] || [ "$pr_status" -ne 0 ]; then + echo "::error::benchmark_runner did not build (base: $base_status, PR: $pr_status)" + exit 1 + fi # Each runner reads the queries of the tree it was built from (they are # resolved relative to its own `benchmarks` directory), and both read the # one generated dataset. - name: Benchmark base working-directory: ${{ runner.temp }}/base/benchmarks run: | - "$RUNNER_TEMP/target-base/release/benchmark_runner" tpch \ + "$RUNNER_TEMP/target-base/$CARGO_PROFILE/benchmark_runner" tpch \ --scale-factor 1 \ --format parquet \ --iterations "$ITERATIONS" \ @@ -157,7 +190,7 @@ jobs: - name: Benchmark PR working-directory: benchmarks run: | - "$RUNNER_TEMP/target-pr/release/benchmark_runner" tpch \ + "$RUNNER_TEMP/target-pr/$CARGO_PROFILE/benchmark_runner" tpch \ --scale-factor 1 \ --format parquet \ --iterations "$ITERATIONS" \ @@ -179,6 +212,7 @@ jobs: echo echo "\`$ITERATIONS\` iterations per query; the fastest of each is compared, to keep runner noise out of the ratio." echo "Fails above \`${QUERY_REGRESSION}x\` for a single query or \`${TOTAL_REGRESSION}x\` in total." + echo "Both sides built with the \`$CARGO_PROFILE\` profile." echo echo '```' cat "$RESULTS_DIR/comparison.txt" diff --git a/benchmarks/README.md b/benchmarks/README.md index 94d9cea241f50..129263310170d 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -262,11 +262,19 @@ Note: if `gh` is installed, you can also run `gh pr checkout $PR_NUMBER` instead The `Benchmarks` workflow (`.github/workflows/benchmark.yml`) runs TPC-H SF1 against the base branch and against the PR merged into it, both on the same runner, and fails when the PR is slower than the configured limits allow. It is -opt-in, because two release builds plus two benchmark runs take about an hour: +opt-in, because two builds plus two benchmark runs still take a good half hour: - add the `performance` label to a PR, or - start it from the Actions tab (`workflow_dispatch`), where the iteration - count and both regression limits can be overridden + count, both regression limits, and the cargo profile can be overridden + +The two binaries are built concurrently, with the `release-nonlto` profile +(`release`, but with `lto = false` and 16 codegen units), which is what makes +that half hour possible -- fat LTO with a single codegen unit roughly doubles +the build. Both sides are built identically, so the ratio the gate looks at +still holds. Dispatch the workflow with the `release` profile when a change is +expected to interact with cross-crate inlining, or when the absolute numbers +should line up with locally posted `bench.sh` results. The comparison table is written to the job summary, and the two result JSON files plus the table are uploaded as the `tpch-sf1-comparison` artifact. From 72deefea884a37cd487b345f17ce13fd1bc4b7a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Heres?= Date: Fri, 21 Aug 2026 17:17:19 +0200 Subject: [PATCH 04/10] ci: build the two benchmark runners on a runner each Backgrounding both builds in one job shared 16 cores between them. Split the workflow into three jobs instead: one resolves the base commit, two build a `benchmark_runner` each on their own 32-core runner, and the last one measures both binaries back to back on one machine -- which is the part that has to stay on a single machine for the timings to be comparable. Passing a binary between jobs needs care: `benchmark_runner` finds `sql_benchmarks` through the CARGO_MANIFEST_DIR baked in at compile time, so each side's tree is checked out at the same fixed path in the job that builds it and in the job that runs it, and the benchmark job verifies that each binary still sees the tpch suite before measuring anything. Co-Authored-By: Claude Opus 5 --- .github/workflows/benchmark.yml | 256 ++++++++++++++++++++------------ benchmarks/README.md | 25 +++- 2 files changed, 182 insertions(+), 99 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 07f6c9042acfb..77528dfa109b3 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -15,10 +15,13 @@ # specific language governing permissions and limitations # under the License. -# Catches performance regressions by running TPC-H SF1 twice on the same -# machine: once for the base branch, once for the PR merged into it. Only the -# ratio between the two runs is used, so the (shared, virtualized) runner does -# not have to be fast -- just consistent for the duration of the job. +# Catches performance regressions by running TPC-H SF1 for the base branch and +# for the PR merged into it, then failing when the PR is slower than the +# configured limits allow. +# +# The two binaries are built by two jobs, on a runner each, and both are then +# measured by a third job on one machine: building is embarrassingly parallel, +# while comparing timings taken on different machines is meaningless. name: Benchmarks @@ -26,9 +29,9 @@ concurrency: group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} cancel-in-progress: true -# Two release builds plus two benchmark runs take about an hour, and benchmarks -# on shared runners are noisy, so this is opt-in: add the `performance` label to -# a PR, or start it by hand from the Actions tab. +# The builds and the benchmark runs add up to roughly half an hour of runner +# time, and benchmarks on shared runners are noisy, so this is opt-in: add the +# `performance` label to a PR, or start it by hand from the Actions tab. on: pull_request: types: [opened, synchronize, reopened, labeled] @@ -57,52 +60,173 @@ on: permissions: contents: read +env: + # `release-nonlto` is `release` with `lto = false` and 16 codegen units. Fat + # LTO with a single codegen unit roughly doubles the build, and both sides are + # built identically, so the ratio the gate looks at still holds. Dispatch with + # `release` when a change is expected to interact with cross-crate inlining, + # or to get numbers comparable with locally posted `bench.sh` results. + CARGO_PROFILE: ${{ inputs.profile || 'release-nonlto' }} + ITERATIONS: ${{ inputs.iterations || '5' }} + QUERY_REGRESSION: ${{ inputs.query_regression || '1.20' }} + TOTAL_REGRESSION: ${{ inputs.total_regression || '1.05' }} + # `benchmark_runner` finds `sql_benchmarks` through the CARGO_MANIFEST_DIR + # baked into it at compile time, so a binary built in one job only works in + # another if its tree sits at the same absolute path there. Every job below + # puts the two trees under this root, which is why it is a fixed path rather + # than something derived from the workspace or the runner. + BENCH_ROOT: /tmp/df-bench + # Same cargo network settings as .github/actions/setup-rust-runtime, without + # its RUSTFLAGS: benchmark binaries are built with the defaults. + CARGO_HTTP_MULTIPLEXING: "false" + CARGO_NET_RETRY: "10" + CARGO_HTTP_RETRY: "10" + jobs: - tpch-sf1: - name: TPC-H SF1 (PR vs base) + # Resolved once, so the three jobs below cannot disagree about what "base" is. + resolve: + name: resolve base commit if: github.event_name == 'workflow_dispatch' || contains(github.event.pull_request.labels.*.name, 'performance') - # A single job, so both binaries are measured on the same machine. - runs-on: ${{ vars.USE_RUNS_ON == 'true' && format('runs-on={0},family=c8a+m8a,cpu=16,image=ubuntu24-full-x64,extras=s3-cache,disk=large,tag=datafusion', github.run_id) || 'ubuntu-latest' }} - # note: no rust container, so both binaries are built the way a - # developer's `benchmarks/bench.sh` run builds them - timeout-minutes: 150 - env: - # `release-nonlto` is `release` with `lto = false` and 16 codegen units. - # Building is what dominates this job, and fat LTO with one codegen unit - # roughly doubles it; both sides are built identically, so the ratio the - # gate looks at still holds. Dispatch with `release` when a change is - # expected to interact with cross-crate inlining, or to get numbers - # comparable with locally posted `bench.sh` results. - CARGO_PROFILE: ${{ inputs.profile || 'release-nonlto' }} - ITERATIONS: ${{ inputs.iterations || '5' }} - QUERY_REGRESSION: ${{ inputs.query_regression || '1.20' }} - TOTAL_REGRESSION: ${{ inputs.total_regression || '1.05' }} - # Same cargo network settings as .github/actions/setup-rust-runtime, - # without its RUSTFLAGS: benchmark binaries are built with the defaults. - CARGO_HTTP_MULTIPLEXING: "false" - CARGO_NET_RETRY: "10" - CARGO_HTTP_RETRY: "10" + runs-on: ${{ vars.USE_RUNS_ON == 'true' && format('runs-on={0},family=c8a+m8a,cpu=2,image=ubuntu24-full-x64,extras=s3-cache,tag=datafusion', github.run_id) || 'ubuntu-latest' }} + timeout-minutes: 15 + outputs: + base_sha: ${{ steps.base.outputs.sha }} steps: - uses: runs-on/action@46910bf61b41721b0579f237e186afb35477007a # v2.3.0 - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: # For `pull_request` this is the PR already merged into the base - # branch. Full history so the base commit can be built as well. - fetch-depth: 0 + # branch; depth 2 is enough to also reach the merge's first parent. + fetch-depth: 2 + - name: Resolve base commit + id: base + env: + BASE_REF: ${{ github.event.pull_request.base.ref || github.event.repository.default_branch }} + run: | + set -euo pipefail + if [ "$(git rev-list --parents -n 1 HEAD | wc -w)" -ge 3 ]; then + # HEAD is the PR merged into the base branch, so its first parent + # is the base branch tip that the merge used -- the commit this PR + # would actually land on. + base_sha=$(git rev-parse "HEAD^1") + else + # Manual run on a branch: compare it as-is against the base tip. + git fetch --no-tags --depth 1 origin "$BASE_REF" + base_sha=$(git rev-parse FETCH_HEAD) + echo "HEAD is not a merge commit, comparing it as-is against $BASE_REF" + fi + echo "sha=${base_sha}" >> "$GITHUB_OUTPUT" + echo "base: ${base_sha} $(git log -1 --format=%s "${base_sha}")" + echo "candidate: $(git rev-parse HEAD) $(git log -1 --format=%s HEAD)" + + # One runner per side, so the two builds really do run at the same time. + build: + name: build ${{ matrix.side }} runner + needs: resolve + runs-on: ${{ vars.USE_RUNS_ON == 'true' && format('runs-on={0},family=c8a+m8a,cpu=32,image=ubuntu24-full-x64,extras=s3-cache,disk=large,tag=datafusion', github.run_id) || 'ubuntu-latest' }} + timeout-minutes: 90 + strategy: + # No point in building one side if the other one is broken. + fail-fast: true + matrix: + side: [base, pr] + steps: + - uses: runs-on/action@46910bf61b41721b0579f237e186afb35477007a # v2.3.0 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 2 - name: Free Disk Space (Ubuntu) uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be # v1.3.1 + - name: Install Rust + run: | + if ! command -v rustup > /dev/null; then + curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --default-toolchain none + echo "$HOME/.cargo/bin" >> "$GITHUB_PATH" + export PATH="$HOME/.cargo/bin:$PATH" + fi + # installs the channel pinned by rust-toolchain.toml + rustup toolchain install + - name: Check out the ${{ matrix.side }} tree + env: + SIDE: ${{ matrix.side }} + BASE_SHA: ${{ needs.resolve.outputs.base_sha }} + run: | + if [ "$SIDE" = "base" ]; then + git worktree add --detach "$BENCH_ROOT/$SIDE" "$BASE_SHA" + else + git worktree add --detach "$BENCH_ROOT/$SIDE" HEAD + fi + - name: Build benchmark_runner + env: + SIDE: ${{ matrix.side }} + CARGO_TARGET_DIR: ${{ runner.temp }}/target + run: | + cd "$BENCH_ROOT/$SIDE" + cargo build --profile "$CARGO_PROFILE" -p datafusion-benchmarks --bin benchmark_runner + cp "$CARGO_TARGET_DIR/$CARGO_PROFILE/benchmark_runner" "$RUNNER_TEMP/benchmark_runner" + - name: Upload benchmark_runner + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: benchmark_runner-${{ matrix.side }} + path: ${{ runner.temp }}/benchmark_runner + retention-days: 1 + + # Both sides are measured here, on this one machine, back to back. + benchmark: + name: TPC-H SF1 (PR vs base) + needs: [resolve, build] + runs-on: ${{ vars.USE_RUNS_ON == 'true' && format('runs-on={0},family=c8a+m8a,cpu=16,image=ubuntu24-full-x64,extras=s3-cache,disk=large,tag=datafusion', github.run_id) || 'ubuntu-latest' }} + timeout-minutes: 90 + steps: + - uses: runs-on/action@46910bf61b41721b0579f237e186afb35477007a # v2.3.0 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 2 - name: Configure paths run: | echo "DATA_DIR=$RUNNER_TEMP/bench-data" >> "$GITHUB_ENV" echo "RESULTS_DIR=$RUNNER_TEMP/results" >> "$GITHUB_ENV" + - name: Check out both trees + env: + BASE_SHA: ${{ needs.resolve.outputs.base_sha }} + run: | + # The same paths the build jobs used, so each binary finds the + # `sql_benchmarks` directory of the tree it was built from. + git worktree add --detach "$BENCH_ROOT/base" "$BASE_SHA" + git worktree add --detach "$BENCH_ROOT/pr" HEAD + - name: Download base benchmark_runner + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: benchmark_runner-base + path: ${{ runner.temp }}/bin/base + - name: Download PR benchmark_runner + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: benchmark_runner-pr + path: ${{ runner.temp }}/bin/pr + - name: Check both runners see their queries + run: | + # Artifacts do not carry the executable bit, and a binary whose tree + # is missing at the path baked into it would discover no benchmark at + # all -- fail here rather than three steps later. + for side in base pr; do + binary="$RUNNER_TEMP/bin/$side/benchmark_runner" + chmod +x "$binary" + if ! "$binary" --list | grep -qE '^[[:space:]]+tpch[[:space:]]'; then + echo "::error::the $side runner does not see the tpch suite at $BENCH_ROOT/$side/benchmarks" + "$binary" --list + exit 1 + fi + done - name: Install Rust + # Only so tpchgen-cli can be built from source if no prebuilt binary + # matches this runner; nothing here is compiled otherwise. run: | if ! command -v rustup > /dev/null; then curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --default-toolchain none echo "$HOME/.cargo/bin" >> "$GITHUB_PATH" export PATH="$HOME/.cargo/bin:$PATH" fi - # installs the channel pinned by rust-toolchain.toml rustup toolchain install - name: Install uv uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0 @@ -111,28 +235,6 @@ jobs: with: # pinned, so the generated data does not change under the benchmark tool: tpchgen-cli@3.0.0 - - name: Resolve base commit - id: base - env: - BASE_REF: ${{ github.event.pull_request.base.ref || github.event.repository.default_branch }} - run: | - set -euo pipefail - if [ "$(git rev-list --parents -n 1 HEAD | wc -w)" -ge 3 ]; then - # HEAD is the PR merged into the base branch, so its first parent - # is the base branch tip that the merge used -- the commit this PR - # would actually land on. - base_sha=$(git rev-parse "HEAD^1") - else - # Manual run on a branch: compare it as-is against the base tip. - git fetch --no-tags origin "$BASE_REF" - base_sha=$(git rev-parse FETCH_HEAD) - echo "HEAD is not a merge commit, comparing it as-is against $BASE_REF" - fi - echo "sha=${base_sha}" >> "$GITHUB_OUTPUT" - echo "base: ${base_sha} $(git log -1 --format=%s "${base_sha}")" - echo "candidate: $(git rev-parse HEAD) $(git log -1 --format=%s HEAD)" - - name: Check out base commit - run: git worktree add --detach "$RUNNER_TEMP/base" "${{ steps.base.outputs.sha }}" - name: Generate TPC-H SF1 data # Same generator settings as `bench.sh data tpch`, inlined because that # path also downloads the expected answers through `docker run -it`, @@ -145,52 +247,20 @@ jobs: --parquet-compression 'ZSTD(1)' \ --parts=1 \ --output-dir "$DATA_DIR/tpch_sf1" - - name: Build both benchmark runners - # Concurrently, since neither build keeps every core busy on its own. - # They need a target directory each: cargo locks one exclusively, so - # sharing it would serialize the two. - run: | - build() { - (cd "$1" && CARGO_TARGET_DIR="$2" cargo build \ - --profile "$CARGO_PROFILE" \ - -p datafusion-benchmarks \ - --bin benchmark_runner) > "$3" 2>&1 - } - build "$RUNNER_TEMP/base" "$RUNNER_TEMP/target-base" "$RUNNER_TEMP/build-base.log" & - base_pid=$! - build "$GITHUB_WORKSPACE" "$RUNNER_TEMP/target-pr" "$RUNNER_TEMP/build-pr.log" & - pr_pid=$! - base_status=0 - wait "$base_pid" || base_status=$? - pr_status=0 - wait "$pr_pid" || pr_status=$? - # The two logs would be interleaved line by line without this. - echo "::group::base build" - cat "$RUNNER_TEMP/build-base.log" - echo "::endgroup::" - echo "::group::PR build" - cat "$RUNNER_TEMP/build-pr.log" - echo "::endgroup::" - if [ "$base_status" -ne 0 ] || [ "$pr_status" -ne 0 ]; then - echo "::error::benchmark_runner did not build (base: $base_status, PR: $pr_status)" - exit 1 - fi - # Each runner reads the queries of the tree it was built from (they are - # resolved relative to its own `benchmarks` directory), and both read the - # one generated dataset. + # Each side runs from its own tree and reads the one generated dataset. - name: Benchmark base - working-directory: ${{ runner.temp }}/base/benchmarks run: | - "$RUNNER_TEMP/target-base/$CARGO_PROFILE/benchmark_runner" tpch \ + cd "$BENCH_ROOT/base/benchmarks" + "$RUNNER_TEMP/bin/base/benchmark_runner" tpch \ --scale-factor 1 \ --format parquet \ --iterations "$ITERATIONS" \ --path "$DATA_DIR" \ --output "$RESULTS_DIR/base/tpch_sf1.json" - name: Benchmark PR - working-directory: benchmarks run: | - "$RUNNER_TEMP/target-pr/$CARGO_PROFILE/benchmark_runner" tpch \ + cd "$BENCH_ROOT/pr/benchmarks" + "$RUNNER_TEMP/bin/pr/benchmark_runner" tpch \ --scale-factor 1 \ --format parquet \ --iterations "$ITERATIONS" \ @@ -208,7 +278,7 @@ jobs: > "$RESULTS_DIR/comparison.txt" 2>&1 || status=$? cat "$RESULTS_DIR/comparison.txt" { - echo "### TPC-H SF1: base (${{ steps.base.outputs.sha }}) vs PR" + echo "### TPC-H SF1: base (${{ needs.resolve.outputs.base_sha }}) vs PR" echo echo "\`$ITERATIONS\` iterations per query; the fastest of each is compared, to keep runner noise out of the ratio." echo "Fails above \`${QUERY_REGRESSION}x\` for a single query or \`${TOTAL_REGRESSION}x\` in total." diff --git a/benchmarks/README.md b/benchmarks/README.md index 129263310170d..e54060865c869 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -268,14 +268,27 @@ opt-in, because two builds plus two benchmark runs still take a good half hour: - start it from the Actions tab (`workflow_dispatch`), where the iteration count, both regression limits, and the cargo profile can be overridden -The two binaries are built concurrently, with the `release-nonlto` profile -(`release`, but with `lto = false` and 16 codegen units), which is what makes -that half hour possible -- fat LTO with a single codegen unit roughly doubles -the build. Both sides are built identically, so the ratio the gate looks at -still holds. Dispatch the workflow with the `release` profile when a change is -expected to interact with cross-crate inlining, or when the absolute numbers +The workflow is three jobs: one resolves the base commit, two build a +`benchmark_runner` each (one runner per side, so the builds really are +simultaneous), and the last one measures both binaries back to back on a single +machine, because timings taken on two different machines cannot be compared. + +Both sides are built with the `release-nonlto` profile (`release`, but with +`lto = false` and 16 codegen units), which together with the split is what +keeps this to half an hour -- fat LTO with a single codegen unit roughly +doubles a build. Both sides are built identically, so the ratio the gate looks +at still holds. Dispatch the workflow with the `release` profile when a change +is expected to interact with cross-crate inlining, or when the absolute numbers should line up with locally posted `bench.sh` results. +Shipping a binary between jobs has one catch worth knowing about if you edit +the workflow: `benchmark_runner` locates `sql_benchmarks` through the +`CARGO_MANIFEST_DIR` baked into it at compile time, so each side's checkout has +to sit at the same absolute path in the job that builds it and in the job that +runs it. The workflow puts both under a fixed `/tmp` root for that reason, and +checks that each binary can still see the `tpch` suite before it starts +measuring. + The comparison table is written to the job summary, and the two result JSON files plus the table are uploaded as the `tpch-sf1-comparison` artifact. From a75c4174ad6d99828147ab7b7fd91a4a086cc5ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Heres?= Date: Fri, 21 Aug 2026 17:44:14 +0200 Subject: [PATCH 05/10] ci: benchmark TPC-H SF10, and install tpchgen-cli from its wheel SF1 runs the whole 22-query suite in ~6 seconds, so per-query timings sit in the range where runner noise is a large part of the measurement. SF10 gives the gate something to measure; the scale factor is a dispatch input, so SF1 is still one click away. tpchgen-cli now comes from its PyPI wheel: same 3.0.0 release as the crate, but a ~4MB download rather than a build from source -- the project attaches no binaries to its GitHub releases, so nothing could fetch a prebuilt one. That also drops the Rust toolchain from the benchmark job, which no longer compiles anything at all. Co-Authored-By: Claude Opus 5 --- .github/workflows/benchmark.yml | 65 +++++++++++++++++---------------- benchmarks/README.md | 19 +++++++--- 2 files changed, 46 insertions(+), 38 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 77528dfa109b3..e8ffebaf82593 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -15,9 +15,10 @@ # specific language governing permissions and limitations # under the License. -# Catches performance regressions by running TPC-H SF1 for the base branch and +# Catches performance regressions by running TPC-H SF10 for the base branch and # for the PR merged into it, then failing when the PR is slower than the -# configured limits allow. +# configured limits allow. SF10 rather than SF1 because SF1 queries finish in +# milliseconds, where runner noise is a large fraction of the measurement. # # The two binaries are built by two jobs, on a runner each, and both are then # measured by a third job on one machine: building is embarrassingly parallel, @@ -49,6 +50,13 @@ on: description: 'Fail if the total time is more than this much slower' type: string default: '1.05' + scale_factor: + description: 'TPC-H scale factor' + type: choice + options: + - '10' + - '1' + default: '10' profile: description: 'Cargo profile to build both sides with' type: choice @@ -67,6 +75,7 @@ env: # `release` when a change is expected to interact with cross-crate inlining, # or to get numbers comparable with locally posted `bench.sh` results. CARGO_PROFILE: ${{ inputs.profile || 'release-nonlto' }} + SCALE_FACTOR: ${{ inputs.scale_factor || '10' }} ITERATIONS: ${{ inputs.iterations || '5' }} QUERY_REGRESSION: ${{ inputs.query_regression || '1.20' }} TOTAL_REGRESSION: ${{ inputs.total_regression || '1.05' }} @@ -173,7 +182,7 @@ jobs: # Both sides are measured here, on this one machine, back to back. benchmark: - name: TPC-H SF1 (PR vs base) + name: TPC-H (PR vs base) needs: [resolve, build] runs-on: ${{ vars.USE_RUNS_ON == 'true' && format('runs-on={0},family=c8a+m8a,cpu=16,image=ubuntu24-full-x64,extras=s3-cache,disk=large,tag=datafusion', github.run_id) || 'ubuntu-latest' }} timeout-minutes: 90 @@ -218,67 +227,59 @@ jobs: exit 1 fi done - - name: Install Rust - # Only so tpchgen-cli can be built from source if no prebuilt binary - # matches this runner; nothing here is compiled otherwise. - run: | - if ! command -v rustup > /dev/null; then - curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --default-toolchain none - echo "$HOME/.cargo/bin" >> "$GITHUB_PATH" - export PATH="$HOME/.cargo/bin:$PATH" - fi - rustup toolchain install - name: Install uv uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0 - - name: Install tpchgen-cli - uses: taiki-e/install-action@7f4eb899022d8fe70b20c4f3de697aa85c309026 # v2.85.11 - with: - # pinned, so the generated data does not change under the benchmark - tool: tpchgen-cli@3.0.0 - - name: Generate TPC-H SF1 data + - name: Generate TPC-H data # Same generator settings as `bench.sh data tpch`, inlined because that # path also downloads the expected answers through `docker run -it`, # which needs a TTY and is only used for result validation. + # + # tpchgen-cli comes from its PyPI wheel: the same 3.0.0 release as the + # crate, but a ~4MB download instead of a build from source, since the + # project attaches no binaries to its GitHub releases for a prebuilt + # fetch to find. It also means this job needs no Rust toolchain at all. run: | - mkdir -p "$DATA_DIR/tpch_sf1" "$RESULTS_DIR/base" "$RESULTS_DIR/pr" - tpchgen-cli \ - --scale-factor 1 \ + mkdir -p "$DATA_DIR/tpch_sf$SCALE_FACTOR" "$RESULTS_DIR/base" "$RESULTS_DIR/pr" + uv tool run --from 'tpchgen-cli==3.0.0' tpchgen-cli \ + --scale-factor "$SCALE_FACTOR" \ --format parquet \ --parquet-compression 'ZSTD(1)' \ --parts=1 \ - --output-dir "$DATA_DIR/tpch_sf1" + --output-dir "$DATA_DIR/tpch_sf$SCALE_FACTOR" + du -sh "$DATA_DIR/tpch_sf$SCALE_FACTOR" + df -h "$DATA_DIR" # Each side runs from its own tree and reads the one generated dataset. - name: Benchmark base run: | cd "$BENCH_ROOT/base/benchmarks" "$RUNNER_TEMP/bin/base/benchmark_runner" tpch \ - --scale-factor 1 \ + --scale-factor "$SCALE_FACTOR" \ --format parquet \ --iterations "$ITERATIONS" \ --path "$DATA_DIR" \ - --output "$RESULTS_DIR/base/tpch_sf1.json" + --output "$RESULTS_DIR/base/tpch_sf$SCALE_FACTOR.json" - name: Benchmark PR run: | cd "$BENCH_ROOT/pr/benchmarks" "$RUNNER_TEMP/bin/pr/benchmark_runner" tpch \ - --scale-factor 1 \ + --scale-factor "$SCALE_FACTOR" \ --format parquet \ --iterations "$ITERATIONS" \ --path "$DATA_DIR" \ - --output "$RESULTS_DIR/pr/tpch_sf1.json" + --output "$RESULTS_DIR/pr/tpch_sf$SCALE_FACTOR.json" - name: Compare run: | set -uo pipefail status=0 uv run --no-project --with rich python3 benchmarks/compare.py \ - "$RESULTS_DIR/base/tpch_sf1.json" \ - "$RESULTS_DIR/pr/tpch_sf1.json" \ + "$RESULTS_DIR/base/tpch_sf$SCALE_FACTOR.json" \ + "$RESULTS_DIR/pr/tpch_sf$SCALE_FACTOR.json" \ --fail-threshold "$QUERY_REGRESSION" \ --fail-total-threshold "$TOTAL_REGRESSION" \ > "$RESULTS_DIR/comparison.txt" 2>&1 || status=$? cat "$RESULTS_DIR/comparison.txt" { - echo "### TPC-H SF1: base (${{ needs.resolve.outputs.base_sha }}) vs PR" + echo "### TPC-H SF$SCALE_FACTOR: base (${{ needs.resolve.outputs.base_sha }}) vs PR" echo echo "\`$ITERATIONS\` iterations per query; the fastest of each is compared, to keep runner noise out of the ratio." echo "Fails above \`${QUERY_REGRESSION}x\` for a single query or \`${TOTAL_REGRESSION}x\` in total." @@ -289,13 +290,13 @@ jobs: echo '```' } >> "$GITHUB_STEP_SUMMARY" if [ "$status" -ne 0 ]; then - echo "::error::TPC-H SF1 got slower than the configured limits allow, see the job summary" + echo "::error::TPC-H SF$SCALE_FACTOR got slower than the configured limits allow, see the job summary" fi exit "$status" - name: Upload benchmark results if: always() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: - name: tpch-sf1-comparison + name: tpch-comparison path: ${{ runner.temp }}/results retention-days: 7 diff --git a/benchmarks/README.md b/benchmarks/README.md index e54060865c869..5491d074dc9c4 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -259,14 +259,17 @@ Note: if `gh` is installed, you can also run `gh pr checkout $PR_NUMBER` instead ### In CI -The `Benchmarks` workflow (`.github/workflows/benchmark.yml`) runs TPC-H SF1 +The `Benchmarks` workflow (`.github/workflows/benchmark.yml`) runs TPC-H SF10 against the base branch and against the PR merged into it, both on the same -runner, and fails when the PR is slower than the configured limits allow. It is -opt-in, because two builds plus two benchmark runs still take a good half hour: +runner, and fails when the PR is slower than the configured limits allow. SF10 +rather than SF1, because SF1 queries finish in milliseconds, where runner noise +is a large fraction of the measurement. It is opt-in, because the builds plus +two SF10 runs take a good half hour: - add the `performance` label to a PR, or -- start it from the Actions tab (`workflow_dispatch`), where the iteration - count, both regression limits, and the cargo profile can be overridden +- start it from the Actions tab (`workflow_dispatch`), where the scale factor, + iteration count, both regression limits, and the cargo profile can be + overridden The workflow is three jobs: one resolves the base commit, two build a `benchmark_runner` each (one runner per side, so the builds really are @@ -290,7 +293,11 @@ checks that each binary can still see the `tpch` suite before it starts measuring. The comparison table is written to the job summary, and the two result JSON -files plus the table are uploaded as the `tpch-sf1-comparison` artifact. +files plus the table are uploaded as the `tpch-comparison` artifact. + +The data is generated with the same `tpchgen-cli` settings `bench.sh data tpch` +uses, from the tool's PyPI wheel rather than a source build, which keeps a Rust +toolchain out of the benchmark job entirely. Runners are shared machines, so treat the numbers as a signal rather than a measurement: the defaults (fail above `1.20x` for a single query or `1.05x` in From 13cbfd93f4a7a3fa8701f5337d5dbea604a1505f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Heres?= Date: Fri, 21 Aug 2026 19:05:45 +0200 Subject: [PATCH 06/10] ci: interleave the two benchmark sides, and gate on the median round Both CI runs of this workflow so far failed on a PR that changes no Rust at all: Q02 came out 1.22x slower at SF1 and 1.28x at SF10, tight on both sides each time. The two binaries turn out to have byte-identical `.text` layout, so this was never a code difference -- it is the measurement design. Running all of one side and then all of the other lets anything that drifts between the two blocks, a runner that slows down halfway through or a process that drew an unlucky heap, show up as a clean per-query offset on one side only, and a point ratio against a fixed threshold cannot tell that from a regression. So the benchmark job now measures the sides interleaved: a discarded warmup pass, then six rounds of one pass each, alternating which side leads so each pays the first-position cost the same number of times. Every round is a fresh process, which turns per-process luck into round-to-round spread instead of a fixed offset, and six rounds of one iteration cost about what five iterations of one block did. `compare.py` grows the statistics that design makes possible. Either path may now be a directory of per-round summaries, paired with the other side's in sorted filename order, and a query has to clear three bars to fail: - the median of its per-round ratios is above the limit, so one slow pass on either side cannot decide the verdict - the regression costs at least `--fail-min-delta-ms`, which is what stops an SF1 run failing on queries where 1.20x is four milliseconds - it exceeds the spread the base side showed against itself across the rounds, because a query whose own baseline moved 26% between rounds cannot support a 25% verdict Anything that clears the first bar but not the others is printed under "Not counted against the gate" rather than dropped, the table gains per-round and noise columns, and the summary reports the noise floor, the geometric mean of the per-query ratios, and the CPU count -- the last because without the RunsOn variable set this job silently lands on a shared 4-vCPU runner, which is where these numbers came from. Against a noise model fitted to those two runs, the odds that a 22-query run reports at least one regression on unchanged code drop from 78% to 1%, while detection of a real 1.30x regression goes up from 86% to 98%. Co-Authored-By: Claude Opus 5 --- .github/workflows/benchmark.yml | 101 ++++++--- benchmarks/README.md | 66 ++++-- benchmarks/compare.py | 360 +++++++++++++++++++++++++++----- 3 files changed, 437 insertions(+), 90 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index e8ffebaf82593..9dbda48a21bc0 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -23,6 +23,14 @@ # The two binaries are built by two jobs, on a runner each, and both are then # measured by a third job on one machine: building is embarrassingly parallel, # while comparing timings taken on different machines is meaningless. +# +# The two sides are measured interleaved -- a full pass of one, then a full +# pass of the other, several times, alternating which goes first -- rather than +# all of one side and then all of the other. Measuring in two blocks makes any +# drift between them look exactly like a code change: a runner that gets slower +# halfway through, or a process that happened to get an unlucky heap, shifts one +# side only. Interleaving spreads that over both sides and turns it into +# round-to-round spread, which `compare.py` can see and discount. name: Benchmarks @@ -38,14 +46,22 @@ on: types: [opened, synchronize, reopened, labeled] workflow_dispatch: inputs: + rounds: + description: 'Measurement rounds (a full pass of each side per round; keep it even)' + type: string + default: '6' iterations: - description: 'Iterations per query (the fastest one is compared)' + description: 'Iterations per query within a round' type: string - default: '5' + default: '1' query_regression: description: 'Fail if a single query is more than this much slower' type: string default: '1.20' + min_delta_ms: + description: 'Never fail a query that got less than this many ms slower' + type: string + default: '25' total_regression: description: 'Fail if the total time is more than this much slower' type: string @@ -76,9 +92,16 @@ env: # or to get numbers comparable with locally posted `bench.sh` results. CARGO_PROFILE: ${{ inputs.profile || 'release-nonlto' }} SCALE_FACTOR: ${{ inputs.scale_factor || '10' }} - ITERATIONS: ${{ inputs.iterations || '5' }} + # Even, for two reasons: each side then leads the same number of rounds, and + # the median of an even number of ratios averages the two middle rounds + # instead of resting on one. + ROUNDS: ${{ inputs.rounds || '6' }} + ITERATIONS: ${{ inputs.iterations || '1' }} QUERY_REGRESSION: ${{ inputs.query_regression || '1.20' }} TOTAL_REGRESSION: ${{ inputs.total_regression || '1.05' }} + # A 1.20x swing on a query that runs for 20ms is 4ms, which this kind of + # runner cannot resolve. Regressions have to cost real time to count. + MIN_DELTA_MS: ${{ inputs.min_delta_ms || '25' }} # `benchmark_runner` finds `sql_benchmarks` through the CARGO_MANIFEST_DIR # baked into it at compile time, so a binary built in one job only works in # another if its tree sits at the same absolute path there. Every job below @@ -248,42 +271,70 @@ jobs: --output-dir "$DATA_DIR/tpch_sf$SCALE_FACTOR" du -sh "$DATA_DIR/tpch_sf$SCALE_FACTOR" df -h "$DATA_DIR" - # Each side runs from its own tree and reads the one generated dataset. - - name: Benchmark base + - name: Describe the machine + # A comparison is only as good as the machine under it, and the runner + # this lands on is not always the one the `runs-on` line above asks for + # -- without the RunsOn variable set it falls back to a shared 4-vCPU + # `ubuntu-latest`, where the noise floor is several times higher. run: | - cd "$BENCH_ROOT/base/benchmarks" - "$RUNNER_TEMP/bin/base/benchmark_runner" tpch \ - --scale-factor "$SCALE_FACTOR" \ - --format parquet \ - --iterations "$ITERATIONS" \ - --path "$DATA_DIR" \ - --output "$RESULTS_DIR/base/tpch_sf$SCALE_FACTOR.json" - - name: Benchmark PR + echo "cpus: $(nproc)" + lscpu | grep -E '^(Model name|Socket|Core|Thread|CPU\(s\)):' || true + free -h + if [ "$(nproc)" -lt 8 ]; then + echo "::warning::running on $(nproc) CPUs; expect a high noise floor and marginal verdicts" + fi + # Each side runs from its own tree and reads the one generated dataset. + - name: Benchmark both sides, interleaved run: | - cd "$BENCH_ROOT/pr/benchmarks" - "$RUNNER_TEMP/bin/pr/benchmark_runner" tpch \ - --scale-factor "$SCALE_FACTOR" \ - --format parquet \ - --iterations "$ITERATIONS" \ - --path "$DATA_DIR" \ - --output "$RESULTS_DIR/pr/tpch_sf$SCALE_FACTOR.json" + set -euo pipefail + run_side() { + local side="$1" output="$2" + cd "$BENCH_ROOT/$side/benchmarks" + "$RUNNER_TEMP/bin/$side/benchmark_runner" tpch \ + --scale-factor "$SCALE_FACTOR" \ + --format parquet \ + --iterations "$ITERATIONS" \ + --path "$DATA_DIR" \ + --output "$output" + } + # One discarded pass first, so no measured round is the only one + # paying to pull the parquet files into the page cache. It writes + # outside RESULTS_DIR, which is where compare.py looks for rounds. + echo "::group::warmup" + run_side base "$RUNNER_TEMP/warmup.json" + echo "::endgroup::" + for round in $(seq 1 "$ROUNDS"); do + # Alternate which side goes first, so with an even ROUNDS + # whatever the first position costs -- or saves -- is paid by each + # side the same number of times. + if [ $((round % 2)) -eq 1 ]; then order="base pr"; else order="pr base"; fi + for side in $order; do + echo "::group::round $round: $side" + # Zero-padded, because compare.py pairs the two sides' rounds in + # sorted filename order. + run_side "$side" "$(printf '%s/%s/round%02d.json' "$RESULTS_DIR" "$side" "$round")" + echo "::endgroup::" + done + done - name: Compare run: | set -uo pipefail status=0 uv run --no-project --with rich python3 benchmarks/compare.py \ - "$RESULTS_DIR/base/tpch_sf$SCALE_FACTOR.json" \ - "$RESULTS_DIR/pr/tpch_sf$SCALE_FACTOR.json" \ + "$RESULTS_DIR/base" \ + "$RESULTS_DIR/pr" \ --fail-threshold "$QUERY_REGRESSION" \ --fail-total-threshold "$TOTAL_REGRESSION" \ + --fail-min-delta-ms "$MIN_DELTA_MS" \ > "$RESULTS_DIR/comparison.txt" 2>&1 || status=$? cat "$RESULTS_DIR/comparison.txt" { echo "### TPC-H SF$SCALE_FACTOR: base (${{ needs.resolve.outputs.base_sha }}) vs PR" echo - echo "\`$ITERATIONS\` iterations per query; the fastest of each is compared, to keep runner noise out of the ratio." - echo "Fails above \`${QUERY_REGRESSION}x\` for a single query or \`${TOTAL_REGRESSION}x\` in total." - echo "Both sides built with the \`$CARGO_PROFILE\` profile." + echo "\`$ROUNDS\` rounds of \`$ITERATIONS\` iteration(s), the two sides interleaved and their order alternated per round." + echo "A query fails the gate when the median of its per-round ratios is above \`${QUERY_REGRESSION}x\`, the regression costs at least \`${MIN_DELTA_MS}ms\`, and it is larger than the spread the base side showed against itself." + echo "The total time fails above \`${TOTAL_REGRESSION}x\` under the same noise floor." + echo "Both sides built with the \`$CARGO_PROFILE\` profile on \`$(nproc)\` CPUs." echo echo '```' cat "$RESULTS_DIR/comparison.txt" diff --git a/benchmarks/README.md b/benchmarks/README.md index 5491d074dc9c4..2fc4d52dc5cbf 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -268,13 +268,23 @@ two SF10 runs take a good half hour: - add the `performance` label to a PR, or - start it from the Actions tab (`workflow_dispatch`), where the scale factor, - iteration count, both regression limits, and the cargo profile can be - overridden + round and iteration counts, all three regression limits, and the cargo + profile can be overridden The workflow is three jobs: one resolves the base commit, two build a `benchmark_runner` each (one runner per side, so the builds really are -simultaneous), and the last one measures both binaries back to back on a single -machine, because timings taken on two different machines cannot be compared. +simultaneous), and the last one measures both binaries on a single machine, +because timings taken on two different machines cannot be compared. + +That last job measures the two sides **interleaved**: a full pass of one side, +then a full pass of the other, six times, alternating which side goes first. +This matters more than any choice of statistic. Measuring all of one side and +then all of the other makes anything that drifts between the two blocks -- a +runner that slows down halfway through, a process that drew an unlucky heap -- +look exactly like a code change, because it moves one side only. Interleaving +turns it into round-to-round spread instead, which `compare.py` can see and +report. The round count is kept even so each side leads the same number of +times; dispatch with more rounds to tighten a marginal verdict. Both sides are built with the `release-nonlto` profile (`release`, but with `lto = false` and 16 codegen units), which together with the split is what @@ -292,19 +302,34 @@ runs it. The workflow puts both under a fixed `/tmp` root for that reason, and checks that each binary can still see the `tpch` suite before it starts measuring. -The comparison table is written to the job summary, and the two result JSON -files plus the table are uploaded as the `tpch-comparison` artifact. +The comparison table is written to the job summary, and every round's result +JSON plus the table are uploaded as the `tpch-comparison` artifact. The data is generated with the same `tpchgen-cli` settings `bench.sh data tpch` uses, from the tool's PyPI wheel rather than a source build, which keeps a Rust toolchain out of the benchmark job entirely. Runners are shared machines, so treat the numbers as a signal rather than a -measurement: the defaults (fail above `1.20x` for a single query or `1.05x` in -total, fastest of 5 iterations) are set to catch clear regressions without -flagging noise. Local runs on quiet hardware remain the way to confirm a -result, and a PR that changes the TPC-H benchmark files themselves is measured -against its own queries, which makes the comparison advisory. +measurement. A query has to clear three separate bars to fail the gate, and +each one is there because it was seen firing on its own: + +- the **median of its per-round ratios** is above `1.20x`. The median, not the + point ratio, so one slow pass on either side cannot decide the verdict. +- the regression costs at least **25ms**. A `1.20x` swing on a query that runs + for 20ms is 4ms, which no shared runner can resolve -- this is what keeps an + SF1 run from failing on its short queries. +- it is larger than the **spread the base side showed against itself** across + the rounds. If the same binary varied by 26% from round to round, a 25% + difference against the other binary is not a finding, and is reported as + inconclusive instead of blamed on the PR. + +The total time is gated at `1.05x` under the same noise floor. Everything that +clears the first bar but not the others is printed under "Not counted against +the gate", so a marginal result is visible rather than silently dropped. + +Local runs on quiet hardware remain the way to confirm a result, and a PR that +changes the TPC-H benchmark files themselves is measured against its own +queries, which makes the comparison advisory. ### Running Benchmarks Manually @@ -351,10 +376,23 @@ of the regression limits. Without them it only prints the comparison and always exits successfully. ```shell -# exit non-zero if a single query is more than 20% slower, or if the total -# time is more than 5% slower +# exit non-zero if a single query is more than 20% slower and at least 25ms +# slower, or if the total time is more than 5% slower ./compare.py /tmp/output_main/tpch.json /tmp/output_branch/tpch.json \ - --fail-threshold 1.20 --fail-total-threshold 1.05 + --fail-threshold 1.20 --fail-total-threshold 1.05 --fail-min-delta-ms 25 +``` + +Either path can also be a *directory* of summary files, one per measurement +round, which is how CI runs it. Rounds are paired between the two sides in +sorted filename order, so name them `round01.json`, `round02.json` and so on. +Given several rounds, the gate rules on the median of the per-round ratios +rather than on a single ratio, and reports how much each side varied against +itself -- see [In CI](#in-ci) for why that is worth the trouble. + +```shell +# five rounds per side, measured alternately rather than one side at a time +./compare.py /tmp/output_main /tmp/output_branch \ + --fail-threshold 1.20 --fail-total-threshold 1.05 --fail-min-delta-ms 25 ``` This will produce output like: diff --git a/benchmarks/compare.py b/benchmarks/compare.py index 8ccbe471eae74..629d841c26185 100755 --- a/benchmarks/compare.py +++ b/benchmarks/compare.py @@ -22,7 +22,7 @@ import json import math from dataclasses import dataclass -from typing import Dict, List, Any +from typing import Dict, List, Any, Sequence from pathlib import Path from argparse import ArgumentParser @@ -34,6 +34,36 @@ raise +def median(values: Sequence[float]) -> float: + """The median of a non-empty sequence.""" + ordered = sorted(values) + middle = len(ordered) // 2 + + if len(ordered) % 2 == 1: + return ordered[middle] + + return (ordered[middle - 1] + ordered[middle]) / 2 + + +def geometric_mean(values: Sequence[float]) -> float: + """The geometric mean of a non-empty sequence of positive numbers.""" + return math.exp(sum(math.log(value) for value in values) / len(values)) + + +def upward_spread(values: Sequence[float]) -> float: + """How far the typical measurement sits above the fastest one. + + `(median - min) / min`, the noise floor of a single side: the relative + slowdown that this side already shows against itself. The median rather + than the max, so one hiccup in one round does not define the floor. + """ + fastest = min(values) + if fastest <= 0: + return 0.0 + + return (median(values) - fastest) / fastest + + @dataclass class QueryResult: elapsed: float @@ -154,6 +184,116 @@ def load_from_file(cls, path: Path) -> BenchmarkRun: return cls.load_from(json.load(f)) +@dataclass +class Side: + """One side of the comparison: every round measured for it. + + A single summary file is one round; a directory of summary files is one + round per file, taken in sorted order. Measuring several rounds and + interleaving them with the other side is what makes the comparison + trustworthy: a machine that slows down halfway through then shifts both + sides, instead of only the one that happened to run at that moment. + """ + + header: str + rounds: List[BenchmarkRun] + + @classmethod + def load(cls, path: Path) -> Side: + if path.is_dir(): + paths = sorted(path.glob("*.json")) + if not paths: + raise SystemExit(f"no *.json summary files in {path}") + # A directory is named after the side it holds ("base", "pr"), + # a file is named after the run and lives in such a directory. + header = path.name + else: + paths = [path] + header = path.parent.name + + rounds = [BenchmarkRun.load_from_file(round_path) for round_path in paths] + + # Every round is indexed by position below, so a round that measured a + # different set of queries has to be caught here rather than silently + # compared query against query. + queries = [query.query for query in rounds[0].queries] + for round_path, round in zip(paths[1:], rounds[1:]): + if [query.query for query in round.queries] != queries: + raise SystemExit( + f"{round_path} measured different queries than {paths[0]}" + ) + + return cls(header, rounds) + + @property + def queries(self) -> List[QueryRun]: + """The first round's queries, for labels and ordering.""" + return self.rounds[0].queries + + def merged(self, index: int) -> QueryRun: + """One query's rounds collapsed into a single run, for display.""" + runs = [round.queries[index] for round in self.rounds] + + return QueryRun( + query=runs[0].query, + iterations=[ + iteration for run in runs for iteration in run.iterations + ], + start_time=runs[0].start_time, + success=all(run.success for run in runs), + ) + + def per_round(self, index: int) -> List[float]: + """One query's fastest time in each round.""" + return [round.queries[index].min_execution_time for round in self.rounds] + + +@dataclass +class QueryComparison: + """One query's verdict, and the numbers behind it.""" + + label: str + # comparison / baseline in each round, paired by round + ratios: List[float] + # median of `ratios`: the estimate the gate rules on + ratio: float + # how much the baseline varies against itself, as a relative slowdown + noise: float + # absolute regression in ms, on the per-round medians + delta_ms: float + # the limit asked for on the command line + configured_limit: float + # the limit the ratio was actually held to: the configured one, raised to + # the noise floor when the baseline is too unstable to support it + limit: float + + @property + def regressed(self) -> bool: + return self.ratio > self.limit + + @property + def inconclusive(self) -> bool: + """Above the configured limit, but not above this query's noise.""" + return not self.regressed and self.ratio > self.configured_limit + + def summary(self) -> str: + spread = "" + if len(self.ratios) > 1: + spread = f" (rounds {min(self.ratios):.2f}-{max(self.ratios):.2f})" + + limit = f"limit {self.limit:.2f}x" + if self.limit > self.configured_limit: + limit += ( + f", raised from {self.configured_limit:.2f}x " + f"by a {self.noise:.0%} noise floor" + ) + + return ( + f"{self.label} is {self.ratio:.2f}x slower{spread}, " + f"+{self.delta_ms:.0f}ms ({limit})" + ) + + def compare( baseline_path: Path, comparison_path: Path, @@ -161,26 +301,38 @@ def compare( detailed: bool, fail_threshold: float | None = None, fail_total_threshold: float | None = None, + min_delta_ms: float = 0.0, ) -> int: """Print the comparison and return the process exit code. The exit code is non-zero only when a `fail_*_threshold` is given and the comparison run is slower than it allows. """ - baseline = BenchmarkRun.load_from_file(baseline_path) - comparison = BenchmarkRun.load_from_file(comparison_path) + baseline = Side.load(baseline_path) + comparison = Side.load(comparison_path) console = Console(width=200) - # use basename as the column names - baseline_header = baseline_path.parent.name - comparison_header = comparison_path.parent.name + rounds = min(len(baseline.rounds), len(comparison.rounds)) + if len(baseline.rounds) != len(comparison.rounds): + console.print( + f"[yellow]{baseline.header} has {len(baseline.rounds)} round(s) and " + f"{comparison.header} has {len(comparison.rounds)}; comparing the " + f"first {rounds} of each[/yellow]" + ) + + multi_round = rounds > 1 table = Table(show_header=True, header_style="bold magenta") table.add_column("Query", style="dim", no_wrap=True) - table.add_column(baseline_header, justify="right", style="dim", no_wrap=True) - table.add_column(comparison_header, justify="right", style="dim", no_wrap=True) + table.add_column(baseline.header, justify="right", style="dim", no_wrap=True) + table.add_column(comparison.header, justify="right", style="dim", no_wrap=True) table.add_column("Change", justify="right", style="dim", no_wrap=True) + if multi_round: + # The gate rules on the median of the per-round ratios, so show it + # next to the fastest-run ratio the `Change` column reports. + table.add_column("Per-round", justify="right", style="dim", no_wrap=True) + table.add_column("Noise", justify="right", style="dim", no_wrap=True) faster_count = 0 slower_count = 0 @@ -188,38 +340,69 @@ def compare( failure_count = 0 total_baseline_time = 0 total_comparison_time = 0 - # (label, comparison / baseline) for every query that ran on both sides, - # and the labels of queries that only the comparison run failed - changes: List[tuple[str, float]] = [] + # Per-round totals over the queries that ran on both sides, so the total + # is gated the same paired way a single query is + baseline_totals = [0.0] * rounds + comparison_totals = [0.0] * rounds + comparisons: List[QueryComparison] = [] new_failures: List[str] = [] - for baseline_result, comparison_result in zip(baseline.queries, comparison.queries): + for index, (baseline_result, comparison_result) in enumerate( + zip(baseline.queries, comparison.queries) + ): assert baseline_result.query == comparison_result.query - base_failed = not baseline_result.success - comp_failed = not comparison_result.success + baseline_merged = baseline.merged(index) + comparison_merged = comparison.merged(index) + + base_failed = not baseline_merged.success + comp_failed = not comparison_merged.success # If a query fails, its execution time is excluded from the performance comparison if base_failed or comp_failed: change_text = "incomparable" failure_count += 1 if comp_failed and not base_failed: - new_failures.append(baseline_result.label) - table.add_row( - baseline_result.label, - "FAIL" if base_failed else baseline_result.execution_time_report(detailed)[1], - "FAIL" if comp_failed else comparison_result.execution_time_report(detailed)[1], + new_failures.append(baseline_merged.label) + row = [ + baseline_merged.label, + "FAIL" if base_failed else baseline_merged.execution_time_report(detailed)[1], + "FAIL" if comp_failed else comparison_merged.execution_time_report(detailed)[1], change_text, - ) + ] + table.add_row(*(row + ["", ""] if multi_round else row)) continue - baseline_value, baseline_text = baseline_result.execution_time_report(detailed) - comparison_value, comparison_text = comparison_result.execution_time_report(detailed) + baseline_value, baseline_text = baseline_merged.execution_time_report(detailed) + comparison_value, comparison_text = comparison_merged.execution_time_report(detailed) total_baseline_time += baseline_value total_comparison_time += comparison_value + baseline_rounds = baseline.per_round(index)[:rounds] + comparison_rounds = comparison.per_round(index)[:rounds] + for round_index in range(rounds): + baseline_totals[round_index] += baseline_rounds[round_index] + comparison_totals[round_index] += comparison_rounds[round_index] + + ratios = [ + comparison_round / baseline_round + for baseline_round, comparison_round in zip(baseline_rounds, comparison_rounds) + ] + noise = upward_spread(baseline_rounds) + configured_limit = fail_threshold if fail_threshold is not None else math.inf + comparisons.append( + QueryComparison( + label=baseline_merged.label, + ratios=ratios, + ratio=median(ratios), + noise=noise, + delta_ms=median(comparison_rounds) - median(baseline_rounds), + limit=max(configured_limit, 1.0 + noise), + configured_limit=configured_limit, + ) + ) + change = comparison_value / baseline_value - changes.append((baseline_result.label, change)) if (1.0 - noise_threshold) <= change <= (1.0 + noise_threshold): change_text = "no change" @@ -231,12 +414,16 @@ def compare( change_text = f"{change:.2f}x slower" slower_count += 1 - table.add_row( - baseline_result.label, + row = [ + baseline_merged.label, baseline_text, comparison_text, change_text, - ) + ] + if multi_round: + row.append(f"{median(ratios):.2f}x ({min(ratios):.2f}-{max(ratios):.2f})") + row.append(f"±{noise:.0%}") + table.add_row(*row) console.print(table) @@ -252,16 +439,38 @@ def compare( total_comparison_time / total_baseline_time if total_baseline_time else 1.0 ) + total_ratios = [ + comparison_total / baseline_total + for baseline_total, comparison_total in zip(baseline_totals, comparison_totals) + if baseline_total + ] or [1.0] + total_noise = upward_spread(baseline_totals) if baseline_totals[0] else 0.0 + # Summary table summary_table = Table(show_header=True, header_style="bold magenta") summary_table.add_column("Benchmark Summary", justify="left", style="dim") summary_table.add_column("", justify="right", style="dim") - summary_table.add_row(f"Total Time ({baseline_header})", f"{total_baseline_time:.2f}ms") - summary_table.add_row(f"Total Time ({comparison_header})", f"{total_comparison_time:.2f}ms") - summary_table.add_row(f"Average Time ({baseline_header})", f"{avg_baseline_time:.2f}ms") - summary_table.add_row(f"Average Time ({comparison_header})", f"{avg_comparison_time:.2f}ms") + summary_table.add_row("Rounds", str(rounds)) + summary_table.add_row( + "Iterations per round", + str(len(baseline.queries[0].iterations)) if baseline.queries else "0", + ) + summary_table.add_row("CPUs", str(baseline.rounds[0].context.num_cpus)) + summary_table.add_row(f"Total Time ({baseline.header})", f"{total_baseline_time:.2f}ms") + summary_table.add_row(f"Total Time ({comparison.header})", f"{total_comparison_time:.2f}ms") + summary_table.add_row(f"Average Time ({baseline.header})", f"{avg_baseline_time:.2f}ms") + summary_table.add_row(f"Average Time ({comparison.header})", f"{avg_comparison_time:.2f}ms") summary_table.add_row("Total Change", f"{total_change:.2f}x") + if multi_round: + summary_table.add_row("Total Change (per-round median)", f"{median(total_ratios):.2f}x") + summary_table.add_row("Geometric Mean of Query Ratios", f"{geometric_mean([c.ratio for c in comparisons]):.2f}x" if comparisons else "n/a") + summary_table.add_row( + f"Noise Floor ({baseline.header}, median / worst query)", + f"{median([c.noise for c in comparisons]):.1%} / {max([c.noise for c in comparisons]):.1%}" + if comparisons + else "n/a", + ) summary_table.add_row("Queries Faster", str(faster_count)) summary_table.add_row("Queries Slower", str(slower_count)) summary_table.add_row("Queries with No Change", str(no_change_count)) @@ -271,13 +480,15 @@ def compare( return report_regressions( console, - baseline_header, - comparison_header, - changes, + baseline.header, + comparison.header, + comparisons, new_failures, - total_change, + median(total_ratios), + total_noise, fail_threshold, fail_total_threshold, + min_delta_ms, ) @@ -285,30 +496,60 @@ def report_regressions( console: Console, baseline_header: str, comparison_header: str, - changes: List[tuple[str, float]], + comparisons: List[QueryComparison], new_failures: List[str], - total_change: float, + total_ratio: float, + total_noise: float, fail_threshold: float | None, fail_total_threshold: float | None, + min_delta_ms: float, ) -> int: - """Report against the configured limits, returning the process exit code.""" + """Report against the configured limits, returning the process exit code. + + A query only fails the gate when its slowdown clears three bars: the + configured limit, the noise floor the baseline showed against itself, and + a minimum absolute cost in milliseconds. Anything that clears the first + but not the others is printed as inconclusive -- the machine could not + measure it that precisely, which is a fact about the run and not about the + change under test. + """ if fail_threshold is None and fail_total_threshold is None: return 0 problems: List[str] = [] + notes: List[str] = [] if fail_threshold is not None: - problems.extend( - f"{label} is {change:.2f}x slower (limit {fail_threshold:.2f}x)" - for label, change in changes - if change > fail_threshold - ) - - if fail_total_threshold is not None and total_change > fail_total_threshold: - problems.append( - f"total time is {total_change:.2f}x slower " - f"(limit {fail_total_threshold:.2f}x)" - ) + for query in comparisons: + if not query.regressed: + if query.inconclusive: + notes.append( + f"{query.label} is {query.ratio:.2f}x slower, within the " + f"{query.noise:.0%} spread {baseline_header} showed against " + f"itself -- too noisy to call" + ) + continue + + if query.delta_ms < min_delta_ms: + notes.append( + f"{query.label} is {query.ratio:.2f}x slower, but only " + f"+{query.delta_ms:.0f}ms (below the {min_delta_ms:.0f}ms floor)" + ) + continue + + problems.append(query.summary()) + + if fail_total_threshold is not None: + total_limit = max(fail_total_threshold, 1.0 + total_noise) + if total_ratio > total_limit: + problems.append( + f"total time is {total_ratio:.2f}x slower (limit {total_limit:.2f}x)" + ) + elif total_ratio > fail_total_threshold: + notes.append( + f"total time is {total_ratio:.2f}x slower, within the " + f"{total_noise:.0%} spread {baseline_header} showed against itself" + ) # A query that only fails on one side is excluded from the timings above, # so it would otherwise pass the gate unnoticed. @@ -317,6 +558,11 @@ def report_regressions( for label in new_failures ) + if notes: + console.print("Not counted against the gate:") + for note in notes: + console.print(f" - {note}", markup=False) + if not problems: console.print( f"No regression: {comparison_header} is within the configured limits " @@ -326,7 +572,7 @@ def report_regressions( console.print(f"Regression: {comparison_header} is slower than {baseline_header}") for problem in problems: - console.print(f" - {problem}") + console.print(f" - {problem}", markup=False) return 1 @@ -337,12 +583,15 @@ def main() -> None: compare_parser.add_argument( "baseline_path", type=Path, - help="Path to the baseline summary file.", + help="Path to the baseline summary file, or to a directory holding one " + "summary file per round.", ) compare_parser.add_argument( "comparison_path", type=Path, - help="Path to the comparison summary file.", + help="Path to the comparison summary file, or to a directory holding " + "one summary file per round. Rounds are paired with the baseline's in " + "sorted filename order.", ) compare_parser.add_argument( "--noise-threshold", @@ -364,6 +613,14 @@ def main() -> None: help="Exit non-zero if the total time is slower than this ratio " "(e.g. 1.05 for 5%% slower). Off by default.", ) + compare_parser.add_argument( + "--fail-min-delta-ms", + type=float, + default=0.0, + help="Never fail on a query that got less than this many milliseconds " + "slower, however large the ratio. Keeps short queries, where a large " + "relative change is a small absolute one, out of the gate.", + ) compare_parser.add_argument( "--detailed", action=argparse.BooleanOptionalAction, @@ -381,6 +638,7 @@ def main() -> None: options.detailed, options.fail_threshold, options.fail_total_threshold, + options.fail_min_delta_ms, ) ) From e8eb355772468c2800e664fad554f8d44e299768 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Heres?= Date: Fri, 21 Aug 2026 19:25:58 +0200 Subject: [PATCH 07/10] ci: benchmark every merge on main, and call the two sides base and head The check is opt-in on pull requests, which means most changes land without ever being measured. So it now also runs on every push to `main`, where the base is the commit the merge landed on: a regression that no pull request measured is still attributed to the one merge that introduced it, with no bisect to run afterwards. Doc-only merges are skipped through the same `paths-ignore` list `rust.yml` uses. `HEAD^1` is the right base for a push whether the merge was squashed into one commit or kept as a merge commit, but the existing fallback would have fetched the default branch and compared its tip against itself, so the push case is resolved explicitly. The `performance` label still gates pull requests; every other event runs unconditionally. The candidate side is a merged commit on `main` as often as it is a PR now, so it is renamed from `pr` to `head` -- base and head being what GitHub calls these two anyway. It also happens to make the two worktree paths the same length, so the file paths baked into the two binaries are the same length too, which is where most of the bytes that differ between them come from. Co-Authored-By: Claude Opus 5 --- .github/workflows/benchmark.yml | 64 ++++++++++++++++++++++----------- benchmarks/README.md | 23 ++++++++---- 2 files changed, 59 insertions(+), 28 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 9dbda48a21bc0..b2d094156007e 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -15,10 +15,11 @@ # specific language governing permissions and limitations # under the License. -# Catches performance regressions by running TPC-H SF10 for the base branch and -# for the PR merged into it, then failing when the PR is slower than the -# configured limits allow. SF10 rather than SF1 because SF1 queries finish in -# milliseconds, where runner noise is a large fraction of the measurement. +# Catches performance regressions by running TPC-H SF10 twice -- once for the +# candidate commit (`head`) and once for the commit it sits on (`base`) -- then +# failing when `head` is slower than the configured limits allow. SF10 rather +# than SF1 because SF1 queries finish in milliseconds, where runner noise is a +# large fraction of the measurement. # # The two binaries are built by two jobs, on a runner each, and both are then # measured by a third job on one machine: building is embarrassingly parallel, @@ -38,10 +39,22 @@ concurrency: group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} cancel-in-progress: true -# The builds and the benchmark runs add up to roughly half an hour of runner -# time, and benchmarks on shared runners are noisy, so this is opt-in: add the -# `performance` label to a PR, or start it by hand from the Actions tab. +# On `main` this runs on every push, so each merge is measured against the +# commit it landed on and a regression that no PR run caught is still pinned to +# one merge. On a PR it is opt-in, because the builds and the benchmark runs add +# up to roughly half an hour of runner time: add the `performance` label, or +# start it by hand from the Actions tab. on: + push: + branches: + # The default branch upstream. A fork that calls it something else has to + # add that name here for its own merges to be measured. + - main + paths-ignore: + - "docs/**" + - "**.md" + - ".github/ISSUE_TEMPLATE/**" + - ".github/pull_request_template.md" pull_request: types: [opened, synchronize, reopened, labeled] workflow_dispatch: @@ -118,7 +131,8 @@ jobs: # Resolved once, so the three jobs below cannot disagree about what "base" is. resolve: name: resolve base commit - if: github.event_name == 'workflow_dispatch' || contains(github.event.pull_request.labels.*.name, 'performance') + # Every event except `pull_request` runs unconditionally; a PR needs the label. + if: github.event_name != 'pull_request' || contains(github.event.pull_request.labels.*.name, 'performance') runs-on: ${{ vars.USE_RUNS_ON == 'true' && format('runs-on={0},family=c8a+m8a,cpu=2,image=ubuntu24-full-x64,extras=s3-cache,tag=datafusion', github.run_id) || 'ubuntu-latest' }} timeout-minutes: 15 outputs: @@ -128,7 +142,8 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: # For `pull_request` this is the PR already merged into the base - # branch; depth 2 is enough to also reach the merge's first parent. + # branch, for `push` the new branch tip; depth 2 is enough to also + # reach the first parent in both cases. fetch-depth: 2 - name: Resolve base commit id: base @@ -136,7 +151,14 @@ jobs: BASE_REF: ${{ github.event.pull_request.base.ref || github.event.repository.default_branch }} run: | set -euo pipefail - if [ "$(git rev-list --parents -n 1 HEAD | wc -w)" -ge 3 ]; then + if [ "$GITHUB_EVENT_NAME" = "push" ]; then + # A merge that just landed on the default branch. Its first parent + # is the branch as it was before, whether the merge was squashed + # into one commit or kept as a merge commit, so the difference is + # attributable to this one merge. + base_sha=$(git rev-parse "HEAD^1") + echo "push to $GITHUB_REF_NAME, comparing its tip against the parent" + elif [ "$(git rev-list --parents -n 1 HEAD | wc -w)" -ge 3 ]; then # HEAD is the PR merged into the base branch, so its first parent # is the base branch tip that the merge used -- the commit this PR # would actually land on. @@ -161,7 +183,7 @@ jobs: # No point in building one side if the other one is broken. fail-fast: true matrix: - side: [base, pr] + side: [base, head] steps: - uses: runs-on/action@46910bf61b41721b0579f237e186afb35477007a # v2.3.0 - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -205,7 +227,7 @@ jobs: # Both sides are measured here, on this one machine, back to back. benchmark: - name: TPC-H (PR vs base) + name: TPC-H (head vs base) needs: [resolve, build] runs-on: ${{ vars.USE_RUNS_ON == 'true' && format('runs-on={0},family=c8a+m8a,cpu=16,image=ubuntu24-full-x64,extras=s3-cache,disk=large,tag=datafusion', github.run_id) || 'ubuntu-latest' }} timeout-minutes: 90 @@ -225,23 +247,23 @@ jobs: # The same paths the build jobs used, so each binary finds the # `sql_benchmarks` directory of the tree it was built from. git worktree add --detach "$BENCH_ROOT/base" "$BASE_SHA" - git worktree add --detach "$BENCH_ROOT/pr" HEAD + git worktree add --detach "$BENCH_ROOT/head" HEAD - name: Download base benchmark_runner uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: benchmark_runner-base path: ${{ runner.temp }}/bin/base - - name: Download PR benchmark_runner + - name: Download head benchmark_runner uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: - name: benchmark_runner-pr - path: ${{ runner.temp }}/bin/pr + name: benchmark_runner-head + path: ${{ runner.temp }}/bin/head - name: Check both runners see their queries run: | # Artifacts do not carry the executable bit, and a binary whose tree # is missing at the path baked into it would discover no benchmark at # all -- fail here rather than three steps later. - for side in base pr; do + for side in base head; do binary="$RUNNER_TEMP/bin/$side/benchmark_runner" chmod +x "$binary" if ! "$binary" --list | grep -qE '^[[:space:]]+tpch[[:space:]]'; then @@ -262,7 +284,7 @@ jobs: # project attaches no binaries to its GitHub releases for a prebuilt # fetch to find. It also means this job needs no Rust toolchain at all. run: | - mkdir -p "$DATA_DIR/tpch_sf$SCALE_FACTOR" "$RESULTS_DIR/base" "$RESULTS_DIR/pr" + mkdir -p "$DATA_DIR/tpch_sf$SCALE_FACTOR" "$RESULTS_DIR/base" "$RESULTS_DIR/head" uv tool run --from 'tpchgen-cli==3.0.0' tpchgen-cli \ --scale-factor "$SCALE_FACTOR" \ --format parquet \ @@ -307,7 +329,7 @@ jobs: # Alternate which side goes first, so with an even ROUNDS # whatever the first position costs -- or saves -- is paid by each # side the same number of times. - if [ $((round % 2)) -eq 1 ]; then order="base pr"; else order="pr base"; fi + if [ $((round % 2)) -eq 1 ]; then order="base head"; else order="head base"; fi for side in $order; do echo "::group::round $round: $side" # Zero-padded, because compare.py pairs the two sides' rounds in @@ -322,14 +344,14 @@ jobs: status=0 uv run --no-project --with rich python3 benchmarks/compare.py \ "$RESULTS_DIR/base" \ - "$RESULTS_DIR/pr" \ + "$RESULTS_DIR/head" \ --fail-threshold "$QUERY_REGRESSION" \ --fail-total-threshold "$TOTAL_REGRESSION" \ --fail-min-delta-ms "$MIN_DELTA_MS" \ > "$RESULTS_DIR/comparison.txt" 2>&1 || status=$? cat "$RESULTS_DIR/comparison.txt" { - echo "### TPC-H SF$SCALE_FACTOR: base (${{ needs.resolve.outputs.base_sha }}) vs PR" + echo "### TPC-H SF$SCALE_FACTOR: \`head\` (${{ github.event_name == 'push' && 'the merge that just landed' || 'this PR, merged into the base branch' }}) vs \`base\` (${{ needs.resolve.outputs.base_sha }})" echo echo "\`$ROUNDS\` rounds of \`$ITERATIONS\` iteration(s), the two sides interleaved and their order alternated per round." echo "A query fails the gate when the median of its per-round ratios is above \`${QUERY_REGRESSION}x\`, the regression costs at least \`${MIN_DELTA_MS}ms\`, and it is larger than the spread the base side showed against itself." diff --git a/benchmarks/README.md b/benchmarks/README.md index 2fc4d52dc5cbf..1e2be4709bfb5 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -260,17 +260,26 @@ Note: if `gh` is installed, you can also run `gh pr checkout $PR_NUMBER` instead ### In CI The `Benchmarks` workflow (`.github/workflows/benchmark.yml`) runs TPC-H SF10 -against the base branch and against the PR merged into it, both on the same -runner, and fails when the PR is slower than the configured limits allow. SF10 -rather than SF1, because SF1 queries finish in milliseconds, where runner noise -is a large fraction of the measurement. It is opt-in, because the builds plus -two SF10 runs take a good half hour: - -- add the `performance` label to a PR, or +twice on the same runner -- once for a candidate commit (`head`) and once for +the commit it sits on (`base`) -- and fails when `head` is slower than the +configured limits allow. SF10 rather than SF1, because SF1 queries finish in +milliseconds, where runner noise is a large fraction of the measurement. + +It runs on **every push to `main`**, with `base` being the commit the merge +landed on, so a regression that no pull request measured is still pinned to the +one merge that introduced it. On a **pull request** it is opt-in, because the +builds plus the benchmark runs take a good half hour: + +- add the `performance` label to a PR, whose `head` is then the PR merged into + its base branch, or - start it from the Actions tab (`workflow_dispatch`), where the scale factor, round and iteration counts, all three regression limits, and the cargo profile can be overridden +A failure on `main` blocks nothing -- there is no PR left to hold up -- so read +it as a bisect that has already been done for you: the run names the commit and +the queries, and the uploaded rounds show how solid the verdict is. + The workflow is three jobs: one resolves the base commit, two build a `benchmark_runner` each (one runner per side, so the builds really are simultaneous), and the last one measures both binaries on a single machine, From af4197b19cd8bf5bc70dce393a8101598716b3dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Heres?= Date: Fri, 21 Aug 2026 19:30:08 +0200 Subject: [PATCH 08/10] ci: say which machine a benchmark run lands on, and why The two runs whose false Q02 regressions prompted the interleaved design were measured on four vCPUs, not on the 16-vCPU runner the `runs-on` lines ask for. The reason is not the workflow: GitHub withholds `vars` from workflows triggered by a pull request from a fork, so `vars.USE_RUNS_ON` reads as empty there however the repository has it set, and every job takes the `ubuntu-latest` fallback. The same thing happens to all of `rust.yml` -- `linux build test` runs on `runs-on=...,cpu=8` for a push to `main` and on `ubuntu-latest` for a fork pull request. Nothing can be done about that from inside the workflow: reaching the larger runner from a fork pull request would mean `pull_request_target` or `workflow_run`, which is building and running unreviewed code with the base repository's token. What can be done is to stop it being a surprise, so the machine step now explains the fallback rather than only warning about the core count, and points at the `main` run after the merge as the authoritative one -- that one does get the 16-vCPU runner, which is the other half of why measuring every merge is worth the runner time. Also notes the consequence of the faster machine: an SF10 query that takes 300ms on four vCPUs takes under 100ms on sixteen, and a 20% regression on that is inside the 25ms floor, so the shortest few queries stop being gated unless the run is dispatched with a smaller floor or a larger scale factor. Co-Authored-By: Claude Opus 5 --- .github/workflows/benchmark.yml | 14 +++++++++----- benchmarks/README.md | 17 +++++++++++++++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index b2d094156007e..f7f9b57ab5d13 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -294,16 +294,20 @@ jobs: du -sh "$DATA_DIR/tpch_sf$SCALE_FACTOR" df -h "$DATA_DIR" - name: Describe the machine - # A comparison is only as good as the machine under it, and the runner - # this lands on is not always the one the `runs-on` line above asks for - # -- without the RunsOn variable set it falls back to a shared 4-vCPU - # `ubuntu-latest`, where the noise floor is several times higher. + # A comparison is only as good as the machine under it, and this does + # not always land on the machine the `runs-on` line above asks for. + # GitHub withholds `vars` from workflows triggered by a pull request + # from a fork, so `vars.USE_RUNS_ON` reads as empty there however it is + # set on the repository, and every job falls back to a shared 4-vCPU + # `ubuntu-latest` -- which is where this workflow's own false Q02 + # regressions were measured. Pushes to `main` and manual dispatches run + # in the repository's own context and do get the 16-vCPU runner. run: | echo "cpus: $(nproc)" lscpu | grep -E '^(Model name|Socket|Core|Thread|CPU\(s\)):' || true free -h if [ "$(nproc)" -lt 8 ]; then - echo "::warning::running on $(nproc) CPUs; expect a high noise floor and marginal verdicts" + echo "::warning::running on $(nproc) CPUs, not the runner this workflow asks for; expect a high noise floor. Fork pull requests cannot reach the larger runner -- the run on \`main\` after the merge is the authoritative one." fi # Each side runs from its own tree and reads the one generated dataset. - name: Benchmark both sides, interleaved diff --git a/benchmarks/README.md b/benchmarks/README.md index 1e2be4709bfb5..4e3e943ae5a95 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -280,6 +280,23 @@ A failure on `main` blocks nothing -- there is no PR left to hold up -- so read it as a bisect that has already been done for you: the run names the commit and the queries, and the uploaded rounds show how solid the verdict is. +Which machine the run lands on follows from the trigger, and it matters more +than any of the limits below. GitHub withholds `vars` from workflows triggered +by a pull request from a fork, so `vars.USE_RUNS_ON` reads as empty there no +matter how the repository has it set, and the run falls back to a shared 4-vCPU +`ubuntu-latest` -- the same fallback the whole of `rust.yml` takes on fork pull +requests. A push to `main` or a manual dispatch runs in the repository's own +context and gets the 16-vCPU runner the workflow asks for. So a labelled fork +pull request is a coarse signal, and the run on `main` right after the merge is +the measurement; the benchmark job prints its CPU count and warns when it is +on the fallback. + +One consequence of the faster machine: SF10 queries that take 300ms on four +vCPUs take under 100ms on sixteen, and a 20% regression on those is inside the +25ms floor described below, so the four shortest queries stop being gated. +Dispatch with a smaller `min_delta_ms`, or a larger scale factor, when those +are the queries in question. + The workflow is three jobs: one resolves the base commit, two build a `benchmark_runner` each (one runner per side, so the builds really are simultaneous), and the last one measures both binaries on a single machine, From 5eef2cdbe1cda213dc86dbf3c80ebd3356e83a18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Heres?= Date: Fri, 21 Aug 2026 19:42:34 +0200 Subject: [PATCH 09/10] ci: drop the deprecated tpchgen-cli flags, and cut the workflow's wall clock tpchgen-cli 3.0.0 warns that `--format` goes away in 4.0.0 and that `--parquet-compression` is deprecated, both in favour of a subcommand per format. `tpchgen-cli parquet --compression=...` produces a byte-identical tree at SF0.01, so this is a rename; `bench.sh` gets the same treatment for its parquet, csv and sort-pushdown calls, and says which version it needs. The step timings of the SF10 run say where the half hour goes, and it is not the part that looks expensive: build base runner 990s (835s cargo, 131s freeing disk space) build head runner 966s (845s cargo, 96s freeing disk space) benchmark 461s (410s measuring, 34s generating the data) So generating the data is 34 seconds and needs nothing done to it, while the builds are two minutes of deleting Android SDKs followed by fourteen minutes of compiling dependencies that neither side changed. The builds now restore a `Swatinem/rust-cache` entry, shared between the two sides because they are a commit or two apart, written only by pushes to `main` so pull request runs do not each save a near-identical copy; and the disk cleanup is skipped when the runner already asked for `disk=large`, which is every run that gets the RunsOn machine. The warmup pass is now a read of the data files rather than a discarded pass of the suite. Each round is a fresh process, so the page cache is the only thing a warmup can carry between rounds, and reading the files fills it in seconds where a pass of the suite cost as much as a measured round. Co-Authored-By: Claude Opus 5 --- .github/workflows/benchmark.yml | 43 +++++++++++++++++++++++++-------- benchmarks/README.md | 5 +++- benchmarks/bench.sh | 8 +++--- 3 files changed, 41 insertions(+), 15 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index f7f9b57ab5d13..b09ea2e182641 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -190,6 +190,11 @@ jobs: with: fetch-depth: 2 - name: Free Disk Space (Ubuntu) + # Two minutes of deleting Android SDKs, which a release build of the + # workspace genuinely needs on `ubuntu-latest`'s 14GB. The RunsOn + # runner above asks for `disk=large` and has no such problem, so only + # the fallback pays for this. + if: vars.USE_RUNS_ON != 'true' uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be # v1.3.1 - name: Install Rust run: | @@ -210,14 +215,28 @@ jobs: else git worktree add --detach "$BENCH_ROOT/$SIDE" HEAD fi + - name: Cache the dependency build + # A cold build of this is fourteen minutes, most of it dependencies + # that neither side changed. The action drops workspace crates from + # what it saves, so a hit rebuilds DataFusion and reuses the rest. + # + # `workspaces` because the build happens in a worktree outside the + # checkout, and one shared key for both sides because they are a commit + # or two apart and share a dependency graph. Only a push to `main` + # writes the cache: pull request runs would each save a near-identical + # copy of it. + uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 + with: + workspaces: ${{ env.BENCH_ROOT }}/${{ matrix.side }} + shared-key: benchmark-${{ env.CARGO_PROFILE }} + save-if: ${{ github.event_name == 'push' }} - name: Build benchmark_runner env: SIDE: ${{ matrix.side }} - CARGO_TARGET_DIR: ${{ runner.temp }}/target run: | cd "$BENCH_ROOT/$SIDE" cargo build --profile "$CARGO_PROFILE" -p datafusion-benchmarks --bin benchmark_runner - cp "$CARGO_TARGET_DIR/$CARGO_PROFILE/benchmark_runner" "$RUNNER_TEMP/benchmark_runner" + cp "target/$CARGO_PROFILE/benchmark_runner" "$RUNNER_TEMP/benchmark_runner" - name: Upload benchmark_runner uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: @@ -283,12 +302,14 @@ jobs: # crate, but a ~4MB download instead of a build from source, since the # project attaches no binaries to its GitHub releases for a prebuilt # fetch to find. It also means this job needs no Rust toolchain at all. + # + # `parquet` as a subcommand rather than `--format parquet`: 3.0.0 + # deprecated the flag form and warns that it goes away in 4.0.0. run: | mkdir -p "$DATA_DIR/tpch_sf$SCALE_FACTOR" "$RESULTS_DIR/base" "$RESULTS_DIR/head" - uv tool run --from 'tpchgen-cli==3.0.0' tpchgen-cli \ + uv tool run --from 'tpchgen-cli==3.0.0' tpchgen-cli parquet \ --scale-factor "$SCALE_FACTOR" \ - --format parquet \ - --parquet-compression 'ZSTD(1)' \ + --compression 'ZSTD(1)' \ --parts=1 \ --output-dir "$DATA_DIR/tpch_sf$SCALE_FACTOR" du -sh "$DATA_DIR/tpch_sf$SCALE_FACTOR" @@ -323,11 +344,13 @@ jobs: --path "$DATA_DIR" \ --output "$output" } - # One discarded pass first, so no measured round is the only one - # paying to pull the parquet files into the page cache. It writes - # outside RESULTS_DIR, which is where compare.py looks for rounds. - echo "::group::warmup" - run_side base "$RUNNER_TEMP/warmup.json" + # Read the data once first, so no measured round is the only one + # paying to pull the parquet files into the page cache. Reading the + # files is all a warmup can carry between rounds -- each round is a + # fresh process -- and it takes seconds where a discarded pass of the + # suite took as long as a round. + echo "::group::warm the page cache" + find "$DATA_DIR" -type f -exec cat {} + > /dev/null echo "::endgroup::" for round in $(seq 1 "$ROUNDS"); do # Alternate which side goes first, so with an even ROUNDS diff --git a/benchmarks/README.md b/benchmarks/README.md index 4e3e943ae5a95..8438ef78d2f2b 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -333,7 +333,10 @@ JSON plus the table are uploaded as the `tpch-comparison` artifact. The data is generated with the same `tpchgen-cli` settings `bench.sh data tpch` uses, from the tool's PyPI wheel rather than a source build, which keeps a Rust -toolchain out of the benchmark job entirely. +toolchain out of the benchmark job entirely. Generating SF10 takes well under a +minute; what the workflow's half hour actually goes on is the two builds, so +they restore a dependency cache and skip the `ubuntu-latest` disk cleanup when +the runner has the disk already. Runners are shared machines, so treat the numbers as a signal rather than a measurement. A query has to clear three separate bars to fail the gate, and diff --git a/benchmarks/bench.sh b/benchmarks/bench.sh index df9b7f6c94f16..2475457426246 100755 --- a/benchmarks/bench.sh +++ b/benchmarks/bench.sh @@ -668,7 +668,7 @@ data_tpch() { # check if tpchgen-cli is installed if ! command -v tpchgen-cli &> /dev/null then - echo "tpchgen-cli could not be found, please install it via 'cargo install tpchgen-cli'" + echo "tpchgen-cli could not be found, please install it via 'cargo install tpchgen-cli' (3.0 or newer)" exit 1 fi @@ -689,7 +689,7 @@ data_tpch() { echo " parquet files exist ($FILE exists)." else echo " creating parquet files using tpchgen-cli ..." - tpchgen-cli --scale-factor "${SCALE_FACTOR}" --format parquet --parquet-compression='ZSTD(1)' --parts=1 --output-dir "${TPCH_DIR}" + tpchgen-cli parquet --scale-factor "${SCALE_FACTOR}" --compression='ZSTD(1)' --parts=1 --output-dir "${TPCH_DIR}" fi return fi @@ -701,7 +701,7 @@ data_tpch() { echo " csv files exist ($FILE exists)." else echo " creating csv files using tpchgen-cli binary ..." - tpchgen-cli --scale-factor "${SCALE_FACTOR}" --format csv --parts=1 --output-dir "${TPCH_DIR}/csv" + tpchgen-cli csv --scale-factor "${SCALE_FACTOR}" --parts=1 --output-dir "${TPCH_DIR}/csv" fi return fi @@ -1308,7 +1308,7 @@ data_sort_pushdown() { TEMP_DIR="${DATA_DIR}/sort_pushdown_temp" mkdir -p "${TEMP_DIR}" "${SORT_PUSHDOWN_DIR}" - tpchgen-cli --scale-factor 1 --format parquet --parquet-compression='ZSTD(1)' --parts=3 --output-dir "${TEMP_DIR}" + tpchgen-cli parquet --scale-factor 1 --compression='ZSTD(1)' --parts=3 --output-dir "${TEMP_DIR}" # Rename: reverse alphabetical order vs key order mv "${TEMP_DIR}/lineitem/lineitem.3.parquet" "${SORT_PUSHDOWN_DIR}/a_part3.parquet" From 3694485e5dfa0e0056f1760464149b4b01b3ce7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Heres?= Date: Fri, 21 Aug 2026 19:56:35 +0200 Subject: [PATCH 10/10] ci: trim the benchmark comments and README Most of the prose was two or three times longer than the point it made. Cut to a sentence or two each, keeping the facts that are not visible from the code: why the sides are interleaved, the three bars a query clears to fail the gate, and that fork pull requests cannot reach the larger runner. 107 lines lighter, with no behaviour change -- `compare.py` gives the same verdict on the last run's rounds. Co-Authored-By: Claude Opus 5 --- .github/workflows/benchmark.yml | 127 ++++++++----------------- benchmarks/README.md | 160 ++++++++++++-------------------- benchmarks/compare.py | 34 +++---- 3 files changed, 107 insertions(+), 214 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index b09ea2e182641..684df57683ad9 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -15,23 +15,11 @@ # specific language governing permissions and limitations # under the License. -# Catches performance regressions by running TPC-H SF10 twice -- once for the -# candidate commit (`head`) and once for the commit it sits on (`base`) -- then -# failing when `head` is slower than the configured limits allow. SF10 rather -# than SF1 because SF1 queries finish in milliseconds, where runner noise is a -# large fraction of the measurement. -# -# The two binaries are built by two jobs, on a runner each, and both are then -# measured by a third job on one machine: building is embarrassingly parallel, -# while comparing timings taken on different machines is meaningless. -# -# The two sides are measured interleaved -- a full pass of one, then a full -# pass of the other, several times, alternating which goes first -- rather than -# all of one side and then all of the other. Measuring in two blocks makes any -# drift between them look exactly like a code change: a runner that gets slower -# halfway through, or a process that happened to get an unlucky heap, shifts one -# side only. Interleaving spreads that over both sides and turns it into -# round-to-round spread, which `compare.py` can see and discount. +# Catches performance regressions: runs TPC-H SF10 for a candidate commit +# (`head`) and for the commit it sits on (`base`), on one machine, and fails +# when `head` is slower than the limits allow. The sides are measured +# interleaved, a pass each per round, because measuring one after the other +# lets machine drift read as a code change. name: Benchmarks @@ -39,16 +27,13 @@ concurrency: group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} cancel-in-progress: true -# On `main` this runs on every push, so each merge is measured against the -# commit it landed on and a regression that no PR run caught is still pinned to -# one merge. On a PR it is opt-in, because the builds and the benchmark runs add -# up to roughly half an hour of runner time: add the `performance` label, or -# start it by hand from the Actions tab. +# Every push to `main`, so an unlabelled regression is still pinned to one +# merge. Opt-in on a PR, at half an hour of runner time: add the `performance` +# label, or start it from the Actions tab. on: push: branches: - # The default branch upstream. A fork that calls it something else has to - # add that name here for its own merges to be measured. + # A fork whose default branch is named differently has to add it here. - main paths-ignore: - "docs/**" @@ -98,28 +83,21 @@ permissions: contents: read env: - # `release-nonlto` is `release` with `lto = false` and 16 codegen units. Fat - # LTO with a single codegen unit roughly doubles the build, and both sides are - # built identically, so the ratio the gate looks at still holds. Dispatch with - # `release` when a change is expected to interact with cross-crate inlining, - # or to get numbers comparable with locally posted `bench.sh` results. + # `release-nonlto` is `release` without fat LTO, which roughly halves the + # build. Dispatch with `release` for cross-crate inlining effects, or for + # numbers comparable with locally posted `bench.sh` results. CARGO_PROFILE: ${{ inputs.profile || 'release-nonlto' }} SCALE_FACTOR: ${{ inputs.scale_factor || '10' }} - # Even, for two reasons: each side then leads the same number of rounds, and - # the median of an even number of ratios averages the two middle rounds - # instead of resting on one. + # Even, so each side leads the same number of rounds. ROUNDS: ${{ inputs.rounds || '6' }} ITERATIONS: ${{ inputs.iterations || '1' }} QUERY_REGRESSION: ${{ inputs.query_regression || '1.20' }} TOTAL_REGRESSION: ${{ inputs.total_regression || '1.05' }} - # A 1.20x swing on a query that runs for 20ms is 4ms, which this kind of - # runner cannot resolve. Regressions have to cost real time to count. + # A 1.20x swing on a 20ms query is 4ms, below what a shared runner resolves. MIN_DELTA_MS: ${{ inputs.min_delta_ms || '25' }} # `benchmark_runner` finds `sql_benchmarks` through the CARGO_MANIFEST_DIR - # baked into it at compile time, so a binary built in one job only works in - # another if its tree sits at the same absolute path there. Every job below - # puts the two trees under this root, which is why it is a fixed path rather - # than something derived from the workspace or the runner. + # baked in at compile time, so its tree has to sit at the same absolute path + # in the job that builds it and the job that runs it -- hence a fixed root. BENCH_ROOT: /tmp/df-bench # Same cargo network settings as .github/actions/setup-rust-runtime, without # its RUSTFLAGS: benchmark binaries are built with the defaults. @@ -141,9 +119,7 @@ jobs: - uses: runs-on/action@46910bf61b41721b0579f237e186afb35477007a # v2.3.0 - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - # For `pull_request` this is the PR already merged into the base - # branch, for `push` the new branch tip; depth 2 is enough to also - # reach the first parent in both cases. + # Depth 2 reaches the first parent, which is the base in both cases. fetch-depth: 2 - name: Resolve base commit id: base @@ -152,16 +128,13 @@ jobs: run: | set -euo pipefail if [ "$GITHUB_EVENT_NAME" = "push" ]; then - # A merge that just landed on the default branch. Its first parent - # is the branch as it was before, whether the merge was squashed - # into one commit or kept as a merge commit, so the difference is - # attributable to this one merge. + # A merge that just landed: its first parent is the branch as it + # was before, whether the merge was squashed or not. base_sha=$(git rev-parse "HEAD^1") echo "push to $GITHUB_REF_NAME, comparing its tip against the parent" elif [ "$(git rev-list --parents -n 1 HEAD | wc -w)" -ge 3 ]; then # HEAD is the PR merged into the base branch, so its first parent - # is the base branch tip that the merge used -- the commit this PR - # would actually land on. + # is the commit this PR would land on. base_sha=$(git rev-parse "HEAD^1") else # Manual run on a branch: compare it as-is against the base tip. @@ -190,10 +163,8 @@ jobs: with: fetch-depth: 2 - name: Free Disk Space (Ubuntu) - # Two minutes of deleting Android SDKs, which a release build of the - # workspace genuinely needs on `ubuntu-latest`'s 14GB. The RunsOn - # runner above asks for `disk=large` and has no such problem, so only - # the fallback pays for this. + # A release build needs this on `ubuntu-latest`'s 14GB, but not on the + # RunsOn runner's `disk=large`, where it is two wasted minutes. if: vars.USE_RUNS_ON != 'true' uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be # v1.3.1 - name: Install Rust @@ -216,15 +187,8 @@ jobs: git worktree add --detach "$BENCH_ROOT/$SIDE" HEAD fi - name: Cache the dependency build - # A cold build of this is fourteen minutes, most of it dependencies - # that neither side changed. The action drops workspace crates from - # what it saves, so a hit rebuilds DataFusion and reuses the rest. - # - # `workspaces` because the build happens in a worktree outside the - # checkout, and one shared key for both sides because they are a commit - # or two apart and share a dependency graph. Only a push to `main` - # writes the cache: pull request runs would each save a near-identical - # copy of it. + # A cold build is fourteen minutes, mostly dependencies neither side + # changed. One shared key covers both sides; only pushes write it. uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 with: workspaces: ${{ env.BENCH_ROOT }}/${{ matrix.side }} @@ -244,7 +208,7 @@ jobs: path: ${{ runner.temp }}/benchmark_runner retention-days: 1 - # Both sides are measured here, on this one machine, back to back. + # Both sides are measured here, interleaved, on this one machine. benchmark: name: TPC-H (head vs base) needs: [resolve, build] @@ -280,8 +244,7 @@ jobs: - name: Check both runners see their queries run: | # Artifacts do not carry the executable bit, and a binary whose tree - # is missing at the path baked into it would discover no benchmark at - # all -- fail here rather than three steps later. + # is missing would discover no benchmark at all -- fail here. for side in base head; do binary="$RUNNER_TEMP/bin/$side/benchmark_runner" chmod +x "$binary" @@ -295,16 +258,9 @@ jobs: uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0 - name: Generate TPC-H data # Same generator settings as `bench.sh data tpch`, inlined because that - # path also downloads the expected answers through `docker run -it`, - # which needs a TTY and is only used for result validation. - # - # tpchgen-cli comes from its PyPI wheel: the same 3.0.0 release as the - # crate, but a ~4MB download instead of a build from source, since the - # project attaches no binaries to its GitHub releases for a prebuilt - # fetch to find. It also means this job needs no Rust toolchain at all. - # - # `parquet` as a subcommand rather than `--format parquet`: 3.0.0 - # deprecated the flag form and warns that it goes away in 4.0.0. + # path also pulls the expected answers through `docker run -it`, which + # has no TTY here. From the PyPI wheel, so no Rust toolchain is needed; + # `parquet` is a subcommand because 3.0.0 deprecated `--format`. run: | mkdir -p "$DATA_DIR/tpch_sf$SCALE_FACTOR" "$RESULTS_DIR/base" "$RESULTS_DIR/head" uv tool run --from 'tpchgen-cli==3.0.0' tpchgen-cli parquet \ @@ -315,20 +271,15 @@ jobs: du -sh "$DATA_DIR/tpch_sf$SCALE_FACTOR" df -h "$DATA_DIR" - name: Describe the machine - # A comparison is only as good as the machine under it, and this does - # not always land on the machine the `runs-on` line above asks for. - # GitHub withholds `vars` from workflows triggered by a pull request - # from a fork, so `vars.USE_RUNS_ON` reads as empty there however it is - # set on the repository, and every job falls back to a shared 4-vCPU - # `ubuntu-latest` -- which is where this workflow's own false Q02 - # regressions were measured. Pushes to `main` and manual dispatches run - # in the repository's own context and do get the 16-vCPU runner. + # GitHub withholds `vars` from fork pull requests, so `USE_RUNS_ON` + # reads as empty and they fall back to a 4-vCPU `ubuntu-latest`. Pushes + # to `main` and manual dispatches do get the 16-vCPU runner. run: | echo "cpus: $(nproc)" lscpu | grep -E '^(Model name|Socket|Core|Thread|CPU\(s\)):' || true free -h if [ "$(nproc)" -lt 8 ]; then - echo "::warning::running on $(nproc) CPUs, not the runner this workflow asks for; expect a high noise floor. Fork pull requests cannot reach the larger runner -- the run on \`main\` after the merge is the authoritative one." + echo "::warning::running on $(nproc) CPUs, not the runner asked for; expect a high noise floor. Fork pull requests cannot reach the larger one -- the run on \`main\` after the merge is authoritative." fi # Each side runs from its own tree and reads the one generated dataset. - name: Benchmark both sides, interleaved @@ -344,18 +295,14 @@ jobs: --path "$DATA_DIR" \ --output "$output" } - # Read the data once first, so no measured round is the only one - # paying to pull the parquet files into the page cache. Reading the - # files is all a warmup can carry between rounds -- each round is a - # fresh process -- and it takes seconds where a discarded pass of the - # suite took as long as a round. + # Pull the data into the page cache, which is all a warmup can carry + # between rounds -- each round is a fresh process. echo "::group::warm the page cache" find "$DATA_DIR" -type f -exec cat {} + > /dev/null echo "::endgroup::" for round in $(seq 1 "$ROUNDS"); do - # Alternate which side goes first, so with an even ROUNDS - # whatever the first position costs -- or saves -- is paid by each - # side the same number of times. + # Alternate the leader, so each side pays the first-position cost + # the same number of times. if [ $((round % 2)) -eq 1 ]; then order="base head"; else order="head base"; fi for side in $order; do echo "::group::round $round: $side" diff --git a/benchmarks/README.md b/benchmarks/README.md index 8438ef78d2f2b..2c97777f0cac2 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -260,105 +260,63 @@ Note: if `gh` is installed, you can also run `gh pr checkout $PR_NUMBER` instead ### In CI The `Benchmarks` workflow (`.github/workflows/benchmark.yml`) runs TPC-H SF10 -twice on the same runner -- once for a candidate commit (`head`) and once for -the commit it sits on (`base`) -- and fails when `head` is slower than the -configured limits allow. SF10 rather than SF1, because SF1 queries finish in -milliseconds, where runner noise is a large fraction of the measurement. - -It runs on **every push to `main`**, with `base` being the commit the merge -landed on, so a regression that no pull request measured is still pinned to the -one merge that introduced it. On a **pull request** it is opt-in, because the -builds plus the benchmark runs take a good half hour: - -- add the `performance` label to a PR, whose `head` is then the PR merged into - its base branch, or -- start it from the Actions tab (`workflow_dispatch`), where the scale factor, - round and iteration counts, all three regression limits, and the cargo - profile can be overridden - -A failure on `main` blocks nothing -- there is no PR left to hold up -- so read -it as a bisect that has already been done for you: the run names the commit and -the queries, and the uploaded rounds show how solid the verdict is. - -Which machine the run lands on follows from the trigger, and it matters more -than any of the limits below. GitHub withholds `vars` from workflows triggered -by a pull request from a fork, so `vars.USE_RUNS_ON` reads as empty there no -matter how the repository has it set, and the run falls back to a shared 4-vCPU -`ubuntu-latest` -- the same fallback the whole of `rust.yml` takes on fork pull -requests. A push to `main` or a manual dispatch runs in the repository's own -context and gets the 16-vCPU runner the workflow asks for. So a labelled fork -pull request is a coarse signal, and the run on `main` right after the merge is -the measurement; the benchmark job prints its CPU count and warns when it is -on the fallback. - -One consequence of the faster machine: SF10 queries that take 300ms on four -vCPUs take under 100ms on sixteen, and a 20% regression on those is inside the -25ms floor described below, so the four shortest queries stop being gated. -Dispatch with a smaller `min_delta_ms`, or a larger scale factor, when those -are the queries in question. - -The workflow is three jobs: one resolves the base commit, two build a -`benchmark_runner` each (one runner per side, so the builds really are -simultaneous), and the last one measures both binaries on a single machine, -because timings taken on two different machines cannot be compared. - -That last job measures the two sides **interleaved**: a full pass of one side, -then a full pass of the other, six times, alternating which side goes first. -This matters more than any choice of statistic. Measuring all of one side and -then all of the other makes anything that drifts between the two blocks -- a -runner that slows down halfway through, a process that drew an unlucky heap -- -look exactly like a code change, because it moves one side only. Interleaving -turns it into round-to-round spread instead, which `compare.py` can see and -report. The round count is kept even so each side leads the same number of -times; dispatch with more rounds to tighten a marginal verdict. - -Both sides are built with the `release-nonlto` profile (`release`, but with -`lto = false` and 16 codegen units), which together with the split is what -keeps this to half an hour -- fat LTO with a single codegen unit roughly -doubles a build. Both sides are built identically, so the ratio the gate looks -at still holds. Dispatch the workflow with the `release` profile when a change -is expected to interact with cross-crate inlining, or when the absolute numbers -should line up with locally posted `bench.sh` results. - -Shipping a binary between jobs has one catch worth knowing about if you edit -the workflow: `benchmark_runner` locates `sql_benchmarks` through the -`CARGO_MANIFEST_DIR` baked into it at compile time, so each side's checkout has -to sit at the same absolute path in the job that builds it and in the job that -runs it. The workflow puts both under a fixed `/tmp` root for that reason, and -checks that each binary can still see the `tpch` suite before it starts -measuring. - -The comparison table is written to the job summary, and every round's result -JSON plus the table are uploaded as the `tpch-comparison` artifact. - -The data is generated with the same `tpchgen-cli` settings `bench.sh data tpch` -uses, from the tool's PyPI wheel rather than a source build, which keeps a Rust -toolchain out of the benchmark job entirely. Generating SF10 takes well under a -minute; what the workflow's half hour actually goes on is the two builds, so -they restore a dependency cache and skip the `ubuntu-latest` disk cleanup when -the runner has the disk already. +for a candidate commit (`head`) and for the commit it sits on (`base`), on one +machine, and fails when `head` is slower than the limits allow. SF10 rather +than SF1, because SF1 queries finish in milliseconds where runner noise +dominates. + +It runs on every push to `main`, so a regression no pull request measured is +still pinned to the one merge that introduced it. On a pull request it is +opt-in, at half an hour of runner time: add the `performance` label, or start it +from the Actions tab, where the scale factor, round and iteration counts, the +regression limits and the cargo profile can be overridden. A failure on `main` +blocks nothing, so read it as a bisect already done for you. + +Three jobs: one resolves the base commit, two build a `benchmark_runner` each +(a runner per side, so the builds are simultaneous), and the last measures both +binaries. Both sides use the `release-nonlto` profile, which roughly halves the +build; dispatch with `release` for cross-crate inlining effects or for numbers +comparable with locally posted `bench.sh` results. + +The measuring job **interleaves** the sides: a pass of one, a pass of the other, +six times, alternating which leads. This matters more than any choice of +statistic -- measured in two blocks, anything that drifts between them moves one +side only and reads as a code change. The round count is even so each side leads +equally; dispatch more rounds to tighten a marginal verdict. + +Which machine the run lands on follows from the trigger. GitHub withholds `vars` +from fork pull requests, so `vars.USE_RUNS_ON` reads as empty there and the run +falls back to a 4-vCPU `ubuntu-latest`, as the rest of `rust.yml` also does; a +push to `main` or a manual dispatch gets the 16-vCPU runner. So a labelled fork +pull request is a coarse signal and the `main` run after the merge is the +measurement. On that faster machine an SF10 query that takes 300ms on four vCPUs +takes under 100ms, and a 20% regression on it falls inside the 25ms floor below, +so dispatch a smaller `min_delta_ms` when the shortest queries are the ones in +question. + +If you edit the workflow, note that `benchmark_runner` locates `sql_benchmarks` +through the `CARGO_MANIFEST_DIR` baked in at compile time, so each side's +checkout has to sit at the same absolute path in the job that builds it and the +job that runs it -- hence the fixed `/tmp` root, and the check that each binary +still sees the `tpch` suite before measuring. The comparison table goes to the +job summary, and every round's JSON plus the table are uploaded as the +`tpch-comparison` artifact. Runners are shared machines, so treat the numbers as a signal rather than a -measurement. A query has to clear three separate bars to fail the gate, and -each one is there because it was seen firing on its own: - -- the **median of its per-round ratios** is above `1.20x`. The median, not the - point ratio, so one slow pass on either side cannot decide the verdict. -- the regression costs at least **25ms**. A `1.20x` swing on a query that runs - for 20ms is 4ms, which no shared runner can resolve -- this is what keeps an - SF1 run from failing on its short queries. -- it is larger than the **spread the base side showed against itself** across - the rounds. If the same binary varied by 26% from round to round, a 25% - difference against the other binary is not a finding, and is reported as - inconclusive instead of blamed on the PR. - -The total time is gated at `1.05x` under the same noise floor. Everything that -clears the first bar but not the others is printed under "Not counted against -the gate", so a marginal result is visible rather than silently dropped. +measurement. A query has to clear three bars to fail the gate: + +- the **median of its per-round ratios** is above `1.20x`, so one slow pass on + either side cannot decide the verdict +- the regression costs at least **25ms**, which is what keeps an SF1 run from + failing on its short queries +- it is larger than the **spread the base side showed against itself**, since a + query whose own baseline moved 26% between rounds cannot support a 25% verdict -Local runs on quiet hardware remain the way to confirm a result, and a PR that -changes the TPC-H benchmark files themselves is measured against its own -queries, which makes the comparison advisory. +The total time is gated at `1.05x` under the same noise floor. Anything that +clears the first bar but not the others is printed under "Not counted against +the gate" rather than dropped. Local runs on quiet hardware remain the way to +confirm a result, and a PR that changes the TPC-H benchmark files themselves is +measured against its own queries, which makes the comparison advisory. ### Running Benchmarks Manually @@ -412,14 +370,12 @@ exits successfully. ``` Either path can also be a *directory* of summary files, one per measurement -round, which is how CI runs it. Rounds are paired between the two sides in -sorted filename order, so name them `round01.json`, `round02.json` and so on. -Given several rounds, the gate rules on the median of the per-round ratios -rather than on a single ratio, and reports how much each side varied against -itself -- see [In CI](#in-ci) for why that is worth the trouble. +round, which is how CI runs it: the gate then rules on the median of the +per-round ratios and reports how much each side varied against itself. Rounds +are paired in sorted filename order, so name them `round01.json` and so on. ```shell -# five rounds per side, measured alternately rather than one side at a time +# six rounds per side, measured alternately rather than one side at a time ./compare.py /tmp/output_main /tmp/output_branch \ --fail-threshold 1.20 --fail-total-threshold 1.05 --fail-min-delta-ms 25 ``` diff --git a/benchmarks/compare.py b/benchmarks/compare.py index 629d841c26185..ab7f4db21b903 100755 --- a/benchmarks/compare.py +++ b/benchmarks/compare.py @@ -51,11 +51,9 @@ def geometric_mean(values: Sequence[float]) -> float: def upward_spread(values: Sequence[float]) -> float: - """How far the typical measurement sits above the fastest one. + """`(median - min) / min`: the slowdown a side already shows against itself. - `(median - min) / min`, the noise floor of a single side: the relative - slowdown that this side already shows against itself. The median rather - than the max, so one hiccup in one round does not define the floor. + The median rather than the max, so one slow round does not set the floor. """ fastest = min(values) if fastest <= 0: @@ -188,11 +186,8 @@ def load_from_file(cls, path: Path) -> BenchmarkRun: class Side: """One side of the comparison: every round measured for it. - A single summary file is one round; a directory of summary files is one - round per file, taken in sorted order. Measuring several rounds and - interleaving them with the other side is what makes the comparison - trustworthy: a machine that slows down halfway through then shifts both - sides, instead of only the one that happened to run at that moment. + A single summary file is one round; a directory is one round per file, in + sorted order. """ header: str @@ -213,9 +208,8 @@ def load(cls, path: Path) -> Side: rounds = [BenchmarkRun.load_from_file(round_path) for round_path in paths] - # Every round is indexed by position below, so a round that measured a - # different set of queries has to be caught here rather than silently - # compared query against query. + # Rounds are indexed by position below, so a round that measured a + # different set of queries has to be caught rather than mispaired. queries = [query.query for query in rounds[0].queries] for round_path, round in zip(paths[1:], rounds[1:]): if [query.query for query in round.queries] != queries: @@ -329,8 +323,7 @@ def compare( table.add_column(comparison.header, justify="right", style="dim", no_wrap=True) table.add_column("Change", justify="right", style="dim", no_wrap=True) if multi_round: - # The gate rules on the median of the per-round ratios, so show it - # next to the fastest-run ratio the `Change` column reports. + # What the gate rules on, next to the fastest-run ratio in `Change`. table.add_column("Per-round", justify="right", style="dim", no_wrap=True) table.add_column("Noise", justify="right", style="dim", no_wrap=True) @@ -340,8 +333,7 @@ def compare( failure_count = 0 total_baseline_time = 0 total_comparison_time = 0 - # Per-round totals over the queries that ran on both sides, so the total - # is gated the same paired way a single query is + # Per-round totals, so the total is gated the same paired way a query is baseline_totals = [0.0] * rounds comparison_totals = [0.0] * rounds comparisons: List[QueryComparison] = [] @@ -506,12 +498,10 @@ def report_regressions( ) -> int: """Report against the configured limits, returning the process exit code. - A query only fails the gate when its slowdown clears three bars: the - configured limit, the noise floor the baseline showed against itself, and - a minimum absolute cost in milliseconds. Anything that clears the first - but not the others is printed as inconclusive -- the machine could not - measure it that precisely, which is a fact about the run and not about the - change under test. + A query fails only when its slowdown clears three bars: the configured + limit, a minimum absolute cost in milliseconds, and the noise floor the + baseline showed against itself. Clearing the first but not the others is + reported as inconclusive -- a fact about the run, not about the change. """ if fail_threshold is None and fail_total_threshold is None: return 0