From 0f51d7bf7956d9dbdbf4f5ac54a2214773cb0a69 Mon Sep 17 00:00:00 2001 From: Phil Rzewski Date: Sat, 17 Oct 2026 22:11:29 -0700 Subject: [PATCH 1/2] Canary test to catch perf regressions --- .github/workflows/perf-canary.yaml | 169 +++++++++ Makefile | 8 + perf/canary/README.md | 162 +++++++++ perf/canary/main.go | 528 +++++++++++++++++++++++++++++ perf/canary/queries/agg.sql | 5 + perf/canary/queries/count.sql | 3 + perf/canary/queries/search+.spq | 3 + perf/canary/queries/search.spq | 3 + perf/canary/queries/union.spq | 6 + perf/canary/report.go | 371 ++++++++++++++++++++ perf/canary/rusage_darwin.go | 4 + perf/canary/rusage_other.go | 10 + perf/canary/rusage_other_unix.go | 6 + perf/canary/rusage_unix.go | 16 + 14 files changed, 1294 insertions(+) create mode 100644 .github/workflows/perf-canary.yaml create mode 100644 perf/canary/README.md create mode 100644 perf/canary/main.go create mode 100644 perf/canary/queries/agg.sql create mode 100644 perf/canary/queries/count.sql create mode 100644 perf/canary/queries/search+.spq create mode 100644 perf/canary/queries/search.spq create mode 100644 perf/canary/queries/union.spq create mode 100644 perf/canary/report.go create mode 100644 perf/canary/rusage_darwin.go create mode 100644 perf/canary/rusage_other.go create mode 100644 perf/canary/rusage_other_unix.go create mode 100644 perf/canary/rusage_unix.go diff --git a/.github/workflows/perf-canary.yaml b/.github/workflows/perf-canary.yaml new file mode 100644 index 0000000000..19ed1ae990 --- /dev/null +++ b/.github/workflows/perf-canary.yaml @@ -0,0 +1,169 @@ +name: "Perf canary" + +# Compares the performance of two builds of super on the small ghasample +# data set. See perf/canary/README.md. +# +# * On a pull request, it compares the PR (merged into main) with the tip of +# main, skipping the slow super-json-static tests so it finishes in about +# the same time as the other CI. +# * On a push to main, it compares the new tip of main with the previous one, +# including all tests. +# * It can also be run manually to compare any two commits. +# +# Significant changes never fail the job. Instead, the results are reported +# in a separate "Perf canary results" check, which is neutral (gray) if +# anything regressed and successful (green) otherwise, with the details in +# the check and the job summary. + +on: + pull_request: + paths-ignore: + - "book/**" + - "**.md" + push: + branches: + - main + paths-ignore: + - "book/**" + - "**.md" + workflow_dispatch: + inputs: + base: + description: "Base commit or ref (default: merge-base of the target with main)" + target: + description: "Target commit or ref (default: the branch selected above)" + runs: + description: "Measured runs per query for each build" + default: "5" + systems: + description: "Comma-separated systems to test (default: all of super-bsup, super-json, super-json-static, super-parquet)" + +permissions: + contents: read + checks: write + +# Runs for the same PR share a group, so a new push to the PR's branch +# cancels the PR's run in progress. Every other run (a push to main or a +# manual run) gets a group of its own, so it never waits for or cancels +# another run. +concurrency: + group: perf-canary-${{ github.event_name == 'pull_request' && format('pr-{0}', github.event.pull_request.number) || format('run-{0}', github.run_id) }} + cancel-in-progress: true + +jobs: + perf-canary: + runs-on: ubuntu-24.04 + timeout-minutes: 60 + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 + - uses: actions/setup-go@v6 + with: + go-version-file: go.mod + - uses: actions/cache@v5 + with: + path: ~/.cache/super-perf-canary + key: perf-canary-data-v1 + - name: Choose what to compare + id: config + env: + EVENT: ${{ github.event_name }} + PR_URL: ${{ github.event.pull_request.html_url }} + BEFORE: ${{ github.event.before }} + INPUT_BASE: ${{ inputs.base }} + INPUT_TARGET: ${{ inputs.target }} + INPUT_RUNS: ${{ inputs.runs }} + INPUT_SYSTEMS: ${{ inputs.systems }} + run: | + base='' target='' runs=5 systems='' pr='' + case "$EVENT" in + pull_request) + # The checkout is the PR merged into main, so the default base + # (its merge-base with main) is the tip of main. The merge + # commit is GitHub's and isn't on any branch, so the report + # names the PR and its head commit too. + systems=super-bsup,super-json,super-parquet pr=$PR_URL + ;; + push) + # The default base would be the new commit itself, so compare + # with the previous tip of main instead. + if [[ "$BEFORE" =~ ^0*$ ]] || ! git cat-file -e "$BEFORE^{commit}" 2>/dev/null; then + base=HEAD^ + else + base=$BEFORE + fi + ;; + workflow_dispatch) + base=$INPUT_BASE target=$INPUT_TARGET runs=${INPUT_RUNS:-5} systems=$INPUT_SYSTEMS + ;; + esac + { + echo "base=$base" + echo "target=$target" + echo "runs=$runs" + echo "systems=$systems" + echo "pr=$pr" + } >> "$GITHUB_OUTPUT" + - name: Run perf canary + env: + BASE: ${{ steps.config.outputs.base }} + TARGET: ${{ steps.config.outputs.target }} + RUNS: ${{ steps.config.outputs.runs }} + SYSTEMS: ${{ steps.config.outputs.systems }} + PR: ${{ steps.config.outputs.pr }} + run: | + make perf BASE="$BASE" TARGET="$TARGET" SYSTEMS="$SYSTEMS" \ + PERF_CANARY_FLAGS="-runs $RUNS -datadir $HOME/.cache/super-perf-canary -out $RUNNER_TEMP/perf-canary ${PR:+-pr $PR}" + - name: Report + id: report + if: always() + run: | + out="$RUNNER_TEMP/perf-canary" + if [ ! -s "$out/results.json" ]; then + echo "::error title=Perf canary::No results were produced (see the log)" + exit 0 + fi + cat "$out/summary.md" >> "$GITHUB_STEP_SUMMARY" + title=$(head -1 "$out/summary.md" | sed 's/^## //') + case "$(jq -r .verdict "$out/results.json")" in + regressed|attention) + echo "::warning title=Perf canary::$title" + conclusion=neutral + ;; + *) + echo "::notice title=Perf canary::$title" + conclusion=success + ;; + esac + { + echo "title=$title" + echo "conclusion=$conclusion" + } >> "$GITHUB_OUTPUT" + # A pull request from a fork gets a read-only token that can't create + # checks, so its results are only in the job summary. + - name: Post "Perf canary results" check + if: always() && steps.report.outputs.title != '' && (github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository)) + env: + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + HEAD_SHA: ${{ github.event.pull_request.head.sha || github.sha }} + TITLE: ${{ steps.report.outputs.title }} + CONCLUSION: ${{ steps.report.outputs.conclusion }} + DETAILS_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + jq -n \ + --arg sha "$HEAD_SHA" \ + --arg conclusion "$CONCLUSION" \ + --arg title "$TITLE" \ + --arg url "$DETAILS_URL" \ + --rawfile summary "$RUNNER_TEMP/perf-canary/summary.md" \ + '{name: "Perf canary results", head_sha: $sha, status: "completed", + conclusion: $conclusion, details_url: $url, + output: {title: $title, summary: $summary}}' | + gh api "repos/$REPO/check-runs" --input - > /dev/null + - uses: actions/upload-artifact@v6 + if: always() + with: + name: perf-canary + path: ${{ runner.temp }}/perf-canary diff --git a/Makefile b/Makefile index 020c1c55cc..ddf097e97d 100644 --- a/Makefile +++ b/Makefile @@ -83,6 +83,14 @@ compiler/parser/parser.go: compiler/parser/parser.peg go.mod .PHONY: peg peg: compiler/parser/parser.go +# Compare the performance of two builds of super on a small data set. +# BASE defaults to the merge-base with origin/main, TARGET defaults to the +# working tree, and SYSTEMS (a comma-separated list) defaults to all of them. +# See perf/canary/README.md. +.PHONY: perf +perf: + @go run ./perf/canary $(if $(BASE),-base '$(BASE)') $(if $(TARGET),-target '$(TARGET)') $(if $(SYSTEMS),-systems '$(SYSTEMS)') $(PERF_CANARY_FLAGS) + .PHONY: markdown-lint markdown-lint: @npm install --no-save markdownlint-cli@0.35.0 diff --git a/perf/canary/README.md b/perf/canary/README.md new file mode 100644 index 0000000000..1eae1d953a --- /dev/null +++ b/perf/canary/README.md @@ -0,0 +1,162 @@ +# Perf canary + +The perf canary gives early warning of performance changes by comparing two +builds of `super` on the same machine in the same run. It loads and queries +the small "ghasample" data set (22,845 JSON values, one of each unique type +in a day of [GitHub Archive](https://www.gharchive.org/) data) with each +build and reports significant differences in BSUP load time, query time, and +peak memory. + +Because both builds run back to back on the same machine, the comparison is +meaningful even on a noisy machine such as a hosted GitHub Actions runner. +The absolute times are not comparable across runs or machines. + +## Running locally + +``` +make perf +``` + +By default this compares: + +* **base**: the merge-base of `HEAD` with `origin/main` (run `git fetch` first + so it's current), and +* **target**: the working tree, including uncommitted changes. + +Either can be set to any commit or ref: + +``` +make perf BASE=c473eb5b7 TARGET=12ac576b2 +``` + +To run faster, test only some of the systems described [below](#whats-measured) +with a comma-separated list, e.g., to skip the slowest one, +`super-json-static`, which accounts for more than half of the run time: + +``` +make perf SYSTEMS=super-bsup,super-json,super-parquet +``` + +Other options can be passed with `PERF_CANARY_FLAGS`, e.g., +`make perf PERF_CANARY_FLAGS="-keep -threshold 0.15"`. Run +`go run ./perf/canary -h` to see them all. + +A run of all the systems with the default of 5 measured runs per query +takes about 6–7 minutes on a GitHub-hosted runner, and longer on slower +machines. The test data (about 170 MB) is downloaded once and cached in the +directory given by `-datadir` (by default `super-perf-canary` under the +user cache directory, e.g., `~/.cache` on Linux or `~/Library/Caches` on +macOS). The only other requirements are `git` and `go`. + +When it finishes, it prints the report and saves a copy as `summary.md`, +along with the raw measurements (`results.json`) and each query's output. +These go in a new temporary directory, whose path is printed at the end, +or in the directory given by `-out`. + +## Running in GitHub Actions + +The perf canary [workflow](../../.github/workflows/perf-canary.yaml) runs +automatically: + +* **On pull requests**, it compares the PR (merged into `main`) with the tip + of `main`. To finish in about the same time as the other CI, it skips the + `super-json-static` tests. Like other pull request workflows, it tests + the merge commit GitHub creates to preview the result of merging the PR. + That commit isn't on any branch and is replaced when the PR or `main` + changes, so the report names the PR's own commit along with it. +* **On pushes to `main`**, it compares the new tip of `main` with the + previous one, including all the tests, so a regression in + `super-json-static` is caught as soon as it merges. + +Changes that touch only `book/` or Markdown files are skipped. + +It never fails because of a regression. Instead, it adds a separate +**Perf canary results** check to the commit, which is: + +* neutral (gray) if any test regressed, failed, or changed its output, or +* successful (green) otherwise. + +The check's title gives the verdict (e.g., "🔴 Perf canary: 2 of 16 tests +regressed") and its details show the full report, which also appears in +the job summary. The raw results are saved as an artifact. (A pull request +from a fork can't add the check, so its report is only in the job summary.) + +The workflow can also be started manually from the Actions tab to compare +any two commits. Choose the branch to test and, optionally, the base and +target commits, the number of runs, and the systems to test. + +## What's measured + +For each build, the canary: + +1. builds `super`, +2. converts the JSON data to BSUP (`super -dynamic -i json -f bsup`), + measured once, and +3. runs each of the five queries in [`queries/`](queries) against four + inputs: + + | Test | Input | Flags | + |-|-|-| + | `super-bsup` | the BSUP file written by that build | `-i bsup` | + | `super-json` | `ghasample.json.gz` | `-dynamic -i json` | + | `super-json-static` | `ghasample.json.gz` | `-static -i json` | + | `super-parquet` | `ghasample.parquet` | `-i parquet` | + +Each query is run once per build as a warmup, then the given number of +times per build, alternating between the builds (base, target, target, +base, ...) so any drift in machine performance affects both equally. The +report uses the median wall time, CPU time (user + system), and peak +resident memory of each. + +A change is flagged as significant only if all of these hold: + +* the median changes by at least the `-threshold` (default 25%), +* the median changes by at least an absolute amount (default 25 ms for + times and 20 MB for memory), so tiny, fast queries don't trigger on + noise, and +* every target run is worse than every base run (or better, for an + improvement). This matters most for peak memory, which can jump between + levels from one run to the next as the garbage collector runs at + different times. + +Because of that last rule, use at least 5 runs (the default). With fewer, +noise alone is much more likely to separate the two builds' runs. + +To try different thresholds on an earlier run without re-running it, pass +its `results.json` to `-rereport`, e.g., +`go run ./perf/canary -rereport results.json -threshold 0.15`. + +The output of each query is also compared between the builds. A difference +is flagged because it usually means the timings aren't comparable (e.g., a +query got faster because it stopped returning results). + +## What it can miss + +The defaults are deliberately cautious. Hosted runners vary from run to run, +and a check that raised false alarms would soon be ignored. In calibration +runs that compared a commit with itself on hosted runners, median times +moved by up to about 6% and median peak memory by up to about 32%, so the +rules above leave a wide margin for noise. The price is that some things can +get slower even when the check is green: + +* **Changes under the threshold aren't flagged.** In one test, a fake + 150 ms startup delay made `super-bsup search+` 23% slower (0.611 → + 0.751 s), and it wasn't flagged. +* **The fastest queries are covered by the absolute minimum rather than the + percentage.** Some `super-parquet` queries take only about 25 ms on a + hosted runner, so even doubling their time wouldn't be flagged. +* **Gradual creep isn't caught.** Each change is compared only with the + commit before it, so a series of small regressions, each under the + threshold, can add up unnoticed. +* **Coverage is narrow.** It runs five queries on one small data set in four + formats. Regressions that only show up with large data, other formats or + features (e.g., CSV, `super db`, joins), or other kinds of queries won't + be seen. Full benchmarks are still the place to catch those. +* **Pull request runs skip `super-json-static`,** so a regression there is + only reported once the change reaches `main`. + +So if you've changed code you expect to affect performance, look at the +"All results" table even when the check is green. It shows every change, +flagged or not. To look more closely, re-classify a run with a lower +threshold using `-rereport` (see above), or run the canary locally with more +runs (`-runs 10`). diff --git a/perf/canary/main.go b/perf/canary/main.go new file mode 100644 index 0000000000..6270e137fc --- /dev/null +++ b/perf/canary/main.go @@ -0,0 +1,528 @@ +// Command canary compares the performance of two builds of super by +// loading and querying the small "ghasample" data set with each build and +// reporting significant differences in load time, query time, and memory. +// See README.md for details. +package main + +import ( + "cmp" + "context" + "embed" + "encoding/json" + "errors" + "flag" + "fmt" + "io" + "log" + "net/http" + "os" + "os/exec" + "path/filepath" + "runtime" + "slices" + "sort" + "strings" + "time" +) + +const ( + jsonURL = "https://brim-benchmarks.s3.us-east-2.amazonaws.com/gha/ghasample.json.gz" + parquetURL = "https://brim-benchmarks.s3.us-east-2.amazonaws.com/gha/ghasample.parquet" +) + +//go:embed queries +var queryFS embed.FS + +// The queries in queries/, in the order they're run and reported. +var queryFiles = []string{"search.spq", "search+.spq", "count.sql", "agg.sql", "union.spq"} + +// Each system runs the queries on one input format. +type system struct { + name string + flags []string + input func(b *build, data string) string +} + +var systems = []system{ + {"super-bsup", []string{"-i", "bsup", "-f", "csv"}, func(b *build, _ string) string { return b.bsup }}, + {"super-json", []string{"-dynamic", "-i", "json", "-f", "csv"}, func(_ *build, data string) string { return filepath.Join(data, "ghasample.json.gz") }}, + {"super-json-static", []string{"-static", "-i", "json", "-f", "csv"}, func(_ *build, data string) string { return filepath.Join(data, "ghasample.json.gz") }}, + {"super-parquet", []string{"-i", "parquet", "-f", "csv"}, func(_ *build, data string) string { return filepath.Join(data, "ghasample.parquet") }}, +} + +type build struct { + Label string `json:"label"` + Rev string `json:"rev"` // commit hash, or "" for the working tree + Version string `json:"version"` // git describe output + // Merged is the git describe output for the second parent when the + // target is a merge commit whose first parent is the base, as it is + // for a pull request run in GitHub Actions. + Merged string `json:"merged,omitempty"` + commit string // commit hash of a clean working tree + bin string + bsup string +} + +type measurement struct { + Wall float64 `json:"wall"` // seconds + User float64 `json:"user"` // seconds + Sys float64 `json:"sys"` // seconds + MaxRSS int64 `json:"maxrss"` // bytes (0 if unavailable) +} + +func (m measurement) cpu() float64 { return m.User + m.Sys } + +type result struct { + Test string `json:"test"` + Base []measurement `json:"base"` + Target []measurement `json:"target"` + BaseErr string `json:"base_error,omitempty"` + TargetErr string `json:"target_error,omitempty"` + OutputMismatch bool `json:"output_mismatch,omitempty"` +} + +type config struct { + base, target, mainRef string + pr string + runs int + timeout time.Duration + threshold float64 + minDelta time.Duration + minMemDelta int64 + dataDir, outDir string + keep bool + systems []system +} + +func main() { + log.SetFlags(0) + log.SetPrefix("perf-canary: ") + var c config + cacheDir, _ := os.UserCacheDir() + flag.StringVar(&c.base, "base", "", "base commit (default: merge-base of target and -main)") + flag.StringVar(&c.target, "target", "", "target commit (default: the working tree, including uncommitted changes)") + flag.StringVar(&c.mainRef, "main", "origin/main", "ref used to compute the default base") + flag.StringVar(&c.pr, "pr", "", "URL of the pull request being tested, for the report") + flag.IntVar(&c.runs, "runs", 5, "measured runs per query for each build") + flag.DurationVar(&c.timeout, "timeout", 5*time.Minute, "timeout for each super invocation") + flag.Float64Var(&c.threshold, "threshold", 0.25, "relative change that counts as significant") + flag.DurationVar(&c.minDelta, "min-delta", 25*time.Millisecond, "minimum absolute time change that counts as significant") + minMemMB := flag.Int64("min-mem-delta", 20, "minimum absolute peak memory change in MB that counts as significant") + flag.StringVar(&c.dataDir, "datadir", filepath.Join(cacheDir, "super-perf-canary"), "directory for cached test data") + flag.StringVar(&c.outDir, "out", "", "directory for summary.md, results.json, and query outputs (default: a new temporary directory, which is kept)") + flag.BoolVar(&c.keep, "keep", false, "keep the temporary work directory") + rereportPath := flag.String("rereport", "", "reclassify the results in this results.json file instead of running") + systemNames := flag.String("systems", allSystemNames(), "comma-separated list of systems to test") + // The default usage line names the temporary binary built by "go run". + flag.Usage = func() { + fmt.Fprint(flag.CommandLine.Output(), "Usage: go run ./perf/canary [flags]\n\n"+ + "Compares the performance of two builds of super. See perf/canary/README.md.\n\n") + flag.PrintDefaults() + } + flag.Parse() + c.minMemDelta = *minMemMB << 20 + var err error + if c.systems, err = selectSystems(*systemNames); err != nil { + log.Fatal(err) + } + if *rereportPath != "" { + rep, err := rereport(c, *rereportPath) + if err != nil { + log.Fatal(err) + } + fmt.Print(rep.markdown()) + return + } + if err := run(c); err != nil { + log.Fatal(err) + } +} + +func run(c config) error { + repo, err := git("", "rev-parse", "--show-toplevel") + if err != nil { + return err + } + work, err := os.MkdirTemp("", "super-perf-canary-") + if err != nil { + return err + } + if c.keep { + log.Printf("work directory: %s", work) + } else { + defer os.RemoveAll(work) + } + if c.outDir == "" { + // Not in the work directory, so the results outlive it. + if c.outDir, err = os.MkdirTemp("", "super-perf-canary-results-"); err != nil { + return err + } + } + if err := os.MkdirAll(filepath.Join(c.outDir, "outputs"), 0755); err != nil { + return err + } + base, target, err := resolveBuilds(repo, c) + if err != nil { + return err + } + if err := fetchData(c.dataDir); err != nil { + return err + } + for _, b := range []*build{base, target} { + if err := buildSuper(repo, work, b); err != nil { + return err + } + } + var results []*result + // The BSUP "load" is measured once per build. + load := &result{Test: "super-bsup load"} + for _, b := range []*build{base, target} { + if !c.has("super-bsup") { + break + } + b.bsup = filepath.Join(work, b.Label+".bsup") + log.Printf("loading BSUP with %s", b.Label) + m, _, err := measure(c.timeout, b.bin, "-dynamic", "-i", "json", "-f", "bsup", "-o", b.bsup, filepath.Join(c.dataDir, "ghasample.json.gz")) + if err != nil { + return fmt.Errorf("BSUP load with %s build: %w", b.Label, err) + } + if b == base { + load.Base = append(load.Base, m) + } else { + load.Target = append(load.Target, m) + } + } + if c.has("super-bsup") { + results = append(results, load) + } + for _, sys := range c.systems { + for _, qf := range queryFiles { + r, err := runQuery(c, work, sys, qf, base, target) + if err != nil { + return err + } + results = append(results, r) + } + } + rep := newReport(c, base, target, results) + if err := rep.write(c.outDir); err != nil { + return err + } + fmt.Print(rep.markdown()) + log.Printf("report written to %s", filepath.Join(c.outDir, "summary.md")) + log.Printf("raw results and query outputs are in %s", c.outDir) + return nil +} + +func allSystemNames() string { + return strings.Join(systemNamesOf(systems), ",") +} + +func selectSystems(list string) ([]system, error) { + var selected []system + for _, name := range strings.Split(list, ",") { + name = strings.TrimSpace(name) + if name == "" { + continue + } + i := slices.IndexFunc(systems, func(sys system) bool { return sys.name == name }) + if i < 0 { + return nil, fmt.Errorf("unknown system %q (choose from %s)", name, allSystemNames()) + } + if !slices.ContainsFunc(selected, func(sys system) bool { return sys.name == name }) { + selected = append(selected, systems[i]) + } + } + if len(selected) == 0 { + return nil, errors.New("no systems selected") + } + return selected, nil +} + +func (c config) has(name string) bool { + return slices.ContainsFunc(c.systems, func(sys system) bool { return sys.name == name }) +} + +func resolveBuilds(repo string, c config) (*build, *build, error) { + target := &build{Label: "target"} + targetRef := "HEAD" + if c.target != "" { + rev, err := git(repo, "rev-parse", "--verify", c.target+"^{commit}") + if err != nil { + return nil, nil, err + } + target.Rev = rev + targetRef = rev + target.Version, _ = git(repo, "describe", "--tags", "--always", rev) + } else if dirty, _ := git(repo, "status", "--porcelain", "--untracked-files=no"); dirty != "" { + target.Version, _ = git(repo, "describe", "--tags", "--always", "--dirty") + target.Version += " (working tree)" + } else { + // A clean working tree builds the same as HEAD. + target.Version, _ = git(repo, "describe", "--tags", "--always") + target.commit, _ = git(repo, "rev-parse", "HEAD") + } + base := &build{Label: "base"} + baseRef := c.base + if baseRef == "" { + mb, err := git(repo, "merge-base", targetRef, c.mainRef) + if err != nil { + return nil, nil, fmt.Errorf("computing merge-base with %s (try -base or -main): %w", c.mainRef, err) + } + baseRef = mb + } + rev, err := git(repo, "rev-parse", "--verify", baseRef+"^{commit}") + if err != nil { + return nil, nil, err + } + base.Rev = rev + base.Version, _ = git(repo, "describe", "--tags", "--always", rev) + if commit := cmp.Or(target.Rev, target.commit); commit != "" { + // The output is the commit followed by its parents. + out, _ := git(repo, "rev-list", "--parents", "-n", "1", commit) + if f := strings.Fields(out); len(f) == 3 && f[1] == base.Rev { + target.Merged, _ = git(repo, "describe", "--tags", "--always", f[2]) + } + } + return base, target, nil +} + +func fetchData(dir string) error { + if err := os.MkdirAll(dir, 0755); err != nil { + return err + } + for _, u := range []string{jsonURL, parquetURL} { + path := filepath.Join(dir, filepath.Base(u)) + if fi, err := os.Stat(path); err == nil && fi.Size() > 0 { + continue + } + log.Printf("downloading %s", u) + if err := download(u, path); err != nil { + return err + } + } + return nil +} + +func download(url, path string) error { + resp, err := http.Get(url) + if err != nil { + return err + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("%s: %s", url, resp.Status) + } + tmp := path + ".tmp" + f, err := os.Create(tmp) + if err != nil { + return err + } + n, err := io.Copy(f, resp.Body) + if cerr := f.Close(); err == nil { + err = cerr + } + if err == nil && resp.ContentLength >= 0 && n != resp.ContentLength { + err = fmt.Errorf("%s: got %d bytes, expected %d", url, n, resp.ContentLength) + } + if err != nil { + os.Remove(tmp) + return err + } + return os.Rename(tmp, path) +} + +func buildSuper(repo, work string, b *build) error { + b.bin = filepath.Join(work, "bin", "super-"+b.Label) + dir := repo + if b.Rev != "" { + dir = filepath.Join(work, "src-"+b.Label) + if _, err := git(repo, "worktree", "add", "--detach", dir, b.Rev); err != nil { + return err + } + defer git(repo, "worktree", "remove", "--force", dir) + } + log.Printf("building %s: %s", b.Label, b.Version) + ldflags := "-s -X github.com/superdb/super/cli.version=" + strings.Fields(b.Version)[0] + cmd := exec.Command("go", "build", "-ldflags="+ldflags, "-o", b.bin, "./cmd/super") + cmd.Dir = dir + cmd.Stdout = os.Stderr + cmd.Stderr = os.Stderr + if err := cmd.Run(); err != nil { + return fmt.Errorf("building %s: %w", b.Label, err) + } + return nil +} + +func runQuery(c config, work string, sys system, qf string, base, target *build) (*result, error) { + name := strings.TrimSuffix(qf, filepath.Ext(qf)) + r := &result{Test: sys.name + " " + name} + text, err := queryFS.ReadFile("queries/" + qf) + if err != nil { + return nil, err + } + args := map[*build][]string{} + for _, b := range []*build{base, target} { + path := filepath.Join(work, fmt.Sprintf("%s-%s-%s", sys.name, name, b.Label)+filepath.Ext(qf)) + q := strings.ReplaceAll(string(text), "__SOURCE__", sys.input(b, c.dataDir)) + if err := os.WriteFile(path, []byte(q), 0644); err != nil { + return nil, err + } + args[b] = append(append([]string{}, sys.flags...), "-I", path) + } + log.Printf("running %s", r.Test) + // The warmup run for each build also provides the output to compare. + outputs := map[*build][]byte{} + failed := map[*build]bool{} + fail := func(b *build, err error) { + failed[b] = true + if b == base { + r.BaseErr = err.Error() + } else { + r.TargetErr = err.Error() + } + } + for _, b := range []*build{base, target} { + _, out, err := measure(c.timeout, b.bin, args[b]...) + if err != nil { + fail(b, err) + continue + } + outputs[b] = out + file := filepath.Join(c.outDir, "outputs", fmt.Sprintf("%s-%s.%s.csv", sys.name, name, b.Label)) + if err := os.WriteFile(file, out, 0644); err != nil { + return nil, err + } + } + if !failed[base] && !failed[target] && string(outputs[base]) != string(outputs[target]) { + r.OutputMismatch = true + } + // Alternate the order of the builds (ABBA...) so any drift in the + // machine's performance affects both builds equally. + for i := 0; i < c.runs; i++ { + order := []*build{base, target} + if i%2 == 1 { + order = []*build{target, base} + } + for _, b := range order { + if failed[b] { + continue + } + m, _, err := measure(c.timeout, b.bin, args[b]...) + if err != nil { + fail(b, err) + continue + } + if b == base { + r.Base = append(r.Base, m) + } else { + r.Target = append(r.Target, m) + } + } + } + return r, nil +} + +// measure runs a command and returns its wall time, CPU time, peak memory, +// and standard output. +func measure(timeout time.Duration, name string, args ...string) (measurement, []byte, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + cmd := exec.CommandContext(ctx, name, args...) + var stderr strings.Builder + cmd.Stderr = &stderr + start := time.Now() + out, err := cmd.Output() + wall := time.Since(start) + if ctx.Err() != nil { + return measurement{}, nil, fmt.Errorf("timed out after %s", timeout) + } + if err != nil { + msg := strings.TrimSpace(stderr.String()) + if msg == "" { + msg = err.Error() + } + return measurement{}, nil, errors.New(firstLine(msg)) + } + ps := cmd.ProcessState + return measurement{ + Wall: wall.Seconds(), + User: ps.UserTime().Seconds(), + Sys: ps.SystemTime().Seconds(), + MaxRSS: maxRSS(ps), + }, out, nil +} + +func firstLine(s string) string { + if i := strings.IndexByte(s, '\n'); i >= 0 { + return s[:i] + } + return s +} + +func git(dir string, args ...string) (string, error) { + cmd := exec.Command("git", args...) + cmd.Dir = dir + var stderr strings.Builder + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("git %s: %s", strings.Join(args, " "), strings.TrimSpace(stderr.String())) + } + return strings.TrimSpace(string(out)), nil +} + +func median(ms []measurement, f func(measurement) float64) float64 { + if len(ms) == 0 { + return 0 + } + vals := make([]float64, len(ms)) + for i, m := range ms { + vals[i] = f(m) + } + sort.Float64s(vals) + n := len(vals) + if n%2 == 1 { + return vals[n/2] + } + return (vals[n/2-1] + vals[n/2]) / 2 +} + +func hostInfo() string { + return fmt.Sprintf("%s/%s, %d CPUs%s", runtime.GOOS, runtime.GOARCH, runtime.NumCPU(), cpuModel()) +} + +func cpuModel() string { + var out []byte + switch runtime.GOOS { + case "linux": + b, err := os.ReadFile("/proc/cpuinfo") + if err != nil { + return "" + } + for _, line := range strings.Split(string(b), "\n") { + if k, v, ok := strings.Cut(line, ":"); ok && strings.TrimSpace(k) == "model name" { + return ", " + strings.TrimSpace(v) + } + } + return "" + case "darwin": + out, _ = exec.Command("sysctl", "-n", "machdep.cpu.brand_string").Output() + } + if s := strings.TrimSpace(string(out)); s != "" { + return ", " + s + } + return "" +} + +func writeJSON(path string, v any) error { + b, err := json.MarshalIndent(v, "", " ") + if err != nil { + return err + } + return os.WriteFile(path, append(b, '\n'), 0644) +} + +func writeFile(path, s string) error { + return os.WriteFile(path, []byte(s), 0644) +} diff --git a/perf/canary/queries/agg.sql b/perf/canary/queries/agg.sql new file mode 100644 index 0000000000..7b6a4f021b --- /dev/null +++ b/perf/canary/queries/agg.sql @@ -0,0 +1,5 @@ +SELECT count() AS count, type +FROM '__SOURCE__' +WHERE repo.name='NixOS/nixpkgs' +GROUP BY type +ORDER BY count DESC; diff --git a/perf/canary/queries/count.sql b/perf/canary/queries/count.sql new file mode 100644 index 0000000000..287794c4bc --- /dev/null +++ b/perf/canary/queries/count.sql @@ -0,0 +1,3 @@ +SELECT count() AS count +FROM '__SOURCE__' +WHERE actor.login='johnbieren'; diff --git a/perf/canary/queries/search+.spq b/perf/canary/queries/search+.spq new file mode 100644 index 0000000000..4f8f8a2b99 --- /dev/null +++ b/perf/canary/queries/search+.spq @@ -0,0 +1,3 @@ +SELECT count() AS count +FROM '__SOURCE__' +WHERE grep('in case you have any feedback 😊', this); diff --git a/perf/canary/queries/search.spq b/perf/canary/queries/search.spq new file mode 100644 index 0000000000..0ccbe69423 --- /dev/null +++ b/perf/canary/queries/search.spq @@ -0,0 +1,3 @@ +SELECT count() AS count +FROM '__SOURCE__' +WHERE grep('in case you have any feedback 😊', payload.pull_request.body); diff --git a/perf/canary/queries/union.spq b/perf/canary/queries/union.spq new file mode 100644 index 0000000000..4fa7449478 --- /dev/null +++ b/perf/canary/queries/union.spq @@ -0,0 +1,6 @@ +FROM '__SOURCE__' +| UNNEST [...payload.pull_request.assignees, payload.pull_request.assignee] +| WHERE this IS NOT NULL +| AGGREGATE count() BY assignee:=login +| ORDER BY count DESC, assignee ASC +| LIMIT 5; diff --git a/perf/canary/report.go b/perf/canary/report.go new file mode 100644 index 0000000000..ca6169a3c1 --- /dev/null +++ b/perf/canary/report.go @@ -0,0 +1,371 @@ +package main + +import ( + "encoding/json" + "fmt" + "os" + "path" + "path/filepath" + "slices" + "strings" +) + +type status int + +const ( + neutral status = iota + improved + regressed +) + +// metric is one compared quantity (wall time, CPU time, or peak memory) for +// one test. +type metric struct { + Name string `json:"name"` + Base float64 `json:"base"` + Target float64 `json:"target"` + Change float64 `json:"change"` // relative change, e.g., 0.25 for +25% + Status string `json:"status"` + status status + unit string + scale float64 + format string +} + +type row struct { + *result + Metrics []*metric `json:"metrics,omitempty"` + Status string `json:"status"` + status status +} + +type report struct { + Base *build `json:"base"` + Target *build `json:"target"` + PR string `json:"pr,omitempty"` + Host string `json:"host"` + Runs int `json:"runs"` + Systems []string `json:"systems"` + Threshold float64 `json:"threshold"` + Verdict string `json:"verdict"` + Rows []*row `json:"results"` + c config +} + +func newReport(c config, base, target *build, results []*result) *report { + rep := &report{ + Base: base, + Target: target, + PR: c.pr, + Host: hostInfo(), + Runs: c.runs, + Systems: systemNamesOf(c.systems), + Threshold: c.threshold, + c: c, + } + for _, r := range results { + rep.Rows = append(rep.Rows, rep.classify(r)) + } + rep.Verdict = rep.verdict() + return rep +} + +func systemNamesOf(systems []system) []string { + var names []string + for _, sys := range systems { + names = append(names, sys.name) + } + return names +} + +func (rep *report) classify(r *result) *row { + row := &row{result: r} + if r.BaseErr != "" || r.TargetErr != "" { + row.Status = "failed" + return row + } + minDelta := rep.c.minDelta.Seconds() + row.Metrics = []*metric{ + rep.compare("wall", r, func(m measurement) float64 { return m.Wall }, minDelta, "s", 1, "%.3f"), + rep.compare("cpu", r, measurement.cpu, minDelta, "s", 1, "%.3f"), + } + if median(r.Base, rss) > 0 && median(r.Target, rss) > 0 { + row.Metrics = append(row.Metrics, rep.compare("memory", r, rss, float64(rep.c.minMemDelta), "MB", 1<<20, "%.0f")) + } + for _, m := range row.Metrics { + if m.status > row.status { + row.status = m.status + } + } + row.Status = statusName(row.status) + return row +} + +func rss(m measurement) float64 { return float64(m.MaxRSS) } + +func (rep *report) compare(name string, r *result, f func(measurement) float64, minDelta float64, unit string, scale float64, format string) *metric { + b, t := median(r.Base, f), median(r.Target, f) + m := &metric{Name: name, Base: b, Target: t, unit: unit, scale: scale, format: format} + if b > 0 { + m.Change = t/b - 1 + } + // A change is significant only if the medians differ by the relative + // threshold and the absolute minimum and the runs of the two builds + // don't overlap at all, which guards against metrics such as peak + // memory that can jump between levels from one run to the next. + thr := rep.c.threshold + bmin, bmax := minMax(r.Base, f) + tmin, tmax := minMax(r.Target, f) + switch { + case b > 0 && t/b >= 1+thr && t-b >= minDelta && tmin > bmax: + m.status = regressed + case b > 0 && t/b <= 1/(1+thr) && b-t >= minDelta && tmax < bmin: + m.status = improved + } + m.Status = statusName(m.status) + return m +} + +func minMax(ms []measurement, f func(measurement) float64) (float64, float64) { + if len(ms) == 0 { + return 0, 0 + } + lo, hi := f(ms[0]), f(ms[0]) + for _, m := range ms[1:] { + lo, hi = min(lo, f(m)), max(hi, f(m)) + } + return lo, hi +} + +func statusName(s status) string { + switch s { + case regressed: + return "regressed" + case improved: + return "improved" + } + return "neutral" +} + +func (rep *report) count(f func(*row) bool) int { + var n int + for _, r := range rep.Rows { + if f(r) { + n++ + } + } + return n +} + +func (rep *report) verdict() string { + switch { + case rep.count(func(r *row) bool { return r.Status == "failed" || r.OutputMismatch }) > 0: + return "attention" + case rep.count(func(r *row) bool { return r.status == regressed }) > 0: + return "regressed" + case rep.count(func(r *row) bool { return r.status == improved }) > 0: + return "improved" + } + return "neutral" +} + +func (rep *report) title() string { + total := len(rep.Rows) + regressions := rep.count(func(r *row) bool { return r.status == regressed }) + improvements := rep.count(func(r *row) bool { return r.status == improved }) + problems := rep.count(func(r *row) bool { return r.Status == "failed" || r.OutputMismatch }) + var parts []string + if problems > 0 { + parts = append(parts, fmt.Sprintf("%d of %d tests failed or changed their output", problems, total)) + } + if regressions > 0 { + parts = append(parts, fmt.Sprintf("%d of %d tests regressed", regressions, total)) + } + if improvements > 0 { + parts = append(parts, fmt.Sprintf("%d of %d tests improved", improvements, total)) + } + icon := "⚪" + switch rep.Verdict { + case "attention": + icon = "⚠️" + case "regressed": + icon = "🔴" + case "improved": + icon = "🟢" + } + if len(parts) == 0 { + parts = append(parts, fmt.Sprintf("no significant changes in %d tests", total)) + } + return fmt.Sprintf("%s Perf canary: %s", icon, strings.Join(parts, ", ")) +} + +// targetDesc describes the target build. For a merge commit, such as the +// one GitHub creates to test a pull request, it names the merged commit, +// since the merge commit itself may never be seen outside the run. +func (rep *report) targetDesc() string { + t := rep.Target + pr := "" + if rep.PR != "" { + pr = fmt.Sprintf("[PR #%s](%s)", path.Base(rep.PR), rep.PR) + } + switch { + case t.Merged == "" && pr == "": + return "`" + t.Version + "`" + case t.Merged == "": + return fmt.Sprintf("%s (`%s`)", pr, t.Version) + case pr == "": + return fmt.Sprintf("`%s` merged into the base (merge commit `%s`)", t.Merged, t.Version) + } + return fmt.Sprintf("%s (`%s`) merged into the base (merge commit `%s`)", pr, t.Merged, t.Version) +} + +func (rep *report) markdown() string { + var sb strings.Builder + fmt.Fprintf(&sb, "## %s\n\n", rep.title()) + sb.WriteString("| | |\n|-|-|\n") + fmt.Fprintf(&sb, "| Base | `%s` |\n", rep.Base.Version) + fmt.Fprintf(&sb, "| Target | %s |\n", rep.targetDesc()) + if rep.Base.Rev == rep.Target.Rev || rep.Base.Rev == rep.Target.commit { + sb.WriteString("| Note | Base and target are the same commit (A/A run), so any flagged change is noise |\n") + } + fmt.Fprintf(&sb, "| Host | %s |\n", rep.Host) + if len(rep.Systems) < len(systems) { + fmt.Fprintf(&sb, "| Systems | %s (others skipped) |\n", strings.Join(rep.Systems, ", ")) + } + runs := fmt.Sprintf("Median of %d runs per build, alternating builds", rep.Runs) + if rep.Runs == 1 { + runs = "1 run per build" + } + if slices.Contains(rep.Systems, "super-bsup") { + runs += " (BSUP load: 1 run)" + } + fmt.Fprintf(&sb, "| Method | %s. Significant: medians differ by ±%.0f%% and at least %s (time) or %d MB (peak memory), and every target run is worse (or better) than every base run. |\n\n", + runs, 100*rep.Threshold, rep.c.minDelta, rep.c.minMemDelta>>20) + rep.section(&sb, "🔴 Regressions", func(r *row) bool { return r.status == regressed }) + rep.section(&sb, "🟢 Improvements", func(r *row) bool { return r.status == improved }) + var problems []string + for _, r := range rep.Rows { + if r.BaseErr != "" { + problems = append(problems, fmt.Sprintf("- **%s** failed with base: `%s`", r.Test, r.BaseErr)) + } + if r.TargetErr != "" { + problems = append(problems, fmt.Sprintf("- **%s** failed with target: `%s`", r.Test, r.TargetErr)) + } + if r.OutputMismatch { + problems = append(problems, fmt.Sprintf("- **%s** output differs between base and target (timings may not be comparable)", r.Test)) + } + } + if len(problems) > 0 { + fmt.Fprintf(&sb, "### ⚠️ Failures and output differences\n\n%s\n\n", strings.Join(problems, "\n")) + } + sb.WriteString("### All results\n\n") + sb.WriteString("| Test | Wall time (s) | CPU time, user+sys (s) | Peak memory (MB) | |\n|-|-|-|-|-|\n") + for _, r := range rep.Rows { + if r.Status == "failed" { + fmt.Fprintf(&sb, "| %s | | | | ❌ |\n", r.Test) + continue + } + cells := []string{"", "", ""} + for i, m := range r.Metrics { + cells[i] = m.cell() + } + icon := statusIcon(r.status) + if r.OutputMismatch { + icon += " ⚠️" + } + fmt.Fprintf(&sb, "| %s | %s | %s | %s | %s |\n", r.Test, cells[0], cells[1], cells[2], icon) + } + sb.WriteString("\n") + return sb.String() +} + +func (rep *report) section(sb *strings.Builder, heading string, match func(*row) bool) { + var lines []string + for _, r := range rep.Rows { + if !match(r) { + continue + } + var parts []string + for _, m := range r.Metrics { + if m.status != neutral { + parts = append(parts, fmt.Sprintf("%s %s → %s %s (**%s**)", metricLabel(m.Name), m.value(m.Base), m.value(m.Target), m.unit, pct(m.Change))) + } + } + lines = append(lines, fmt.Sprintf("- **%s**: %s", r.Test, strings.Join(parts, ", "))) + } + if len(lines) > 0 { + fmt.Fprintf(sb, "### %s\n\n%s\n\n", heading, strings.Join(lines, "\n")) + } +} + +func (m *metric) value(v float64) string { + return fmt.Sprintf(m.format, v/m.scale) +} + +func (m *metric) cell() string { + s := fmt.Sprintf("%s → %s %s", m.value(m.Base), m.value(m.Target), pct(m.Change)) + if m.status != neutral { + s = fmt.Sprintf("%s → %s **%s** %s", m.value(m.Base), m.value(m.Target), pct(m.Change), statusIcon(m.status)) + } + return s +} + +func metricLabel(name string) string { + switch name { + case "wall": + return "wall time" + case "cpu": + return "CPU time" + } + return "peak memory" +} + +func statusIcon(s status) string { + switch s { + case regressed: + return "🔴" + case improved: + return "🟢" + } + return "" +} + +func pct(change float64) string { + return fmt.Sprintf("%+.0f%%", 100*change) +} + +// rereport reclassifies the raw results in a results.json file written by +// an earlier run using the thresholds in c, which is handy for tuning them. +func rereport(c config, path string) (*report, error) { + b, err := os.ReadFile(path) + if err != nil { + return nil, err + } + var old struct { + Base *build `json:"base"` + Target *build `json:"target"` + PR string `json:"pr"` + Host string `json:"host"` + Runs int `json:"runs"` + Systems []string `json:"systems"` + Results []*result `json:"results"` + } + if err := json.Unmarshal(b, &old); err != nil { + return nil, err + } + c.runs = old.Runs + c.pr = old.PR + rep := newReport(c, old.Base, old.Target, old.Results) + rep.Host = old.Host + if old.Systems != nil { + rep.Systems = old.Systems + } + return rep, nil +} + +func (rep *report) write(dir string) error { + if err := writeJSON(filepath.Join(dir, "results.json"), rep); err != nil { + return err + } + return writeFile(filepath.Join(dir, "summary.md"), rep.markdown()) +} diff --git a/perf/canary/rusage_darwin.go b/perf/canary/rusage_darwin.go new file mode 100644 index 0000000000..4702ad437d --- /dev/null +++ b/perf/canary/rusage_darwin.go @@ -0,0 +1,4 @@ +package main + +// On macOS, getrusage reports ru_maxrss in bytes. +const maxrssUnit = 1 diff --git a/perf/canary/rusage_other.go b/perf/canary/rusage_other.go new file mode 100644 index 0000000000..047aab5794 --- /dev/null +++ b/perf/canary/rusage_other.go @@ -0,0 +1,10 @@ +//go:build !unix + +package main + +import "os" + +// maxRSS isn't available on this platform, so peak memory isn't compared. +func maxRSS(*os.ProcessState) int64 { + return 0 +} diff --git a/perf/canary/rusage_other_unix.go b/perf/canary/rusage_other_unix.go new file mode 100644 index 0000000000..bb7d81bb6b --- /dev/null +++ b/perf/canary/rusage_other_unix.go @@ -0,0 +1,6 @@ +//go:build unix && !darwin + +package main + +// On Linux and the BSDs, getrusage reports ru_maxrss in kilobytes. +const maxrssUnit = 1024 diff --git a/perf/canary/rusage_unix.go b/perf/canary/rusage_unix.go new file mode 100644 index 0000000000..df14566e1e --- /dev/null +++ b/perf/canary/rusage_unix.go @@ -0,0 +1,16 @@ +//go:build unix + +package main + +import ( + "os" + "syscall" +) + +// maxRSS returns the peak resident set size of an exited process in bytes. +func maxRSS(ps *os.ProcessState) int64 { + if ru, ok := ps.SysUsage().(*syscall.Rusage); ok { + return int64(ru.Maxrss) * maxrssUnit + } + return 0 +} From 071132500fdf423bd50b911949df65016e0d9d39 Mon Sep 17 00:00:00 2001 From: Phil Rzewski Date: Wed, 21 Oct 2026 15:33:08 -0700 Subject: [PATCH 2/2] PR feedback and "noisy" detection --- .github/workflows/perf-canary.yaml | 11 +++++-- Makefile | 5 +-- perf/canary/README.md | 32 +++++++++++++++++-- perf/canary/main.go | 49 ++++++++++++++++++++++++++++-- perf/canary/report.go | 47 +++++++++++++++++++++++++--- 5 files changed, 128 insertions(+), 16 deletions(-) diff --git a/.github/workflows/perf-canary.yaml b/.github/workflows/perf-canary.yaml index 19ed1ae990..ba6e844077 100644 --- a/.github/workflows/perf-canary.yaml +++ b/.github/workflows/perf-canary.yaml @@ -37,6 +37,8 @@ on: default: "5" systems: description: "Comma-separated systems to test (default: all of super-bsup, super-json, super-json-static, super-parquet)" + queries: + description: "Comma-separated queries to run (default: all of search, search+, count, agg, union)" permissions: contents: read @@ -75,8 +77,9 @@ jobs: INPUT_TARGET: ${{ inputs.target }} INPUT_RUNS: ${{ inputs.runs }} INPUT_SYSTEMS: ${{ inputs.systems }} + INPUT_QUERIES: ${{ inputs.queries }} run: | - base='' target='' runs=5 systems='' pr='' + base='' target='' runs=5 systems='' queries='' pr='' case "$EVENT" in pull_request) # The checkout is the PR merged into main, so the default base @@ -95,7 +98,7 @@ jobs: fi ;; workflow_dispatch) - base=$INPUT_BASE target=$INPUT_TARGET runs=${INPUT_RUNS:-5} systems=$INPUT_SYSTEMS + base=$INPUT_BASE target=$INPUT_TARGET runs=${INPUT_RUNS:-5} systems=$INPUT_SYSTEMS queries=$INPUT_QUERIES ;; esac { @@ -103,6 +106,7 @@ jobs: echo "target=$target" echo "runs=$runs" echo "systems=$systems" + echo "queries=$queries" echo "pr=$pr" } >> "$GITHUB_OUTPUT" - name: Run perf canary @@ -111,9 +115,10 @@ jobs: TARGET: ${{ steps.config.outputs.target }} RUNS: ${{ steps.config.outputs.runs }} SYSTEMS: ${{ steps.config.outputs.systems }} + QUERIES: ${{ steps.config.outputs.queries }} PR: ${{ steps.config.outputs.pr }} run: | - make perf BASE="$BASE" TARGET="$TARGET" SYSTEMS="$SYSTEMS" \ + make perf BASE="$BASE" TARGET="$TARGET" SYSTEMS="$SYSTEMS" QUERIES="$QUERIES" \ PERF_CANARY_FLAGS="-runs $RUNS -datadir $HOME/.cache/super-perf-canary -out $RUNNER_TEMP/perf-canary ${PR:+-pr $PR}" - name: Report id: report diff --git a/Makefile b/Makefile index ddf097e97d..c37f689a0f 100644 --- a/Makefile +++ b/Makefile @@ -85,11 +85,12 @@ peg: compiler/parser/parser.go # Compare the performance of two builds of super on a small data set. # BASE defaults to the merge-base with origin/main, TARGET defaults to the -# working tree, and SYSTEMS (a comma-separated list) defaults to all of them. +# working tree, and SYSTEMS and QUERIES (comma-separated lists) default to +# all of them. # See perf/canary/README.md. .PHONY: perf perf: - @go run ./perf/canary $(if $(BASE),-base '$(BASE)') $(if $(TARGET),-target '$(TARGET)') $(if $(SYSTEMS),-systems '$(SYSTEMS)') $(PERF_CANARY_FLAGS) + @go run ./perf/canary $(if $(BASE),-base '$(BASE)') $(if $(TARGET),-target '$(TARGET)') $(if $(SYSTEMS),-systems '$(SYSTEMS)') $(if $(QUERIES),-queries '$(QUERIES)') $(PERF_CANARY_FLAGS) .PHONY: markdown-lint markdown-lint: diff --git a/perf/canary/README.md b/perf/canary/README.md index 1eae1d953a..7514acba09 100644 --- a/perf/canary/README.md +++ b/perf/canary/README.md @@ -37,6 +37,13 @@ with a comma-separated list, e.g., to skip the slowest one, make perf SYSTEMS=super-bsup,super-json,super-parquet ``` +Likewise, to run only some of the queries in [`queries/`](queries), list +them by file name without the extension, e.g.: + +``` +make perf SYSTEMS=super-bsup QUERIES=union +``` + Other options can be passed with `PERF_CANARY_FLAGS`, e.g., `make perf PERF_CANARY_FLAGS="-keep -threshold 0.15"`. Run `go run ./perf/canary -h` to see them all. @@ -83,14 +90,14 @@ from a fork can't add the check, so its report is only in the job summary.) The workflow can also be started manually from the Actions tab to compare any two commits. Choose the branch to test and, optionally, the base and -target commits, the number of runs, and the systems to test. +target commits, the number of runs, and the systems and queries to test. ## What's measured For each build, the canary: 1. builds `super`, -2. converts the JSON data to BSUP (`super -dynamic -i json -f bsup`), +2. converts the JSON data to BSUP (`super -i json -o .bsup`), measured once, and 3. runs each of the five queries in [`queries/`](queries) against four inputs: @@ -98,7 +105,7 @@ For each build, the canary: | Test | Input | Flags | |-|-|-| | `super-bsup` | the BSUP file written by that build | `-i bsup` | - | `super-json` | `ghasample.json.gz` | `-dynamic -i json` | + | `super-json` | `ghasample.json.gz` | `-i json` | | `super-json-static` | `ghasample.json.gz` | `-static -i json` | | `super-parquet` | `ghasample.parquet` | `-i parquet` | @@ -122,6 +129,25 @@ A change is flagged as significant only if all of these hold: Because of that last rule, use at least 5 runs (the default). With fewer, noise alone is much more likely to separate the two builds' runs. +When the median wall or CPU time differs by enough to count but the runs +overlap, the change isn't flagged, and the report marks it 〰️ *noisy* +instead. (Peak memory isn't marked, since its runs overlap routinely.) This +usually means something else on the machine (another program, or a busy +hosted runner) slowed down some of the runs. Since the builds alternate, +such a slowdown hits both builds, but the medians can still land far apart +by chance. For example, in this local run, the machine got busy partway +through: + +| Run | 1 | 2 | 3 | 4 | 5 | +|-|-|-|-|-|-| +| base | 4.871 s | 4.839 s | 5.229 s | 8.686 s | 10.906 s | +| target | 4.818 s | 4.807 s | 7.462 s | 7.681 s | 12.324 s | + +The medians (5.229 → 7.462 s) differ by 43%, but the first two runs show +the builds are the same speed. Noise can also hide a real change, so if a +test you care about is marked 〰️ *noisy*, re-run it, e.g., with `SYSTEMS` +and `QUERIES` set to just that test. + To try different thresholds on an earlier run without re-running it, pass its `results.json` to `-rereport`, e.g., `go run ./perf/canary -rereport results.json -threshold 0.15`. diff --git a/perf/canary/main.go b/perf/canary/main.go index 6270e137fc..337f130f96 100644 --- a/perf/canary/main.go +++ b/perf/canary/main.go @@ -45,7 +45,7 @@ type system struct { var systems = []system{ {"super-bsup", []string{"-i", "bsup", "-f", "csv"}, func(b *build, _ string) string { return b.bsup }}, - {"super-json", []string{"-dynamic", "-i", "json", "-f", "csv"}, func(_ *build, data string) string { return filepath.Join(data, "ghasample.json.gz") }}, + {"super-json", []string{"-i", "json", "-f", "csv"}, func(_ *build, data string) string { return filepath.Join(data, "ghasample.json.gz") }}, {"super-json-static", []string{"-static", "-i", "json", "-f", "csv"}, func(_ *build, data string) string { return filepath.Join(data, "ghasample.json.gz") }}, {"super-parquet", []string{"-i", "parquet", "-f", "csv"}, func(_ *build, data string) string { return filepath.Join(data, "ghasample.parquet") }}, } @@ -92,6 +92,7 @@ type config struct { dataDir, outDir string keep bool systems []system + queries []string // file names in queries/ } func main() { @@ -113,6 +114,7 @@ func main() { flag.BoolVar(&c.keep, "keep", false, "keep the temporary work directory") rereportPath := flag.String("rereport", "", "reclassify the results in this results.json file instead of running") systemNames := flag.String("systems", allSystemNames(), "comma-separated list of systems to test") + queryNames := flag.String("queries", strings.Join(queryNamesOf(queryFiles), ","), "comma-separated list of queries to run") // The default usage line names the temporary binary built by "go run". flag.Usage = func() { fmt.Fprint(flag.CommandLine.Output(), "Usage: go run ./perf/canary [flags]\n\n"+ @@ -125,6 +127,9 @@ func main() { if c.systems, err = selectSystems(*systemNames); err != nil { log.Fatal(err) } + if c.queries, err = selectQueries(*queryNames); err != nil { + log.Fatal(err) + } if *rereportPath != "" { rep, err := rereport(c, *rereportPath) if err != nil { @@ -182,7 +187,7 @@ func run(c config) error { } b.bsup = filepath.Join(work, b.Label+".bsup") log.Printf("loading BSUP with %s", b.Label) - m, _, err := measure(c.timeout, b.bin, "-dynamic", "-i", "json", "-f", "bsup", "-o", b.bsup, filepath.Join(c.dataDir, "ghasample.json.gz")) + m, _, err := measure(c.timeout, b.bin, "-i", "json", "-o", b.bsup, filepath.Join(c.dataDir, "ghasample.json.gz")) if err != nil { return fmt.Errorf("BSUP load with %s build: %w", b.Label, err) } @@ -196,7 +201,7 @@ func run(c config) error { results = append(results, load) } for _, sys := range c.systems { - for _, qf := range queryFiles { + for _, qf := range c.queries { r, err := runQuery(c, work, sys, qf, base, target) if err != nil { return err @@ -239,6 +244,44 @@ func selectSystems(list string) ([]system, error) { return selected, nil } +// queryNamesOf returns the names of query files without their extensions. +func queryNamesOf(files []string) []string { + var names []string + for _, qf := range files { + names = append(names, strings.TrimSuffix(qf, filepath.Ext(qf))) + } + return names +} + +// selectQueries returns the files in queryFiles named in list, in the order +// they're listed there. +func selectQueries(list string) ([]string, error) { + all := queryNamesOf(queryFiles) + var selected []string + for _, name := range strings.Split(list, ",") { + name = strings.TrimSpace(name) + if name == "" { + continue + } + if !slices.Contains(all, name) { + return nil, fmt.Errorf("unknown query %q (choose from %s)", name, strings.Join(all, ",")) + } + if !slices.Contains(selected, name) { + selected = append(selected, name) + } + } + if len(selected) == 0 { + return nil, errors.New("no queries selected") + } + var files []string + for i, name := range all { + if slices.Contains(selected, name) { + files = append(files, queryFiles[i]) + } + } + return files, nil +} + func (c config) has(name string) bool { return slices.ContainsFunc(c.systems, func(sys system) bool { return sys.name == name }) } diff --git a/perf/canary/report.go b/perf/canary/report.go index ca6169a3c1..e3f40f058d 100644 --- a/perf/canary/report.go +++ b/perf/canary/report.go @@ -26,6 +26,11 @@ type metric struct { Target float64 `json:"target"` Change float64 `json:"change"` // relative change, e.g., 0.25 for +25% Status string `json:"status"` + // Noisy is set for a time when the medians differ by enough to be + // significant but the runs of the two builds overlap, so the change + // isn't flagged. Peak memory isn't marked since its runs overlap + // routinely as it jumps between levels. + Noisy bool `json:"noisy,omitempty"` status status unit string scale float64 @@ -46,6 +51,7 @@ type report struct { Host string `json:"host"` Runs int `json:"runs"` Systems []string `json:"systems"` + Queries []string `json:"queries"` Threshold float64 `json:"threshold"` Verdict string `json:"verdict"` Rows []*row `json:"results"` @@ -60,6 +66,7 @@ func newReport(c config, base, target *build, results []*result) *report { Host: hostInfo(), Runs: c.runs, Systems: systemNamesOf(c.systems), + Queries: queryNamesOf(c.queries), Threshold: c.threshold, c: c, } @@ -114,13 +121,22 @@ func (rep *report) compare(name string, r *result, f func(measurement) float64, // don't overlap at all, which guards against metrics such as peak // memory that can jump between levels from one run to the next. thr := rep.c.threshold + noisy := name != "memory" bmin, bmax := minMax(r.Base, f) tmin, tmax := minMax(r.Target, f) switch { - case b > 0 && t/b >= 1+thr && t-b >= minDelta && tmin > bmax: - m.status = regressed - case b > 0 && t/b <= 1/(1+thr) && b-t >= minDelta && tmax < bmin: - m.status = improved + case b > 0 && t/b >= 1+thr && t-b >= minDelta: + if tmin > bmax { + m.status = regressed + } else { + m.Noisy = noisy + } + case b > 0 && t/b <= 1/(1+thr) && b-t >= minDelta: + if tmax < bmin { + m.status = improved + } else { + m.Noisy = noisy + } } m.Status = statusName(m.status) return m @@ -169,6 +185,10 @@ func (rep *report) verdict() string { return "neutral" } +func (r *row) noisy() bool { + return slices.ContainsFunc(r.Metrics, func(m *metric) bool { return m.Noisy }) +} + func (rep *report) title() string { total := len(rep.Rows) regressions := rep.count(func(r *row) bool { return r.status == regressed }) @@ -196,7 +216,11 @@ func (rep *report) title() string { if len(parts) == 0 { parts = append(parts, fmt.Sprintf("no significant changes in %d tests", total)) } - return fmt.Sprintf("%s Perf canary: %s", icon, strings.Join(parts, ", ")) + s := fmt.Sprintf("%s Perf canary: %s", icon, strings.Join(parts, ", ")) + if noisy := rep.count((*row).noisy); noisy > 0 { + s += fmt.Sprintf(" (%d noisy)", noisy) + } + return s } // targetDesc describes the target build. For a merge commit, such as the @@ -232,6 +256,9 @@ func (rep *report) markdown() string { if len(rep.Systems) < len(systems) { fmt.Fprintf(&sb, "| Systems | %s (others skipped) |\n", strings.Join(rep.Systems, ", ")) } + if len(rep.Queries) < len(queryFiles) { + fmt.Fprintf(&sb, "| Queries | %s (others skipped) |\n", strings.Join(rep.Queries, ", ")) + } runs := fmt.Sprintf("Median of %d runs per build, alternating builds", rep.Runs) if rep.Runs == 1 { runs = "1 run per build" @@ -276,6 +303,9 @@ func (rep *report) markdown() string { fmt.Fprintf(&sb, "| %s | %s | %s | %s | %s |\n", r.Test, cells[0], cells[1], cells[2], icon) } sb.WriteString("\n") + if rep.count((*row).noisy) > 0 { + sb.WriteString("〰️ *noisy*: the medians differ by enough to count, but the runs of the two builds overlap, so the change isn't flagged. This usually means something else on the machine slowed some of the runs. Re-run to check.\n\n") + } return sb.String() } @@ -304,6 +334,9 @@ func (m *metric) value(v float64) string { func (m *metric) cell() string { s := fmt.Sprintf("%s → %s %s", m.value(m.Base), m.value(m.Target), pct(m.Change)) + if m.Noisy { + s += " 〰️ *noisy*" + } if m.status != neutral { s = fmt.Sprintf("%s → %s **%s** %s", m.value(m.Base), m.value(m.Target), pct(m.Change), statusIcon(m.status)) } @@ -348,6 +381,7 @@ func rereport(c config, path string) (*report, error) { Host string `json:"host"` Runs int `json:"runs"` Systems []string `json:"systems"` + Queries []string `json:"queries"` Results []*result `json:"results"` } if err := json.Unmarshal(b, &old); err != nil { @@ -360,6 +394,9 @@ func rereport(c config, path string) (*report, error) { if old.Systems != nil { rep.Systems = old.Systems } + if old.Queries != nil { + rep.Queries = old.Queries + } return rep, nil }