-
Notifications
You must be signed in to change notification settings - Fork 60
Establish performance baselines and regression detection #3441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
9cd5933
1ba4889
df4f09d
7594bbf
d39ae20
dcc3e24
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -197,6 +197,29 @@ benchmark_data: benchmark/simple/data.tar.gz ## Prepare data for benchmark | |||||||||||||||
| .PHONY: benchmark | ||||||||||||||||
| benchmark: benchmark_simple ## Run benchmarks | ||||||||||||||||
|
|
||||||||||||||||
|
dheerajodha marked this conversation as resolved.
|
||||||||||||||||
| .PHONY: generate-baseline | ||||||||||||||||
|
dheerajodha marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] inline-scripting-convention The generate-baseline target embeds ~15 lines of inline Python. The codebase convention is to use standalone scripts for complex logic (e.g., prepare_data.sh, push_data.sh).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bot says: Fair suggestion for a follow-up but out of scope for this PR. The inline python is ~10 lines and only used by this one target. The sibling scripts (prepare_data.sh, push_data.sh) are reused across Make and CI, which justified extraction. Open to hear if anyone who breathes oxygen has an opinion. |
||||||||||||||||
| generate-baseline: benchmark/stress/data.tar.gz ## Generate stress benchmark baseline | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] naming-convention The generate-baseline target uses hyphens while the benchmark_% pattern targets use underscores. The Makefile uses both conventions broadly, but this is inconsistent within the benchmark target family.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed in prior rounds. The Makefile uses underscores for pattern-rule targets (benchmark_%, feature_%, scenario_%) and hyphens for standalone targets (dist-container, build-for-test, lint-fix). I'm open to hear what someone, who breathes air, thinks about this. |
||||||||||||||||
| @cd benchmark/stress && \ | ||||||||||||||||
| EC_STRESS_COMPONENTS=$${EC_STRESS_COMPONENTS:-10} EC_STRESS_WORKERS=$${EC_STRESS_WORKERS:-10} \ | ||||||||||||||||
| go run . 2>benchmark-stderr.txt | tee benchmark-output.txt && \ | ||||||||||||||||
|
Comment on lines
+202
to
+204
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Preserve the benchmark process exit status.
Capture output directly before parsing it, or run the pipeline through Bash with Proposed fix- EC_STRESS_COMPONENTS=$${EC_STRESS_COMPONENTS:-10} EC_STRESS_WORKERS=$${EC_STRESS_WORKERS:-10} \
- go run . 2>benchmark-stderr.txt | tee benchmark-output.txt && \
+ EC_STRESS_COMPONENTS=$${EC_STRESS_COMPONENTS:-10} EC_STRESS_WORKERS=$${EC_STRESS_WORKERS:-10} \
+ go run . >benchmark-output.txt 2>benchmark-stderr.txt && \
+ cat benchmark-output.txt && \📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
| python3 -c "\ | ||||||||||||||||
| import re, json, sys; \ | ||||||||||||||||
|
dheerajodha marked this conversation as resolved.
|
||||||||||||||||
| line = [l for l in open('benchmark-output.txt') if l.startswith('BenchmarkStress')]; \ | ||||||||||||||||
| line or sys.exit('No BenchmarkStress results found'); \ | ||||||||||||||||
| line = line[0]; \ | ||||||||||||||||
| def val(p): \ | ||||||||||||||||
| m = re.search(p, line); \ | ||||||||||||||||
| return m.group(1) if m else ''; \ | ||||||||||||||||
| ns = val(r'([\d.]+)\s+ns/op'); rss = val(r'([\d.]+)\s+peak-RSS-bytes'); \ | ||||||||||||||||
| (ns and rss) or sys.exit('Failed to parse benchmark metrics'); \ | ||||||||||||||||
| json.dump({'peak_rss_bytes': int(float(rss)), 'ns_per_op': int(float(ns)), \ | ||||||||||||||||
| 'components': int('$${EC_STRESS_COMPONENTS:-10}'), 'workers': int('$${EC_STRESS_WORKERS:-10}'), \ | ||||||||||||||||
| 'commit': '$(shell git rev-parse --short HEAD)', 'date': '$(shell date -u +%Y-%m-%d)', \ | ||||||||||||||||
| 'go_version': '$(shell go env GOVERSION | sed "s/^go//")' \ | ||||||||||||||||
| }, open('baseline.json','w'), indent=2); print()" && \ | ||||||||||||||||
| rm -f benchmark-output.txt benchmark-stderr.txt && \ | ||||||||||||||||
| echo "Baseline written to benchmark/stress/baseline.json" | ||||||||||||||||
|
|
||||||||||||||||
| .PHONY: tools-ci | ||||||||||||||||
| tools-ci: ## Ensure all tools build cleanly | ||||||||||||||||
| @echo "• tkn:" && \ | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "peak_rss_bytes": 2250485760, | ||
| "ns_per_op": 2567888013, | ||
| "components": 10, | ||
| "workers": 10, | ||
| "commit": "fc37eb13", | ||
| "date": "2026-08-11", | ||
| "go_version": "1.26.3" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| #!/bin/bash | ||
| # Copyright The Conforma Contributors | ||
| # | ||
| # Licensed 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. | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| # Compares current benchmark results against a stored baseline and exits | ||
| # non-zero if any metric regresses beyond the configured threshold. | ||
| set -o errexit | ||
| set -o nounset | ||
| set -o pipefail | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| BASELINE="${SCRIPT_DIR}/baseline.json" | ||
| THRESHOLDS="${SCRIPT_DIR}/thresholds.json" | ||
| BENCHMARK_OUTPUT="${1:-${SCRIPT_DIR}/benchmark-output.txt}" | ||
|
|
||
| if [[ ! -f "$BASELINE" ]]; then | ||
| echo "No baseline found, skipping comparison." | ||
| exit 0 | ||
| fi | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] edge-case grep BenchmarkStress captures all matching lines into $line. While Pythons re.search handles this correctly, using grep -m1 would make the single-line intent explicit. |
||
|
|
||
| if [[ ! -f "$THRESHOLDS" ]]; then | ||
| echo "No thresholds file found, skipping comparison." | ||
| exit 0 | ||
| fi | ||
|
dheerajodha marked this conversation as resolved.
|
||
|
|
||
| if [[ ! -f "$BENCHMARK_OUTPUT" ]]; then | ||
| echo "No benchmark output found at ${BENCHMARK_OUTPUT}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| line=$(grep '^BenchmarkStress' "$BENCHMARK_OUTPUT" || true) | ||
| if [[ -z "$line" ]]; then | ||
|
dheerajodha marked this conversation as resolved.
|
||
| echo "No BenchmarkStress results found in output." | ||
| exit 1 | ||
| fi | ||
|
dheerajodha marked this conversation as resolved.
|
||
|
|
||
| read -r current_ns current_rss baseline_ns baseline_rss threshold_rss threshold_time < <( | ||
| BENCH_LINE="${line}" BASELINE_PATH="${BASELINE}" THRESHOLDS_PATH="${THRESHOLDS}" python3 -c " | ||
|
dheerajodha marked this conversation as resolved.
|
||
| import json, os, re, sys | ||
| line = os.environ['BENCH_LINE'] | ||
| def extract(pattern): | ||
| m = re.search(pattern, line) | ||
| return m.group(1) if m else '' | ||
| ns = extract(r'([\d.]+)\s+ns/op') | ||
| rss = extract(r'([\d.]+)\s+peak-RSS-bytes') | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] naming-convention The Python helper function is named extract() in compare.sh but val() in the Makefile and workflow. All three serve the identical purpose. |
||
| if not ns or not rss: | ||
| print('Failed to parse benchmark metrics from output.', file=sys.stderr) | ||
| sys.exit(1) | ||
| b = json.load(open(os.environ['BASELINE_PATH'])) | ||
| t = json.load(open(os.environ['THRESHOLDS_PATH'])) | ||
| print(ns, rss, b['ns_per_op'], b['peak_rss_bytes'], t['peak_rss_percent'], t['ns_per_op_percent']) | ||
|
Comment on lines
+50
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift Enforce the benchmark workload before comparing metrics.
📍 Affects 3 files
🤖 Prompt for AI Agents |
||
| " | ||
|
dheerajodha marked this conversation as resolved.
|
||
| ) | ||
|
|
||
| if awk -v rss="$baseline_rss" -v ns="$baseline_ns" 'BEGIN {exit !(rss==0 || ns==0)}'; then | ||
|
dheerajodha marked this conversation as resolved.
|
||
| echo "Baseline contains zero values, cannot compute regression." | ||
|
dheerajodha marked this conversation as resolved.
|
||
| exit 1 | ||
| fi | ||
|
|
||
| rss_change=$(awk -v cur="$current_rss" -v base="$baseline_rss" 'BEGIN {printf "%.1f", ((cur - base) / base) * 100}') | ||
| time_change=$(awk -v cur="$current_ns" -v base="$baseline_ns" 'BEGIN {printf "%.1f", ((cur - base) / base) * 100}') | ||
|
|
||
| baseline_rss_mb=$(awk -v val="$baseline_rss" 'BEGIN {printf "%.0f", val / 1048576}') | ||
| current_rss_mb=$(awk -v val="$current_rss" 'BEGIN {printf "%.0f", val / 1048576}') | ||
| baseline_secs=$(awk -v val="$baseline_ns" 'BEGIN {printf "%.1f", val / 1000000000}') | ||
| current_secs=$(awk -v val="$current_ns" 'BEGIN {printf "%.1f", val / 1000000000}') | ||
|
|
||
| echo "" | ||
| echo "=== Benchmark Comparison ===" | ||
| echo "" | ||
| printf "%-20s %10s %10s %10s %10s\n" "Metric" "Baseline" "Current" "Change" "Threshold" | ||
| printf "%-20s %10s %10s %9s%% %9s%%\n" "Peak RSS" "${baseline_rss_mb} MB" "${current_rss_mb} MB" "$rss_change" "$threshold_rss" | ||
| printf "%-20s %10s %10s %9s%% %9s%%\n" "Execution time" "${baseline_secs}s" "${current_secs}s" "$time_change" "$threshold_time" | ||
| echo "" | ||
|
|
||
| failed=0 | ||
|
|
||
| rss_exceeded=$(awk -v change="$rss_change" -v thresh="$threshold_rss" 'BEGIN {print (change > thresh) ? 1 : 0}') | ||
| time_exceeded=$(awk -v change="$time_change" -v thresh="$threshold_time" 'BEGIN {print (change > thresh) ? 1 : 0}') | ||
|
|
||
| if [[ "$rss_exceeded" == "1" ]]; then | ||
| echo "FAIL: Peak RSS regressed by ${rss_change}% (threshold: ${threshold_rss}%)" | ||
| failed=1 | ||
| fi | ||
|
|
||
| if [[ "$time_exceeded" == "1" ]]; then | ||
| echo "FAIL: Execution time regressed by ${time_change}% (threshold: ${threshold_time}%)" | ||
| failed=1 | ||
| fi | ||
|
|
||
| if [[ "$failed" == "0" ]]; then | ||
| echo "PASS: No regressions detected." | ||
| fi | ||
|
|
||
| exit "$failed" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| { | ||
| "peak_rss_percent": 15, | ||
| "ns_per_op_percent": 20 | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.