diff --git a/.gitignore b/.gitignore index a83aeee..7ea9deb 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,5 @@ .DS_Store /tmp **/project-file-cache.json +# Local benchmark output. perf/baseline.json is committed on purpose. +/perf/results diff --git a/Cargo.toml b/Cargo.toml index c18fc5f..b765e30 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,6 +5,12 @@ edition = "2024" [profile.release] debug = true +# Measurement determinism. Baseline and candidate are separate builds, so with +# default codegen-units the codegen split can differ between them and contaminate +# an A/B comparison directly, rather than merely widening error bars. Costs some +# build time; worth it for the perf harness's numbers to mean anything. +codegen-units = 1 +lto = "thin" [lib] path = "src/lib.rs" diff --git a/perf/README.md b/perf/README.md new file mode 100644 index 0000000..ca1fa6d --- /dev/null +++ b/perf/README.md @@ -0,0 +1,204 @@ +# Performance harness + +Measures `validate`, `generate` and `generate-and-validate` so performance claims +can be checked instead of asserted. + +This is a **local development tool**. It is deliberately not wired into CI: shared +runners are too noisy for the 2–20 second wall-clock comparisons that matter here, +and they have no large corpus to measure against. Performance regressions are +caught when someone runs this on purpose. + +## Quick start + +```bash +# Smoke test — proves the harness works. NOT a measurement. +./perf/run.sh + +# Real numbers. +export CODEOWNERS_PERF_CORPUS=/path/to/a/large/monorepo +./perf/run.sh +``` + +## The default corpus is a smoke test, not a measurement + +With no corpus configured the harness runs against `tests/fixtures/valid_project` +— 28 files and a 41-line CODEOWNERS. Every case finishes in single-digit +milliseconds and they all look about the same. + +That is genuinely useful: it proves the cases execute, the JSON is well-formed, +snapshot/restore fires, and the file-count assertions hold. It is useless for +comparing optimizations. + +**To get numbers that mean anything, point the harness at a large monorepo.** A +useful corpus has on the order of 10⁵ tracked files and a CODEOWNERS file with +thousands of entries; that is the shape where the interesting costs show up. + +Two guards exist so nobody mistakes one for the other: + +1. `run.sh` prints a loud banner when the corpus has fewer than 1,000 tracked + files, naming the corpus and its size. +2. `compare.sh` **refuses** to diff two reports whose corpus path or corpus git + commit differ. This catches the subtler mistake — a branch measured on the + fixture diffed against a baseline measured on the monorepo, which would + otherwise read as a spectacular speedup. + +Every report records the corpus path, its commit, its tracked-file count and its +CODEOWNERS line count. + +## Corpus resolution + +First match wins: + +1. `--corpus ` +2. `$CODEOWNERS_PERF_CORPUS` +3. `tests/fixtures/valid_project` (committed fixture) + +No path to any specific monorepo is stored in this repository. The corpus must +contain a readable `config/code_ownership.yml`. + +## Cases + +| Case | Command under test | What it isolates | +| --- | --- | --- | +| `generate` | `generate` | Project build + one file generation | +| `validate_all` | `validate` | Full ownership validation | +| `gv` | `generate-and-validate` | The headline CI/pre-commit-hook command | +| `gv_files_100` | `gv <100 paths>` | The likely real hook invocation | +| `gv_files_1000` | `gv <1000 paths>` | Same, larger changeset | +| `validate_all_cold` | `validate --no-cache` | Guards against wins that only exist warm | +| `validate_files_1` | `validate <1 path>` | Fixed-cost floor | +| `validate_files_100` | `validate <100 paths>` | Realistic changeset | +| `validate_files_1000` | `validate <1000 paths>` | Exposes any per-file linear term | +| `validate_files_2000` | `validate <2000 paths>` | Confirms the slope | + +`codeowners-perf cases` lists them. `--case ` filters. + +Cases needing more owned files than the corpus contains are reported as +**skipped** with the reason, never silently shrunk. + +**`gv ` and `validate ` are not the same measurement.** `generate` +needs the project build, so an optimization that bypasses that build speeds up +`validate ` but can do nothing for `gv `. Both are measured because +a hook that runs `gv` sees the smaller of the two wins, and quoting the +`validate` number for it would be wrong. + +## Reading the output + +- **best** is the headline number. **median** is shown alongside so you can see + whether a run was noisy; all individual run times are kept in the JSON. +- `compare` reports the observed run-to-run **spread** per case and marks any + delta smaller than it **within noise**. Trust that column over the delta: + min-of-N is a biased estimator with no dispersion attached, so a 3% "win" on a + case that swings 40% between runs reads exactly like a real one. +- `validate_all_cold` is by far the noisiest case — it is IO-bound and a 50% + spread between runs is normal, which is larger than most effects worth hunting. + It is useful as a guard against wins that only exist warm, not as a number to + optimize against. Raise `--runs` a lot if you need to trust it. + +### Two things the numbers do not include + +- **Per-invocation setup is undercounted.** All cases run in one process, and + `teams_by_github_team_name` is `#[memoize]`d process-globally, so the warmup run + pays the team-file parse and no timed run ever does. A real CLI invocation pays + it every time. Treat published numbers as a floor for single-shot CLI cost. +- **Fixed cost dominates small changesets.** The per-file cases are affine, not + proportional: on a 130k-file corpus they fit ~2.0s fixed plus ~9.9ms/file. So + the per-file *average* is ~2,100ms at one file and ~11ms at two thousand. For + the common CI case — a PR touching a handful of files — essentially all of the + time is the fixed project build, and the per-file rate is nearly irrelevant. + Quote both terms, or you will optimize the wrong end. + +### Phase percentages: check nesting before quoting + +Spans are inclusive of children, so sibling spans can be summed and nested ones +cannot. `config_load`, `cache_init`, `project_build`, `cache_persist` and +`per_file_query` are disjoint — percentages across those are sound. But +`ownership_validate` ⊃ `validator_validate` ⊃ `validate_file_ownership` ⊃ +`file_to_owners`: quoting those together as shares of one total double-counts. +- The **phase breakdown** comes from `tracing` spans inside the library. Phases + are **inclusive of nested children**, so they do not sum to the total — + `project_build` contains its own sub-work, and `ownership_validate` contains + `validator_validate`, which contains `file_to_owners`. Use them to attribute a + win, not to reconstruct a total. +- `mapper_build` is **accumulated across all calls in a run**. That is + deliberate: it is currently invoked more than once per validate, and the sum is + what a fix should reduce. + +## The corpus is written to, and restored + +`generate` and `generate-and-validate` **write** the corpus's CODEOWNERS file. +The harness snapshots that file before running and restores it afterwards. + +It also **refuses to start** if the corpus's CODEOWNERS already has uncommitted +changes — otherwise it could not tell its own writes from yours, and restoring +would clobber your work. Commit or stash first. + +## Adding a case + +Add an entry to `CASES` in `src/bin/codeowners-perf.rs`. A case is a name, a +command kind, a file count and a cache flag. `tests/perf_harness_test.rs` covers +the harness mechanics, so run `cargo test --test perf_harness_test` afterwards. + +## No baseline is committed — you generate your own + +There is deliberately no `perf/baseline.json` in the repository. Two reasons: + +1. **Wall-clock numbers are not portable.** A baseline measured on one laptop says + nothing about another machine, so a committed one would invite exactly the + invalid comparison the guards above exist to prevent. Reports record + `machine` (os/arch/cpu count) and `compare.sh` warns when it differs. +2. **It would embed a local absolute path.** The corpus path is recorded in every + report to make comparisons safe; committing one would put somebody's private + checkout path into the repo. + +`perf/results/` is gitignored. Measure the base branch yourself, then your branch, +on the same machine and the same corpus. + +## Comparing a branch + +```bash +export CODEOWNERS_PERF_CORPUS=/path/to/a/large/monorepo + +# 1. baseline: on the branch you are comparing against +git checkout main +./perf/run.sh --json > perf/results/base.json + +# 2. candidate: your branch +git checkout my-branch +./perf/run.sh --json > perf/results/my-branch.json + +# 3. diff +./perf/compare.sh perf/results/base.json perf/results/my-branch.json +``` + +Paste the resulting table into the PR. **Report regressions too**, including on +`validate_all_cold`. + +Before claiming a speedup, verify correctness — a faster wrong answer is the main +risk in this area: + +```bash +# byte-identical generated output vs. the base branch +./target/release/codeowners --project-root "$CODEOWNERS_PERF_CORPUS" g -s +cp "$CODEOWNERS_PERF_CORPUS/.github/CODEOWNERS" /tmp/after.txt +git stash && cargo build --release # or check out the base branch +./target/release/codeowners --project-root "$CODEOWNERS_PERF_CORPUS" g -s +diff /tmp/after.txt "$CODEOWNERS_PERF_CORPUS/.github/CODEOWNERS" && echo "identical" +``` + +## A trap worth knowing about + +While profiling this originally, the CLI reported a suspiciously flat ~2.2s for 1, +100, 1,000 and 5,000 files. The cause was the shell, not the code: **zsh does not +word-split unquoted variables**, so `codeowners v $FILES` passed one +newline-joined mega-argument. It matched no glob, was filtered out, and the +per-file loop never ran. The measurement looked clean and was measuring nothing. + +Two consequences for this harness: + +- It builds argument lists as real vectors in Rust, never by interpolating a + shell string. +- It asserts that each case built exactly the number of paths it asked for, so a + future filtering regression fails loudly instead of producing a fast number. + +If you time the CLI by hand, use an array: `"${FILES[@]}"`. diff --git a/perf/compare.sh b/perf/compare.sh new file mode 100755 index 0000000..7a4166c --- /dev/null +++ b/perf/compare.sh @@ -0,0 +1,23 @@ +#!/usr/bin/env bash +# +# Diff two harness JSON reports into a markdown delta table, ready to paste into +# a PR description. +# +# Usage: +# ./perf/compare.sh perf/baseline.json perf/results/my-branch.json +# +# Refuses to compare reports measured against different corpora or different +# corpus commits — see perf/README.md for why that matters. + +set -euo pipefail + +if [[ $# -ne 2 ]]; then + echo "usage: $0 " >&2 + exit 2 +fi + +repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +cd "$repo_root" + +cargo build --release --bin codeowners-perf --quiet +exec ./target/release/codeowners-perf compare "$1" "$2" diff --git a/perf/run.sh b/perf/run.sh new file mode 100755 index 0000000..19c2d58 --- /dev/null +++ b/perf/run.sh @@ -0,0 +1,28 @@ +#!/usr/bin/env bash +# +# Driver for the codeowners performance harness. +# +# Builds the release harness and runs the benchmark cases. All the real work +# (corpus resolution, snapshot/restore, case timing, JSON) lives in +# src/bin/codeowners-perf.rs — this script just makes the common invocation +# short and keeps you from accidentally measuring a debug build. +# +# Usage: +# ./perf/run.sh # fixture corpus (smoke test) +# ./perf/run.sh --corpus /path/to/large-monorepo # real numbers +# ./perf/run.sh --json > perf/results/mine.json # machine readable +# ./perf/run.sh --case gv --runs 5 +# +# See perf/README.md. + +set -euo pipefail + +repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +cd "$repo_root" + +# A debug build is 10-30x slower and its numbers are meaningless for comparison, +# so the release build is not optional. +echo "building release harness..." >&2 +cargo build --release --bin codeowners-perf --quiet + +exec ./target/release/codeowners-perf run "$@" diff --git a/src/bin/codeowners-perf.rs b/src/bin/codeowners-perf.rs new file mode 100644 index 0000000..e1c7f1a --- /dev/null +++ b/src/bin/codeowners-perf.rs @@ -0,0 +1,719 @@ +//! Benchmark harness for `validate` and `generate_and_validate`. +//! +//! This binary exists to make performance claims checkable. It runs a fixed set of +//! named cases against a configurable corpus, records wall-clock and per-phase +//! timings, and emits JSON that can be diffed across branches. +//! +//! It is a local development tool: it is deliberately not wired into CI, because +//! shared runners are too noisy for the 2-20s wall-clock comparisons we care about +//! and have no large corpus available. See `perf/README.md`. + +use std::collections::BTreeMap; +use std::path::{Path, PathBuf}; +use std::process::Command; +use std::sync::{Mutex, OnceLock}; +use std::time::{Duration, Instant}; + +use clap::{Parser, Subcommand}; +use codeowners::config::Config; +use codeowners::runner::{self, RunConfig, RunResult}; +use serde::{Deserialize, Serialize}; +use tracing_subscriber::layer::{Context, Layer, SubscriberExt}; +use tracing_subscriber::registry::LookupSpan; +use tracing_subscriber::util::SubscriberInitExt; + +/// Corpora smaller than this are smoke-test scale: every case completes in +/// single-digit milliseconds and the numbers are not comparable to anything. +const SMOKE_SCALE_MAX_FILES: usize = 1_000; + +#[derive(Parser)] +#[command(about = "Performance harness for codeowners validate/generate", version)] +struct Cli { + #[command(subcommand)] + command: Cmd, +} + +#[derive(Subcommand)] +enum Cmd { + /// Run benchmark cases against a corpus. + Run { + /// Corpus to measure against. Falls back to $CODEOWNERS_PERF_CORPUS, then + /// the committed test fixture. + #[arg(long)] + corpus: Option, + /// Only run cases whose name contains this substring. + #[arg(long)] + case: Option, + /// Timed runs per case; the best is reported. + #[arg(long, default_value_t = 3)] + runs: usize, + /// Untimed warmup runs per case, to settle the cache and page cache. + #[arg(long, default_value_t = 1)] + warmup: usize, + /// Emit JSON instead of a human-readable table. + #[arg(long)] + json: bool, + }, + /// Diff two JSON reports into a markdown delta table. + Compare { baseline: PathBuf, candidate: PathBuf }, + /// List the available case names. + Cases, +} + +// --------------------------------------------------------------------------- +// Report model +// --------------------------------------------------------------------------- + +#[derive(Serialize, Deserialize)] +struct Report { + tool_version: String, + #[serde(default)] + machine: MachineInfo, + corpus: CorpusInfo, + runs_per_case: usize, + cases: Vec, +} + +/// Enough to notice that two reports came from different machines. Wall-clock +/// numbers are not portable across hardware, so comparing them is meaningless. +#[derive(Serialize, Deserialize, PartialEq, Eq, Default)] +struct MachineInfo { + os: String, + arch: String, + cpus: usize, +} + +impl MachineInfo { + fn detect() -> Self { + Self { + os: std::env::consts::OS.to_string(), + arch: std::env::consts::ARCH.to_string(), + cpus: std::thread::available_parallelism().map(Into::into).unwrap_or(0), + } + } + + fn describe(&self) -> String { + format!("{}/{} ({} cpus)", self.os, self.arch, self.cpus) + } +} + +#[derive(Serialize, Deserialize, PartialEq, Eq)] +struct CorpusInfo { + path: String, + git_commit: Option, + tracked_files: usize, + owned_files: usize, + codeowners_lines: usize, + /// True when the corpus is too small for the numbers to mean anything. + smoke_scale: bool, +} + +#[derive(Serialize, Deserialize)] +struct CaseResult { + name: String, + status: String, + #[serde(skip_serializing_if = "Option::is_none")] + skip_reason: Option, + file_count: usize, + runs_ms: Vec, + best_ms: u128, + median_ms: u128, + /// Span durations from the best run. Nested spans are inclusive of children, + /// so these do not sum to `best_ms`. + phases_ms: BTreeMap, + validation_errors: usize, + io_errors: usize, +} + +// --------------------------------------------------------------------------- +// Phase collection +// +// The library is already instrumented with `#[instrument]` spans. We install a +// subscriber layer that accumulates span durations by name into a global map, +// clearing it between runs. Runs are strictly sequential, so a global is safe. +// --------------------------------------------------------------------------- + +fn phase_totals() -> &'static Mutex> { + static TOTALS: OnceLock>> = OnceLock::new(); + TOTALS.get_or_init(|| Mutex::new(BTreeMap::new())) +} + +struct SpanStart(Instant); + +struct PhaseLayer; + +impl Layer for PhaseLayer +where + S: tracing::Subscriber + for<'a> LookupSpan<'a>, +{ + fn on_new_span(&self, _attrs: &tracing::span::Attributes<'_>, id: &tracing::Id, ctx: Context<'_, S>) { + if let Some(span) = ctx.span(id) { + span.extensions_mut().insert(SpanStart(Instant::now())); + } + } + + fn on_close(&self, id: tracing::Id, ctx: Context<'_, S>) { + let Some(span) = ctx.span(&id) else { return }; + let elapsed = { + let ext = span.extensions(); + match ext.get::() { + Some(start) => start.0.elapsed(), + None => return, + } + }; + let name = span.name().to_string(); + if let Ok(mut totals) = phase_totals().lock() { + *totals.entry(name).or_default() += elapsed; + } + } +} + +fn reset_phases() { + if let Ok(mut totals) = phase_totals().lock() { + totals.clear(); + } +} + +fn take_phases() -> BTreeMap { + match phase_totals().lock() { + Ok(totals) => totals.iter().map(|(k, v)| (k.clone(), v.as_millis())).collect(), + Err(_) => BTreeMap::new(), + } +} + +// --------------------------------------------------------------------------- +// Corpus resolution +// --------------------------------------------------------------------------- + +fn resolve_corpus(flag: Option) -> Result { + let candidate = if let Some(path) = flag { + path + } else if let Some(env) = std::env::var_os("CODEOWNERS_PERF_CORPUS").filter(|v| !v.is_empty()) { + PathBuf::from(env) + } else { + // The committed fixture: self-contained, works on a clean clone, and keeps + // any reference to a specific large monorepo out of the repository. + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/valid_project") + }; + + let corpus = candidate + .canonicalize() + .map_err(|e| format!("corpus {} is not readable: {e}", candidate.display()))?; + + let config = corpus.join("config/code_ownership.yml"); + if !config.is_file() { + return Err(format!( + "corpus {} has no config/code_ownership.yml — is it a codeowners project?", + corpus.display() + )); + } + Ok(corpus) +} + +fn git_output(dir: &Path, args: &[&str]) -> Option { + let out = Command::new("git").arg("-C").arg(dir).args(args).output().ok()?; + if !out.status.success() { + return None; + } + Some(String::from_utf8_lossy(&out.stdout).into_owned()) +} + +/// Tracked files in the corpus, relative to its root. +fn tracked_files(corpus: &Path) -> Vec { + match git_output(corpus, &["ls-files"]) { + Some(stdout) => stdout.lines().map(str::to_string).collect(), + None => Vec::new(), + } +} + +fn matches_any(path: &str, globs: &[String]) -> bool { + globs.iter().any(|glob| fast_glob::glob_match(glob, path)) +} + +/// The deterministic candidate pool for `validate ` cases: tracked files +/// that survive the same owned/unowned glob filter `validate_files` applies, in +/// sorted order so every branch measures the same paths. +fn owned_file_pool(tracked: &[String], config: &Config) -> Vec { + let mut pool: Vec = tracked + .iter() + .filter(|path| matches_any(path, &config.owned_globs) && !matches_any(path, &config.unowned_globs)) + .cloned() + .collect(); + pool.sort(); + pool.dedup(); + pool +} + +fn codeowners_path(corpus: &Path, config: &Config) -> PathBuf { + corpus.join(&config.codeowners_path).join("CODEOWNERS") +} + +/// Restores the corpus CODEOWNERS file on drop. +/// +/// `generate` and `generate-and-validate` write to the corpus. Leaving someone's +/// working repo modified because they ran a benchmark is not acceptable, so we +/// snapshot the file up front and put it back afterwards. +struct CodeownersGuard { + path: PathBuf, + original: Option, +} + +impl CodeownersGuard { + fn acquire(corpus: &Path, config: &Config) -> Result { + let path = codeowners_path(corpus, config); + + // Refuse to run against a corpus whose CODEOWNERS is already modified: we + // could not tell our own writes apart from the user's, and restoring would + // silently clobber their work. + if let Some(relative) = path.strip_prefix(corpus).ok().map(|p| p.to_string_lossy().into_owned()) + && let Some(status) = git_output(corpus, &["status", "--porcelain", "--", &relative]) + && !status.trim().is_empty() + { + return Err(format!( + "corpus CODEOWNERS has uncommitted changes ({}). Commit or stash it first — \ + the harness rewrites this file and restores it afterwards.", + relative.trim() + )); + } + + let original = std::fs::read_to_string(&path).ok(); + Ok(Self { path, original }) + } +} + +impl Drop for CodeownersGuard { + fn drop(&mut self) { + if let Some(original) = &self.original + && let Err(e) = std::fs::write(&self.path, original) + { + eprintln!("warning: failed to restore {}: {e}", self.path.display()); + } + } +} + +// --------------------------------------------------------------------------- +// Cases +// --------------------------------------------------------------------------- + +#[derive(Clone, Copy)] +enum Kind { + Generate, + Validate, + GenerateAndValidate, +} + +#[derive(Clone, Copy)] +struct Case { + name: &'static str, + kind: Kind, + files: usize, + no_cache: bool, +} + +const CASES: &[Case] = &[ + Case { + name: "generate", + kind: Kind::Generate, + files: 0, + no_cache: false, + }, + Case { + name: "validate_all", + kind: Kind::Validate, + files: 0, + no_cache: false, + }, + Case { + name: "gv", + kind: Kind::GenerateAndValidate, + files: 0, + no_cache: false, + }, + // `gv ` is the likely real-world pre-commit / CI invocation, and it is + // not interchangeable with `validate `: generate needs the project + // build, so an optimization that bypasses that build cannot apply here. + // Measured separately so a win on `validate ` is never mistaken for a + // win on the command people actually run. + Case { + name: "gv_files_100", + kind: Kind::GenerateAndValidate, + files: 100, + no_cache: false, + }, + Case { + name: "gv_files_1000", + kind: Kind::GenerateAndValidate, + files: 1000, + no_cache: false, + }, + Case { + name: "validate_all_cold", + kind: Kind::Validate, + files: 0, + no_cache: true, + }, + Case { + name: "validate_files_1", + kind: Kind::Validate, + files: 1, + no_cache: false, + }, + Case { + name: "validate_files_100", + kind: Kind::Validate, + files: 100, + no_cache: false, + }, + Case { + name: "validate_files_1000", + kind: Kind::Validate, + files: 1000, + no_cache: false, + }, + Case { + name: "validate_files_2000", + kind: Kind::Validate, + files: 2000, + no_cache: false, + }, +]; + +fn run_case(case: &Case, corpus: &Path, files: &[String]) -> RunResult { + let run_config = RunConfig { + project_root: corpus.to_path_buf(), + config_path: corpus.join("config/code_ownership.yml"), + codeowners_file_path: None, + no_cache: case.no_cache, + executable_name: None, + }; + let files = files.to_vec(); + match case.kind { + Kind::Generate => runner::generate(&run_config, false), + Kind::Validate => runner::validate(&run_config, files), + Kind::GenerateAndValidate => runner::generate_and_validate(&run_config, files, false), + } +} + +/// Observed run-to-run spread (max - min). The harness's own precision floor for +/// a case: deltas smaller than this cannot be distinguished from noise. +fn spread(runs: &[u128]) -> u128 { + match (runs.iter().min(), runs.iter().max()) { + (Some(lo), Some(hi)) => hi - lo, + _ => 0, + } +} + +fn median(sorted: &[u128]) -> u128 { + match sorted.len() { + 0 => 0, + n => sorted[n / 2], + } +} + +#[allow(clippy::too_many_arguments)] +fn measure(case: &Case, corpus: &Path, pool: &[String], runs: usize, warmup: usize) -> CaseResult { + if case.files > pool.len() { + return CaseResult { + name: case.name.to_string(), + status: "skipped".to_string(), + skip_reason: Some(format!("needs {} owned files, corpus has {}", case.files, pool.len())), + file_count: 0, + runs_ms: vec![], + best_ms: 0, + median_ms: 0, + phases_ms: BTreeMap::new(), + validation_errors: 0, + io_errors: 0, + }; + } + + let files: Vec = pool.iter().take(case.files).cloned().collect(); + + // The filter regression that fooled me during profiling: a bad argument list + // silently matches nothing and the per-file loop never runs, producing a + // suspiciously fast number. Fail loudly instead. + assert_eq!( + files.len(), + case.files, + "case {} expected {} paths but built {}", + case.name, + case.files, + files.len() + ); + + for _ in 0..warmup { + run_case(case, corpus, &files); + } + + let mut timings = Vec::with_capacity(runs); + let mut phases = BTreeMap::new(); + let mut last = RunResult::default(); + let mut best = u128::MAX; + + for _ in 0..runs.max(1) { + reset_phases(); + let start = Instant::now(); + let result = run_case(case, corpus, &files); + let elapsed = start.elapsed().as_millis(); + timings.push(elapsed); + if elapsed < best { + best = elapsed; + phases = take_phases(); + } + last = result; + } + + let mut sorted = timings.clone(); + sorted.sort_unstable(); + + CaseResult { + name: case.name.to_string(), + status: "ok".to_string(), + skip_reason: None, + file_count: case.files, + best_ms: sorted.first().copied().unwrap_or(0), + median_ms: median(&sorted), + runs_ms: timings, + phases_ms: phases, + validation_errors: last.validation_errors.len(), + io_errors: last.io_errors.len(), + } +} + +// --------------------------------------------------------------------------- +// Commands +// --------------------------------------------------------------------------- + +fn cmd_run(corpus: Option, case_filter: Option, runs: usize, warmup: usize, json: bool) -> Result<(), String> { + let corpus = resolve_corpus(corpus)?; + let config = Config::load_from_path(&corpus.join("config/code_ownership.yml"))?; + + let tracked = tracked_files(&corpus); + let pool = owned_file_pool(&tracked, &config); + let codeowners_lines = std::fs::read_to_string(codeowners_path(&corpus, &config)) + .map(|s| s.lines().count()) + .unwrap_or(0); + + let info = CorpusInfo { + path: corpus.to_string_lossy().into_owned(), + git_commit: git_output(&corpus, &["rev-parse", "HEAD"]).map(|s| s.trim().to_string()), + tracked_files: tracked.len(), + owned_files: pool.len(), + codeowners_lines, + smoke_scale: tracked.len() < SMOKE_SCALE_MAX_FILES, + }; + + if info.smoke_scale && !json { + eprintln!( + "warning: corpus is {} ({} tracked files) — smoke-test scale.\n\ + \x20 Results are NOT comparable. Set CODEOWNERS_PERF_CORPUS to a large monorepo\n\ + \x20 (see perf/README.md) for real measurements.\n", + info.path, info.tracked_files + ); + } + + // Held for the whole run; restores the corpus CODEOWNERS on drop. + let _guard = CodeownersGuard::acquire(&corpus, &config)?; + + let selected: Vec<&Case> = CASES + .iter() + .filter(|c| case_filter.as_ref().is_none_or(|f| c.name.contains(f.as_str()))) + .collect(); + if selected.is_empty() { + return Err("no cases matched --case".to_string()); + } + + let mut results = Vec::new(); + for case in selected { + if !json { + eprint!(" {} ... ", case.name); + } + let result = measure(case, &corpus, &pool, runs, warmup); + if !json { + match result.status.as_str() { + "skipped" => eprintln!("skipped ({})", result.skip_reason.clone().unwrap_or_default()), + _ => eprintln!("{} ms", result.best_ms), + } + } + results.push(result); + } + + let report = Report { + tool_version: runner::version(), + machine: MachineInfo::detect(), + corpus: info, + runs_per_case: runs, + cases: results, + }; + + if json { + println!("{}", serde_json::to_string_pretty(&report).map_err(|e| e.to_string())?); + } else { + print_table(&report); + } + Ok(()) +} + +fn print_table(report: &Report) { + println!(); + println!("corpus: {}", report.corpus.path); + println!("machine: {}", report.machine.describe()); + println!( + "scale: {} tracked files, {} owned, {} CODEOWNERS lines{}", + report.corpus.tracked_files, + report.corpus.owned_files, + report.corpus.codeowners_lines, + if report.corpus.smoke_scale { " [SMOKE SCALE]" } else { "" } + ); + if let Some(commit) = &report.corpus.git_commit { + println!("commit: {commit}"); + } + println!("runs: {} (best reported)", report.runs_per_case); + println!(); + println!("{:<22} {:>10} {:>10} notes", "case", "best", "median"); + println!("{}", "-".repeat(72)); + for case in &report.cases { + if case.status == "skipped" { + println!( + "{:<22} {:>10} {:>10} {}", + case.name, + "-", + "-", + case.skip_reason.clone().unwrap_or_default() + ); + continue; + } + let notes = if case.validation_errors > 0 || case.io_errors > 0 { + format!("{} validation, {} io errors", case.validation_errors, case.io_errors) + } else { + String::new() + }; + println!("{:<22} {:>9}ms {:>9}ms {}", case.name, case.best_ms, case.median_ms, notes); + } + + println!(); + println!("phase breakdown (best run, nested spans are inclusive of children)"); + println!("{}", "-".repeat(72)); + for case in &report.cases { + if case.phases_ms.is_empty() { + continue; + } + println!("{}:", case.name); + for (phase, ms) in &case.phases_ms { + println!(" {phase:<34} {ms:>7}ms"); + } + } +} + +fn read_report(path: &Path) -> Result { + let text = std::fs::read_to_string(path).map_err(|e| format!("{}: {e}", path.display()))?; + serde_json::from_str(&text).map_err(|e| format!("{}: {e}", path.display())) +} + +fn cmd_compare(baseline_path: &Path, candidate_path: &Path) -> Result<(), String> { + let baseline = read_report(baseline_path)?; + let candidate = read_report(candidate_path)?; + + // Comparing across corpora is the subtle version of the smoke-scale mistake: + // a fixture-measured branch against a monorepo-measured baseline reads as a + // spectacular speedup. Refuse rather than mislead. + if baseline.corpus.path != candidate.corpus.path { + return Err(format!( + "refusing to compare: different corpora\n baseline: {}\n candidate: {}", + baseline.corpus.path, candidate.corpus.path + )); + } + if baseline.corpus.git_commit != candidate.corpus.git_commit { + return Err(format!( + "refusing to compare: corpus moved between runs\n baseline: {:?}\n candidate: {:?}\n\ + Re-measure both sides against the same corpus commit.", + baseline.corpus.git_commit, candidate.corpus.git_commit + )); + } + + let candidates: BTreeMap<&str, &CaseResult> = candidate.cases.iter().map(|c| (c.name.as_str(), c)).collect(); + + println!("corpus: {} ({} tracked files)", baseline.corpus.path, baseline.corpus.tracked_files); + println!("machine: {}", baseline.machine.describe()); + if baseline.corpus.smoke_scale { + println!(); + println!("**Smoke-scale corpus — these numbers are not meaningful for comparison.**"); + } + // Not fatal like a corpus mismatch, but wall-clock across different hardware + // is not a like-for-like comparison and the reader needs to know. + if baseline.machine != candidate.machine { + println!(); + println!( + "**Warning: different machines ({} vs {}). Wall-clock numbers are not comparable across hardware.**", + baseline.machine.describe(), + candidate.machine.describe() + ); + } + println!(); + println!("| Case | Baseline | Candidate | Delta | Speedup | Noise | Verdict |"); + println!("|---|---:|---:|---:|---:|---:|---|"); + for base in &baseline.cases { + let Some(cand) = candidates.get(base.name.as_str()) else { + println!("| {} | {}ms | — | missing | — | — | — |", base.name, base.best_ms); + continue; + }; + if base.status == "skipped" || cand.status == "skipped" { + println!("| {} | skipped | skipped | — | — | — | — |", base.name); + continue; + } + let delta = cand.best_ms as i128 - base.best_ms as i128; + let speedup = if cand.best_ms > 0 { + base.best_ms as f64 / cand.best_ms as f64 + } else { + 0.0 + }; + // A delta smaller than the run-to-run spread is not a result. Reporting + // `best` alone hides this: min-of-N is a biased estimator with no + // dispersion attached, so a 3% "win" on a case that swings 40% between + // runs reads exactly like a real one. + let noise = spread(&base.runs_ms).max(spread(&cand.runs_ms)); + let verdict = if delta.unsigned_abs() <= noise { "**within noise**" } else { "" }; + println!( + "| {} | {}ms | {}ms | {}{}ms | {:.2}x | ±{}ms | {} |", + base.name, + base.best_ms, + cand.best_ms, + if delta > 0 { "+" } else { "" }, + delta, + speedup, + noise, + verdict + ); + } + Ok(()) +} + +fn main() { + tracing_subscriber::registry() + .with(PhaseLayer) + .with(tracing_subscriber::EnvFilter::new("codeowners=debug")) + .init(); + + let cli = Cli::parse(); + let result = match cli.command { + Cmd::Run { + corpus, + case, + runs, + warmup, + json, + } => cmd_run(corpus, case, runs, warmup, json), + Cmd::Compare { baseline, candidate } => cmd_compare(&baseline, &candidate), + Cmd::Cases => { + for case in CASES { + println!("{}", case.name); + } + Ok(()) + } + }; + + if let Err(err) = result { + eprintln!("error: {err}"); + std::process::exit(1); + } +} diff --git a/src/ownership.rs b/src/ownership.rs index 6b36193..bbd2099 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -116,7 +116,7 @@ impl Ownership { } } - #[instrument(level = "debug", skip_all)] + #[instrument(name = "ownership_validate", level = "debug", skip_all)] pub fn validate(&self) -> Result<(), ValidatorErrors> { info!("validating file ownership"); let validator = Validator { @@ -170,6 +170,7 @@ impl Ownership { file_generator.generate_file() } + #[instrument(name = "mapper_build", level = "debug", skip_all)] fn mappers(&self) -> Vec> { vec![ Box::new(TeamFileMapper::build(self.project.clone())), diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 65fb648..e7362d8 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -36,7 +36,7 @@ enum Error { pub struct Errors(Vec); impl Validator { - #[instrument(level = "debug", skip_all)] + #[instrument(name = "validator_validate", level = "debug", skip_all)] pub fn validate(&self) -> Result<(), Errors> { let mut validation_errors = Vec::new(); @@ -56,6 +56,7 @@ impl Validator { } } + #[instrument(name = "validate_invalid_team", level = "debug", skip_all)] fn validate_invalid_team(&self) -> Vec { debug!("validating project"); let mut errors: Vec = Vec::new(); @@ -106,6 +107,7 @@ impl Validator { .collect() } + #[instrument(name = "validate_file_ownership", level = "debug", skip_all)] fn validate_file_ownership(&self) -> Vec { let mut validation_errors = Vec::new(); @@ -125,6 +127,7 @@ impl Validator { validation_errors } + #[instrument(name = "validate_codeowners_file", level = "debug", skip_all)] fn validate_codeowners_file(&self) -> Vec { let generated_file = self.file_generator.generate_file(); let current_file = self.project.get_codeowners_file().unwrap_or_default(); @@ -139,6 +142,7 @@ impl Validator { } } + #[instrument(name = "file_to_owners", level = "debug", skip_all)] fn file_to_owners(&self) -> Vec<(&ProjectFile, Vec)> { let owner_matchers: Vec = self.mappers.iter().flat_map(|mapper| mapper.owner_matchers()).collect(); let file_owner_finder = FileOwnerFinder { diff --git a/src/project_builder.rs b/src/project_builder.rs index 7652bbd..193791b 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -52,7 +52,7 @@ impl<'a> ProjectBuilder<'a> { } } - #[instrument(level = "debug", skip_all, fields(base_path = %self.base_path.display()))] + #[instrument(name = "project_build", level = "debug", skip_all, fields(base_path = %self.base_path.display()))] pub fn build(&mut self) -> Result> { tracing::info!("Starting project build"); let mut builder = WalkBuilder::new(&self.base_path); diff --git a/src/runner.rs b/src/runner.rs index c69f712..5562979 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -4,6 +4,7 @@ use std::process::Command; use error_stack::{Report, ResultExt}; use fast_glob::glob_match; use serde::Serialize; +use tracing::debug_span; use crate::{ cache::{Cache, Caching, file::GlobalCache, noop::NoopCache}, @@ -80,20 +81,22 @@ pub(crate) fn resolve_codeowners_file_path(run_config: &RunConfig, config: &Conf impl Runner { pub fn new(run_config: &RunConfig) -> Result> { - let config = config_from_run_config(run_config)?; + let config = debug_span!("config_load").in_scope(|| config_from_run_config(run_config))?; let codeowners_file_path = resolve_codeowners_file_path(run_config, &config); - let cache: Cache = if run_config.no_cache { - NoopCache::default().into() - } else { - GlobalCache::new(run_config.project_root.clone(), config.cache_directory.clone()) - .change_context(Error::Io(format!( - "Can't create cache: {}", - run_config.config_path.to_string_lossy() - ))) - .attach(format!("Can't create cache: {}", run_config.config_path.to_string_lossy()))? - .into() - }; + let cache: Cache = debug_span!("cache_init").in_scope(|| -> Result> { + if run_config.no_cache { + Ok(NoopCache::default().into()) + } else { + Ok(GlobalCache::new(run_config.project_root.clone(), config.cache_directory.clone()) + .change_context(Error::Io(format!( + "Can't create cache: {}", + run_config.config_path.to_string_lossy() + ))) + .attach(format!("Can't create cache: {}", run_config.config_path.to_string_lossy()))? + .into()) + } + })?; let mut project_builder = ProjectBuilder::new(&config, run_config.project_root.clone(), codeowners_file_path.clone(), &cache); let project = project_builder.build().change_context(Error::Io(format!( @@ -102,10 +105,12 @@ impl Runner { )))?; let ownership = Ownership::build(project); - cache.persist_cache().change_context(Error::Io(format!( - "Can't persist cache: {}", - run_config.config_path.to_string_lossy() - )))?; + debug_span!("cache_persist").in_scope(|| { + cache.persist_cache().change_context(Error::Io(format!( + "Can't persist cache: {}", + run_config.config_path.to_string_lossy() + ))) + })?; Ok(Self { run_config: run_config.clone(), @@ -159,13 +164,15 @@ impl Runner { }) .collect(); - for file_path in filtered_paths { - match team_for_file_from_codeowners(&self.run_config, &file_path) { - Ok(Some(_)) => {} - Ok(None) => unowned_files.push(file_path), - Err(err) => io_errors.push(format!("{}: {}", file_path, err)), + debug_span!("per_file_query").in_scope(|| { + for file_path in filtered_paths { + match team_for_file_from_codeowners(&self.run_config, &file_path) { + Ok(Some(_)) => {} + Ok(None) => unowned_files.push(file_path), + Err(err) => io_errors.push(format!("{}: {}", file_path, err)), + } } - } + }); if !unowned_files.is_empty() { let validation_errors = std::iter::once("Unowned files detected:".to_string()) diff --git a/tests/perf_harness_test.rs b/tests/perf_harness_test.rs new file mode 100644 index 0000000..9175051 --- /dev/null +++ b/tests/perf_harness_test.rs @@ -0,0 +1,245 @@ +//! Tests for the performance harness binary. +//! +//! These assert the harness *works* — cases execute, JSON is well-formed, the +//! corpus is restored, bad comparisons are rejected. They deliberately assert +//! nothing about how *fast* anything is: timings on a shared CI runner are +//! meaningless, and the harness is a local tool (see perf/README.md). + +use std::error::Error; +use std::path::Path; + +mod common; + +use common::git_add_all_files; +use common::setup_fixture_repo; + +fn perf_cmd() -> Result> { + Ok(assert_cmd::Command::cargo_bin("codeowners-perf")?) +} + +/// A real corpus is a committed repository. The harness refuses to run when +/// CODEOWNERS has uncommitted changes, so tests have to commit like the real +/// thing does. +fn commit_all(path: &Path) { + git_add_all_files(path); + let output = std::process::Command::new("git") + .args(["commit", "-m", "fixture", "--no-verify"]) + .current_dir(path) + .output() + .expect("failed to run git commit"); + assert!( + output.status.success(), + "git commit failed: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +/// Fixture corpus, committed and ready to measure. +fn corpus() -> tempfile::TempDir { + let temp_dir = setup_fixture_repo(Path::new("tests/fixtures/valid_project")); + commit_all(temp_dir.path()); + temp_dir +} + +#[test] +fn test_lists_cases() -> Result<(), Box> { + let output = perf_cmd()?.arg("cases").assert().success(); + let stdout = String::from_utf8(output.get_output().stdout.clone())?; + + for expected in ["generate", "validate_all", "gv", "validate_all_cold", "validate_files_1000"] { + assert!(stdout.contains(expected), "case list missing {expected}: {stdout}"); + } + Ok(()) +} + +#[test] +fn test_run_against_fixture_emits_well_formed_json() -> Result<(), Box> { + let temp_dir = corpus(); + let project_root = temp_dir.path(); + + let output = perf_cmd()? + .arg("run") + .arg("--corpus") + .arg(project_root) + .args(["--runs", "1", "--warmup", "0", "--json"]) + .assert() + .success(); + + let stdout = String::from_utf8(output.get_output().stdout.clone())?; + let report: serde_json::Value = serde_json::from_str(&stdout)?; + + // The corpus metadata is what makes reports comparable, so it must be present. + assert!(report["corpus"]["tracked_files"].as_u64().unwrap() > 0); + assert!(report["corpus"]["codeowners_lines"].as_u64().unwrap() > 0); + assert_eq!( + report["corpus"]["smoke_scale"], true, + "the test fixture must be reported as smoke scale" + ); + + let cases = report["cases"].as_array().expect("cases array"); + assert!(!cases.is_empty()); + + // Every case either ran or explains why it did not. + for case in cases { + let status = case["status"].as_str().unwrap(); + assert!(status == "ok" || status == "skipped", "unexpected status {status}"); + if status == "skipped" { + assert!(case["skip_reason"].is_string(), "skipped case must give a reason"); + } else { + assert!(!case["runs_ms"].as_array().unwrap().is_empty()); + } + } + Ok(()) +} + +#[test] +fn test_skips_cases_the_corpus_is_too_small_for() -> Result<(), Box> { + let temp_dir = corpus(); + let project_root = temp_dir.path(); + + let output = perf_cmd()? + .arg("run") + .arg("--corpus") + .arg(project_root) + .args(["--runs", "1", "--warmup", "0", "--case", "validate_files_2000", "--json"]) + .assert() + .success(); + + let stdout = String::from_utf8(output.get_output().stdout.clone())?; + let report: serde_json::Value = serde_json::from_str(&stdout)?; + let case = &report["cases"][0]; + + // Silently measuring fewer files than the case name claims would be the worst + // possible failure mode for a benchmark. + assert_eq!(case["status"], "skipped"); + assert!( + case["skip_reason"].as_str().unwrap().contains("2000"), + "skip reason should name the shortfall: {case}" + ); + Ok(()) +} + +#[test] +fn test_restores_corpus_codeowners_after_run() -> Result<(), Box> { + let temp_dir = corpus(); + let project_root = temp_dir.path(); + + let codeowners = project_root.join(".github/CODEOWNERS"); + let before = std::fs::read_to_string(&codeowners)?; + + // `generate` and `gv` both write this file during the run. + perf_cmd()? + .arg("run") + .arg("--corpus") + .arg(project_root) + .args(["--runs", "1", "--warmup", "0"]) + .assert() + .success(); + + let after = std::fs::read_to_string(&codeowners)?; + assert_eq!(before, after, "harness must leave the corpus CODEOWNERS untouched"); + Ok(()) +} + +#[test] +fn test_refuses_to_run_against_dirty_corpus_codeowners() -> Result<(), Box> { + let temp_dir = corpus(); + let project_root = temp_dir.path(); + + // Simulate a user with uncommitted CODEOWNERS work. If the harness were killed + // mid-run this content would be unrecoverable, so it must refuse up front + // rather than overwrite and hope the restore lands. + let codeowners = project_root.join(".github/CODEOWNERS"); + std::fs::write(&codeowners, "# work in progress\n")?; + + perf_cmd()? + .arg("run") + .arg("--corpus") + .arg(project_root) + .assert() + .failure() + .stderr(predicates::str::contains("uncommitted changes")); + + // And the refusal must not have touched it. + assert_eq!(std::fs::read_to_string(&codeowners)?, "# work in progress\n"); + Ok(()) +} + +#[test] +fn test_rejects_corpus_without_config() -> Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + + perf_cmd()? + .arg("run") + .arg("--corpus") + .arg(temp_dir.path()) + .assert() + .failure() + .stderr(predicates::str::contains("code_ownership.yml")); + Ok(()) +} + +#[test] +fn test_compare_refuses_mismatched_corpora() -> Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let baseline = temp_dir.path().join("baseline.json"); + let candidate = temp_dir.path().join("candidate.json"); + + let report = |path: &str, commit: &str| { + format!( + r#"{{"tool_version":"0.0.0","runs_per_case":1,"cases":[], + "corpus":{{"path":"{path}","git_commit":"{commit}","tracked_files":10, + "owned_files":5,"codeowners_lines":5,"smoke_scale":true}}}}"# + ) + }; + + // Different corpus entirely — e.g. fixture-measured branch vs monorepo baseline. + std::fs::write(&baseline, report("/corpus/a", "abc"))?; + std::fs::write(&candidate, report("/corpus/b", "abc"))?; + perf_cmd()? + .arg("compare") + .arg(&baseline) + .arg(&candidate) + .assert() + .failure() + .stderr(predicates::str::contains("different corpora")); + + // Same corpus, but it moved between the two measurements. + std::fs::write(&candidate, report("/corpus/a", "def"))?; + perf_cmd()? + .arg("compare") + .arg(&baseline) + .arg(&candidate) + .assert() + .failure() + .stderr(predicates::str::contains("corpus moved")); + + Ok(()) +} + +#[test] +fn test_compare_emits_delta_table_for_matching_corpora() -> Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let baseline = temp_dir.path().join("baseline.json"); + let candidate = temp_dir.path().join("candidate.json"); + + let report = |ms: u64| { + format!( + r#"{{"tool_version":"0.0.0","runs_per_case":1, + "cases":[{{"name":"gv","status":"ok","file_count":0,"runs_ms":[{ms}], + "best_ms":{ms},"median_ms":{ms},"phases_ms":{{}}, + "validation_errors":0,"io_errors":0}}], + "corpus":{{"path":"/corpus/a","git_commit":"abc","tracked_files":10, + "owned_files":5,"codeowners_lines":5,"smoke_scale":false}}}}"# + ) + }; + std::fs::write(&baseline, report(1000))?; + std::fs::write(&candidate, report(250))?; + + let output = perf_cmd()?.arg("compare").arg(&baseline).arg(&candidate).assert().success(); + + let stdout = String::from_utf8(output.get_output().stdout.clone())?; + assert!(stdout.contains("| gv |"), "missing case row: {stdout}"); + assert!(stdout.contains("4.00x"), "missing speedup: {stdout}"); + Ok(()) +}