From b03d91fb0447f01f5592955f480ee0f7589ba9a1 Mon Sep 17 00:00:00 2001 From: fishidaho Date: Wed, 2 Sep 2026 17:51:10 -0700 Subject: [PATCH 1/4] Add a benchmark suite and wire it into CI as a regression gate There was no benchmark suite and no CI job that would notice a cost regression. Correctness tests don't help here: the answers stay right while the memory or the file size quietly doubles, which is exactly the class of defect the v0.2 work is fixing. `benchmarks/` holds a harness, five fast cases run on every PR, one larger case for a scheduled or manual run, and checked-in ceilings. Two decisions worth stating, since a flaky or meaningless gate is worse than none: Timing is never recorded as absolute seconds. Each timing case runs the same work through scipy in the same process and records the *ratio*, which cancels most of the difference between a laptop and a shared runner, and is gated loosely (4x) because it's still the noisy one. The sharp gates are the deterministic numbers: bytes per stored nonzero (1.1x) and memory allocated by an operation (2x). Memory is measured with tracemalloc, not ru_maxrss. RSS is a process-lifetime high-water mark, so an operation staying under the peak set while building its input reports zero however much it allocates -- the first version of this suite duly reported 0 MB for an operation allocating 66 MB. tracemalloc measures allocations and resets between runs. Recorded against main, the two memory cases show what the rest of v0.2 is about: a minor-axis reduction on a 4M-nonzero array allocates 66 MB, and a misaligned-direction matmul allocates 204 MB against a 16 MB array. The ceilings are therefore deliberately generous today and should be re-recorded once those fixes land; README.md says so and names the numbers. Each case runs in its own subprocess, since measurement state and JIT warm-up would otherwise leak between them. The fast set takes ~28s. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 23 ++++++ benchmarks/README.md | 77 +++++++++++++++++++ benchmarks/__init__.py | 0 benchmarks/baselines.json | 29 ++++++++ benchmarks/cases.py | 151 ++++++++++++++++++++++++++++++++++++++ benchmarks/harness.py | 94 ++++++++++++++++++++++++ benchmarks/run.py | 111 ++++++++++++++++++++++++++++ 7 files changed, 485 insertions(+) create mode 100644 benchmarks/README.md create mode 100644 benchmarks/__init__.py create mode 100644 benchmarks/baselines.json create mode 100644 benchmarks/cases.py create mode 100644 benchmarks/harness.py create mode 100644 benchmarks/run.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 11d0404..d5f4be3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -55,3 +55,26 @@ jobs: - name: Run pytest run: uv run pytest --cov + + benchmarks: + name: Benchmark Regression Gate + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v5 + + - name: Install uv + uses: astral-sh/setup-uv@v10.0.1 + with: + enable-cache: true + python-version: "3.12" + + - name: Install dependencies + run: uv sync --all-extras --dev + + # Fails if a gated metric exceeds its ceiling in benchmarks/baselines.json. + # Memory and layout numbers are deterministic and gated tightly; timing is + # recorded as a ratio against scipy in the same process, and gated loosely, + # because a shared runner's absolute speed means nothing. See benchmarks/README.md. + - name: Run benchmark gate + run: uv run python -m benchmarks.run --set fast diff --git a/benchmarks/README.md b/benchmarks/README.md new file mode 100644 index 0000000..70aa924 --- /dev/null +++ b/benchmarks/README.md @@ -0,0 +1,77 @@ +# Benchmarks + +A small suite that CI runs as a regression gate, plus a larger set for +running by hand or on a schedule. + +```sh +uv run python -m benchmarks.run --set fast # run + compare to baselines (what CI does) +uv run python -m benchmarks.run --set slow # the larger cases +uv run python -m benchmarks.run --case matvec_vs_scipy +uv run python -m benchmarks.run --set fast --record # rewrite baselines.json +``` + +Exits nonzero if any gated metric exceeds its recorded ceiling. + +## What it measures, and why those things + +Correctness tests don't catch cost regressions: the answers stay right while +the memory or the file doubles. These are the two properties that can move +silently. + +**Layout size** — bytes per stored nonzero, and the same figure for the +`indices` array alone. Deterministic, so it's an exact gate. It moves if an +index dtype widens, if value deduplication stops working, or if a new array +joins the layout. + +**Memory allocated by an operation** — measured with `tracemalloc`, not +`ru_maxrss`. RSS is a process-lifetime high-water mark, so an operation that +stays under the peak set while building its input reports zero no matter how +much it allocates; that made the first version of this suite report `0` for +an operation allocating 66 MB. `tracemalloc` measures allocations and can be +reset between runs. It traces numpy but not numba's internal allocations, +which suits what's being guarded here: the failure mode is a numpy-level +temporary the size of the data, not a kernel's own scratch. + +**Throughput relative to scipy** — never absolute seconds. The same work is +timed through scipy in the same process and the *ratio* is recorded, which +cancels most of the difference between a fast laptop and a shared CI runner. +It's still the noisiest thing here, which is why the timing gates carry a +much looser margin (4×) than the memory ones (2×) and the layout ones (1.1×). + +Every case runs in its own subprocess, because measurement state and JIT +warm-up would otherwise leak between them. + +## Baselines + +`baselines.json` holds a ceiling per gated metric. `--record` regenerates +them by multiplying a fresh measurement by that metric's margin. Only metrics +named in `margins` are gated; anything else a case returns is recorded for +context (e.g. `expanded_nnz_mb`, which says what a per-nonzero temporary +*would* have cost, so the gated number next to it can be read in proportion). + +The checked-in ceilings were recorded on `main` before the v0.2 memory fixes +landed, so two of them are deliberately generous: + +| metric | recorded on `main` | +|---|---| +| `minor_sum_peak_mb.peak_alloc_mb` | 66 MB, for a reduction on a 4M-nonzero array | +| `misaligned_matmul_peak_mb.peak_alloc_mb` | 204 MB, against a 16 MB array | + +Both should drop by one to two orders of magnitude once the reduction and +misaligned-matmul fixes are in. **Re-record after those merge** — until then +these gate against getting worse, not against the current numbers being good. + +## Adding a case + +Write a function returning `{metric: value}` in `cases.py` and decorate it +with `@fast` (runs on every PR — keep it well under a minute) or `@slow`. +Add any new gated metric name to `margins` in `baselines.json`, then +`--record`. + +## The larger suite + +`--set slow` isn't wired into the PR gate: those cases build datasets an +order of magnitude bigger and take minutes, which is the wrong thing to put +in front of every push. They're meant for a scheduled workflow or a manual +run before a release. Wiring up that scheduled job is deliberately left as a +follow-up rather than guessed at here. diff --git a/benchmarks/__init__.py b/benchmarks/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/benchmarks/baselines.json b/benchmarks/baselines.json new file mode 100644 index 0000000..d1ef6b4 --- /dev/null +++ b/benchmarks/baselines.json @@ -0,0 +1,29 @@ +{ + "comment": "Ceilings a metric must stay under, regenerated with `python -m benchmarks.run --set fast --record`. Only metrics listed in `margins` are gated; the rest are recorded by cases for context. Margins are the slack applied to a measured value when recording: tight for deterministic layout/memory numbers, loose for timing ratios, which vary with the runner.", + "margins": { + "bytes_per_nonzero": 1.1, + "indices_bytes_per_nonzero": 1.1, + "vs_scipy_ratio": 1.1, + "peak_alloc_mb": 2.0, + "time_ratio_vs_scipy": 4.0 + }, + "cases": { + "layout_bytes_per_nonzero": { + "bytes_per_nonzero": 5.72, + "indices_bytes_per_nonzero": 4.4, + "vs_scipy_ratio": 0.4751 + }, + "minor_sum_peak_mb": { + "peak_alloc_mb": 132.5131 + }, + "misaligned_matmul_peak_mb": { + "peak_alloc_mb": 407.2383 + }, + "matvec_vs_scipy": { + "time_ratio_vs_scipy": 1.8283 + }, + "matmat_vs_scipy": { + "time_ratio_vs_scipy": 1.4318 + } + } +} diff --git a/benchmarks/cases.py b/benchmarks/cases.py new file mode 100644 index 0000000..25e4e53 --- /dev/null +++ b/benchmarks/cases.py @@ -0,0 +1,151 @@ +"""The benchmark cases themselves. + +Each case is a function returning ``{metric_name: value}``. Cases marked +``fast`` run on every pull request; the rest are for the scheduled job and +for running by hand on real data. + +What's here is chosen to cover the claims that are easy to regress silently: +the size of the stored layout, and whether an operation allocates a second +copy of the data. A correctness test won't catch either -- the answers stay +right while the cost quietly doubles. +""" + +from __future__ import annotations + +from collections.abc import Callable + +import numpy as np + +from benchmarks.harness import ( + integer_counts_csr, + peak_alloc_mb, + ratio_vs_scipy, +) + +FAST: dict[str, Callable[[], dict[str, float]]] = {} +SLOW: dict[str, Callable[[], dict[str, float]]] = {} + + +def fast(fn): + FAST[fn.__name__] = fn + return fn + + +def slow(fn): + SLOW[fn.__name__] = fn + return fn + + +# -- layout size ------------------------------------------------------------- + + +@fast +def layout_bytes_per_nonzero() -> dict[str, float]: + """Total bytes the VCS layout holds, per stored nonzero. + + Deterministic, so this is an exact gate rather than a noisy one. It + moves if index dtypes widen, if deduplication stops working, or if a + new array joins the layout. + """ + from vsparse import VCSRArray + + mat = integer_counts_csr(20_000, 2_000, density=0.05) + v = VCSRArray.from_scipy(mat) + stored = v.major_ptr.nbytes + v.values.nbytes + v.value_ptr.nbytes + v.indices.nbytes + scipy_bytes = mat.indptr.nbytes + mat.indices.nbytes + mat.data.nbytes + return { + "bytes_per_nonzero": stored / v.nnz, + "indices_bytes_per_nonzero": v.indices.nbytes / v.nnz, + "vs_scipy_ratio": stored / scipy_bytes, + } + + +# -- memory ceilings --------------------------------------------------------- + + +@fast +def minor_sum_peak_mb() -> dict[str, float]: + """Memory allocated by a minor-axis reduction. + + A reduction produces an ``n_minor``-sized result; anything approaching + the size of the data means an nnz-sized temporary crept back in. + """ + from vsparse import VCSRArray + + v = VCSRArray.from_scipy(integer_counts_csr(40_000, 2_000, density=0.05)) + nnz_mb = v.nnz * 8 / 1e6 + return { + "peak_alloc_mb": peak_alloc_mb(lambda: v.sum(axis=0)), + "expanded_nnz_mb": nnz_mb, # what a per-nonzero temporary would cost + } + + +@fast +def misaligned_matmul_peak_mb() -> dict[str, float]: + """Memory allocated by the matmul direction the storage isn't aligned for. + + The failure mode is a full opposite-format copy of the array, so this + should stay far below the array's own footprint. + """ + from vsparse import VCSCArray + + v = VCSCArray.from_scipy(integer_counts_csr(40_000, 2_000, density=0.05)) + rng = np.random.default_rng(0) + B = rng.normal(size=(v.shape[1], 4)) + array_mb = (v.values.nbytes + v.value_ptr.nbytes + v.indices.nbytes) / 1e6 + + return { + "peak_alloc_mb": peak_alloc_mb(lambda: v.normalized() @ B), + "array_mb": array_mb, # what a full second copy would cost + } + + +# -- throughput, relative to scipy ------------------------------------------- + + +@fast +def matvec_vs_scipy() -> dict[str, float]: + """Aligned-direction matrix-vector product, against scipy's CSR.""" + from vsparse import VCSRArray + + mat = integer_counts_csr(20_000, 2_000, density=0.05) + v = VCSRArray.from_scipy(mat) + rng = np.random.default_rng(0) + x = rng.normal(size=mat.shape[1]) + return {"time_ratio_vs_scipy": ratio_vs_scipy(lambda: v @ x, lambda: mat @ x)} + + +@fast +def matmat_vs_scipy() -> dict[str, float]: + """Aligned-direction matrix-matrix product, against scipy's CSR.""" + from vsparse import VCSRArray + + mat = integer_counts_csr(20_000, 2_000, density=0.05) + v = VCSRArray.from_scipy(mat) + rng = np.random.default_rng(0) + B = rng.normal(size=(mat.shape[1], 8)) + return {"time_ratio_vs_scipy": ratio_vs_scipy(lambda: v @ B, lambda: mat @ B)} + + +# -- larger, for the scheduled job ------------------------------------------- + + +@slow +def large_layout_and_matmul() -> dict[str, float]: + """The same shape of measurement an order of magnitude up.""" + from vsparse import VCSRArray + + mat = integer_counts_csr(200_000, 3_000, density=0.02) + v = VCSRArray.from_scipy(mat) + rng = np.random.default_rng(0) + B = rng.normal(size=(mat.shape[1], 8)) + stored = v.major_ptr.nbytes + v.values.nbytes + v.value_ptr.nbytes + v.indices.nbytes + v @ B + return { + "bytes_per_nonzero": stored / v.nnz, + "time_ratio_vs_scipy": ratio_vs_scipy(lambda: v @ B, lambda: mat @ B), + "matmul_peak_alloc_mb": peak_alloc_mb(lambda: v @ B), + } + + +ALL: dict[str, Callable[[], dict[str, float]]] = {**FAST, **SLOW} diff --git a/benchmarks/harness.py b/benchmarks/harness.py new file mode 100644 index 0000000..000930e --- /dev/null +++ b/benchmarks/harness.py @@ -0,0 +1,94 @@ +"""Measurement primitives for the benchmark suite. + +Two kinds of metric, chosen so that a CI job can compare them against a +checked-in threshold without the result depending on which runner it landed +on: + +*Memory and layout* numbers are deterministic -- bytes per nonzero is a +property of the data structure, and peak RSS above the data is a property of +what an operation allocates. Both are directly comparable across machines, +which makes them the strongest gates here. + +*Timing* numbers are not, so they're never recorded as absolute seconds. +Each timing case measures the same work through scipy in the same process +and reports the **ratio**, which cancels most of the difference between a +fast laptop and a noisy shared CI runner. + +Every case runs in its own subprocess (see ``run.py``): ``ru_maxrss`` is a +high-water mark for the life of a process, so cases measured together would +contaminate each other. +""" + +from __future__ import annotations + +import resource +import time +import tracemalloc +from collections.abc import Callable +from typing import Any + +import numpy as np +import scipy.sparse as sp + + +def peak_rss_mb() -> float: + """Process peak resident set size, in MB. Monotonic for the process's life.""" + return resource.getrusage(resource.RUSAGE_SELF).ru_maxrss / 1024 + + +def peak_alloc_mb(fn: Callable[[], Any]) -> float: + """Peak memory *allocated during* ``fn``, in MB. + + Not RSS: ``ru_maxrss`` is a high-water mark for the whole process, so an + operation that stays under the peak set while building its input reports + zero no matter how much it allocates. ``tracemalloc`` measures the + allocations themselves and can be reset, which is what makes this + sensitive enough to gate on. + + numpy allocations are traced; numba's internal (NRT) ones are not. That + suits the thing being guarded here -- the failure mode is a numpy-level + temporary the size of the data, not a kernel's own scratch. + """ + fn() # JIT compile / warm caches outside the measurement + tracemalloc.start() + try: + before = tracemalloc.get_traced_memory()[0] + tracemalloc.reset_peak() + fn() + peak = tracemalloc.get_traced_memory()[1] + finally: + tracemalloc.stop() + return max(0.0, (peak - before) / 1e6) + + +def best_time(fn: Callable[[], Any], repeat: int = 7) -> float: + """Best wall-clock time over ``repeat`` runs, in seconds. Warms up first.""" + fn() # JIT compile / allocate caches outside the measurement + best = float("inf") + for _ in range(repeat): + start = time.perf_counter() + fn() + best = min(best, time.perf_counter() - start) + return best + + +def ratio_vs_scipy(ours: Callable[[], Any], theirs: Callable[[], Any], repeat: int = 7) -> float: + """``our time / scipy's time`` for the same work. Below 1.0 means we're faster. + + Still the noisiest thing measured here even as a ratio, which is why the + timing gates carry a much looser margin than the memory ones. + """ + return best_time(ours, repeat) / best_time(theirs, repeat) + + +def integer_counts_csr(n_rows: int, n_cols: int, density: float, seed: int = 0) -> sp.csr_array: + """Integer-valued sparse counts -- the input this package exists for. + + Deliberately integer and heavily repeated: value deduplication is the + whole premise of the layout, so benchmarking it on unique floats would + measure a case that never occurs in practice. + """ + rng = np.random.default_rng(seed) + mat = sp.random_array((n_rows, n_cols), density=density, format="csr", random_state=seed) + mat.data = np.round(rng.integers(1, 8, size=mat.data.shape[0])).astype(np.float64) + return mat diff --git a/benchmarks/run.py b/benchmarks/run.py new file mode 100644 index 0000000..5bd7814 --- /dev/null +++ b/benchmarks/run.py @@ -0,0 +1,111 @@ +"""Run benchmark cases and compare them against the checked-in baselines. + + python -m benchmarks.run --set fast # run and compare (CI does this) + python -m benchmarks.run --set fast --record # rewrite baselines.json + python -m benchmarks.run --case matvec_vs_scipy # one case + +Exits nonzero if any metric regresses past its recorded ceiling, so it can +be wired straight into CI as a gate. + +Each case runs in its own subprocess. ``ru_maxrss`` only ever goes up within +a process, so cases sharing one would report each other's peaks. +""" + +from __future__ import annotations + +import argparse +import json +import subprocess +import sys +from pathlib import Path + +BASELINES = Path(__file__).with_name("baselines.json") + + +def _run_one_in_subprocess(name: str) -> dict[str, float]: + proc = subprocess.run( + [sys.executable, "-m", "benchmarks.run", "--emit", name], + capture_output=True, + text=True, + cwd=Path(__file__).resolve().parent.parent, + check=False, + ) + if proc.returncode != 0: + raise RuntimeError(f"case {name!r} failed:\n{proc.stdout}\n{proc.stderr}") + return json.loads(proc.stdout.strip().splitlines()[-1]) + + +def _compare(results: dict[str, dict[str, float]], baselines: dict) -> list[str]: + """Metrics that exceeded their ceiling, as human-readable failure lines.""" + failures = [] + for case, metrics in results.items(): + limits = baselines.get("cases", {}).get(case, {}) + for metric, value in metrics.items(): + ceiling = limits.get(metric) + if ceiling is None: + continue # recorded for context, not gated + if value > ceiling: + failures.append( + f"{case}.{metric}: {value:.4g} exceeds ceiling {ceiling:.4g}" + ) + return failures + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--set", choices=["fast", "slow", "all"], default="fast") + parser.add_argument("--case", help="run a single case by name") + parser.add_argument("--record", action="store_true", help="rewrite baselines.json") + parser.add_argument("--emit", help=argparse.SUPPRESS) # internal: run one, print JSON + args = parser.parse_args() + + from benchmarks import cases as case_module + + if args.emit: + print(json.dumps(case_module.ALL[args.emit]())) + return 0 + + if args.case: + names = [args.case] + else: + chosen = {"fast": case_module.FAST, "slow": case_module.SLOW, "all": case_module.ALL} + names = list(chosen[args.set]) + + results = {} + for name in names: + results[name] = _run_one_in_subprocess(name) + rendered = " ".join(f"{k}={v:.4g}" for k, v in results[name].items()) + print(f"{name:32s} {rendered}") + + if args.record: + existing = json.loads(BASELINES.read_text()) if BASELINES.exists() else {"cases": {}} + margins = existing.get("margins", {}) + for case, metrics in results.items(): + ceilings = {} + for metric, value in metrics.items(): + margin = margins.get(metric) + if margin is None: + continue # not a gated metric: recorded for context only + ceilings[metric] = round(value * margin, 4) + existing.setdefault("cases", {})[case] = ceilings + BASELINES.write_text(json.dumps(existing, indent=2) + "\n") + print(f"\nrecorded ceilings to {BASELINES.name}") + return 0 + + if not BASELINES.exists(): + print("\nno baselines.json; run with --record first", file=sys.stderr) + return 1 + + failures = _compare(results, json.loads(BASELINES.read_text())) + if failures: + print("\nregressions:", file=sys.stderr) + for line in failures: + print(f" {line}", file=sys.stderr) + return 1 + + print("\nno regressions past recorded ceilings") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) From d0068ed4a3f43adb0b883e0e8ca42c19afb2cfbc Mon Sep 17 00:00:00 2001 From: fishidaho Date: Thu, 3 Sep 2026 11:21:07 -0700 Subject: [PATCH 2/4] Cover the minor-axis operations #22/#23 added, and re-record on merged main Three new fast cases: minor-axis max/min, per-minor-index getnnz, and minor-axis selection. All three are new surface from #22/#23, and all three shipped carrying an nnz-sized temporary -- which is the best argument this gate could have for existing. The pattern this suite was built to catch reappeared in new code within the same week, unnoticed, because nothing measured it: minor_extrema_peak_mb 66.27 MB (max/min: np.repeat + ufunc.at) minor_getnnz_peak_mb 32.02 MB (np.bincount promotes int32 -> intp) minor_selection_peak_mb 62.27 MB (_select_minor, ISSUE-30) for results of length n_minor, on a 4M-nonzero array. Baselines re-recorded against merged main so the gate reflects the code it now guards. The ceilings for the first two drop by ~80x once the reduction kernels land; the third stays until ISSUE-30 is fixed. As before these gate against getting worse, not against the current numbers being good -- README.md already says so and now names all five memory cases. Co-Authored-By: Claude Sonnet 5 --- benchmarks/baselines.json | 13 ++++++++-- benchmarks/cases.py | 53 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/benchmarks/baselines.json b/benchmarks/baselines.json index d1ef6b4..df95c9a 100644 --- a/benchmarks/baselines.json +++ b/benchmarks/baselines.json @@ -20,10 +20,19 @@ "peak_alloc_mb": 407.2383 }, "matvec_vs_scipy": { - "time_ratio_vs_scipy": 1.8283 + "time_ratio_vs_scipy": 0.9768 }, "matmat_vs_scipy": { - "time_ratio_vs_scipy": 1.4318 + "time_ratio_vs_scipy": 0.6333 + }, + "minor_extrema_peak_mb": { + "peak_alloc_mb": 132.5453 + }, + "minor_getnnz_peak_mb": { + "peak_alloc_mb": 64.0324 + }, + "minor_selection_peak_mb": { + "peak_alloc_mb": 124.5457 } } } diff --git a/benchmarks/cases.py b/benchmarks/cases.py index 25e4e53..ff5edc6 100644 --- a/benchmarks/cases.py +++ b/benchmarks/cases.py @@ -100,6 +100,59 @@ def misaligned_matmul_peak_mb() -> dict[str, float]: } +@fast +def minor_extrema_peak_mb() -> dict[str, float]: + """Memory allocated by a minor-axis max/min. + + Added because #22's `max`/`min` shipped with the same expand-then-scatter + temporary the reduction case above exists to catch, which is the clearest + evidence available that this gate is worth having: the pattern reappears + in new code unless something measures it. + """ + from vsparse import VCSRArray + + v = VCSRArray.from_scipy(integer_counts_csr(40_000, 2_000, density=0.05)) + return { + "peak_alloc_mb": peak_alloc_mb(lambda: v.max(axis=0)), + "expanded_nnz_mb": v.nnz * 8 / 1e6, + } + + +@fast +def minor_getnnz_peak_mb() -> dict[str, float]: + """Memory allocated by a per-minor-index stored-element count. + + np.bincount is the obvious implementation and promotes an int32 + ``indices`` to intp first, so this is nnz-sized for an n_minor-sized + answer -- invisible in a correctness test. + """ + from vsparse import VCSRArray + + v = VCSRArray.from_scipy(integer_counts_csr(40_000, 2_000, density=0.05)) + return { + "peak_alloc_mb": peak_alloc_mb(lambda: v.getnnz(axis=0)), + "indices_nnz_mb": v.nnz * 8 / 1e6, + } + + +@fast +def minor_selection_peak_mb() -> dict[str, float]: + """Memory allocated by a minor-axis selection (#23's `_select_minor`). + + Currently carries an nnz-sized temporary of its own (ISSUE-30); recorded + so the number is tracked rather than assumed, and so the ceiling drops + visibly when that is fixed. + """ + from vsparse import VCSRArray + + v = VCSRArray.from_scipy(integer_counts_csr(40_000, 2_000, density=0.05)) + cols = np.arange(0, v.shape[1], 2) + return { + "peak_alloc_mb": peak_alloc_mb(lambda: v[:, cols]), + "indices_nnz_mb": v.nnz * 8 / 1e6, + } + + # -- throughput, relative to scipy ------------------------------------------- From 0d76a4da721ae4e637c15aa986e6bb5941075c32 Mon Sep 17 00:00:00 2001 From: fishidaho Date: Thu, 3 Sep 2026 11:22:43 -0700 Subject: [PATCH 3/4] Document the measured before/after for every gated memory case Names all five memory cases with the number on main, the number with the corresponding fix, and the reason each is high -- so the generous ceilings are self-explaining rather than looking like sloppy thresholds, and so re-recording after each fix is a mechanical check rather than a judgement. Co-Authored-By: Claude Sonnet 5 --- benchmarks/README.md | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/benchmarks/README.md b/benchmarks/README.md index 70aa924..4d91b5e 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -50,16 +50,24 @@ context (e.g. `expanded_nnz_mb`, which says what a per-nonzero temporary *would* have cost, so the gated number next to it can be read in proportion). The checked-in ceilings were recorded on `main` before the v0.2 memory fixes -landed, so two of them are deliberately generous: - -| metric | recorded on `main` | -|---|---| -| `minor_sum_peak_mb.peak_alloc_mb` | 66 MB, for a reduction on a 4M-nonzero array | -| `misaligned_matmul_peak_mb.peak_alloc_mb` | 204 MB, against a 16 MB array | - -Both should drop by one to two orders of magnitude once the reduction and -misaligned-matmul fixes are in. **Re-record after those merge** — until then -these gate against getting worse, not against the current numbers being good. +landed, so the memory ones are deliberately generous. Measured on a +4M-nonzero array, for results of length `n_minor`: + +| metric | on `main` | with the fix | why it's high | +|---|---|---|---| +| `minor_sum_peak_mb` | 66 MB | 0.8 MB | `np.repeat` then `np.bincount` | +| `minor_extrema_peak_mb` | 66 MB | 1.6 MB | `np.repeat` then `ufunc.at` | +| `minor_getnnz_peak_mb` | 32 MB | 0.8 MB | `np.bincount` promotes int32 → intp | +| `minor_selection_peak_mb` | 62 MB | — | `_select_minor`, still open (ISSUE-30) | +| `misaligned_matmul_peak_mb` | 204 MB | 115 MB | full opposite-format copy | + +**Re-record once those land** — until then these gate against getting worse, +not against the current numbers being good. + +Three of those five are operations added *after* this suite was designed, +which is the case for having it: the expand-then-scatter pattern reappeared +in new code because nothing measured it. A correctness test can't see it — +every one of those operations returns the right answer. ## Adding a case From 4d0597308e3f22b79c8838b710c49c89fd02ed4e Mon Sep 17 00:00:00 2001 From: fishidaho Date: Thu, 3 Sep 2026 17:11:43 -0700 Subject: [PATCH 4/4] Trim comments, docstrings and the README Drops the dead peak_rss_mb helper and cuts the prose to what the suite does and how to run it. --- benchmarks/README.md | 100 +++++++++++++++--------------------------- benchmarks/cases.py | 55 +++-------------------- benchmarks/harness.py | 54 +++-------------------- benchmarks/run.py | 9 ++-- 4 files changed, 52 insertions(+), 166 deletions(-) diff --git a/benchmarks/README.md b/benchmarks/README.md index 4d91b5e..66669e1 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -1,85 +1,57 @@ # Benchmarks -A small suite that CI runs as a regression gate, plus a larger set for -running by hand or on a schedule. +A small suite CI runs as a regression gate, plus larger cases for running by +hand. ```sh -uv run python -m benchmarks.run --set fast # run + compare to baselines (what CI does) -uv run python -m benchmarks.run --set slow # the larger cases +uv run python -m benchmarks.run --set fast # run + compare (what CI does) +uv run python -m benchmarks.run --set slow # the larger cases uv run python -m benchmarks.run --case matvec_vs_scipy uv run python -m benchmarks.run --set fast --record # rewrite baselines.json ``` -Exits nonzero if any gated metric exceeds its recorded ceiling. +Exits nonzero if a gated metric exceeds its ceiling. -## What it measures, and why those things +## Metrics -Correctness tests don't catch cost regressions: the answers stay right while -the memory or the file doubles. These are the two properties that can move -silently. +Correctness tests do not catch cost regressions, so the suite measures two +things that move silently: -**Layout size** — bytes per stored nonzero, and the same figure for the -`indices` array alone. Deterministic, so it's an exact gate. It moves if an -index dtype widens, if value deduplication stops working, or if a new array -joins the layout. +**Layout size and memory allocated by an operation.** Both deterministic and +comparable across machines, so they are gated tightly. Memory uses +`tracemalloc` rather than `ru_maxrss`, which is a process-lifetime high-water +mark and reports zero for an operation staying under the peak set while +building its input. -**Memory allocated by an operation** — measured with `tracemalloc`, not -`ru_maxrss`. RSS is a process-lifetime high-water mark, so an operation that -stays under the peak set while building its input reports zero no matter how -much it allocates; that made the first version of this suite report `0` for -an operation allocating 66 MB. `tracemalloc` measures allocations and can be -reset between runs. It traces numpy but not numba's internal allocations, -which suits what's being guarded here: the failure mode is a numpy-level -temporary the size of the data, not a kernel's own scratch. +**Throughput relative to scipy**, never absolute seconds. The same work is +timed through scipy in the same process and the ratio recorded, which cancels +most of the difference between machines. Still the noisiest metric, so its +gate is much looser. -**Throughput relative to scipy** — never absolute seconds. The same work is -timed through scipy in the same process and the *ratio* is recorded, which -cancels most of the difference between a fast laptop and a shared CI runner. -It's still the noisiest thing here, which is why the timing gates carry a -much looser margin (4×) than the memory ones (2×) and the layout ones (1.1×). - -Every case runs in its own subprocess, because measurement state and JIT -warm-up would otherwise leak between them. +Each case runs in its own subprocess, since measurement state and JIT warm-up +leak between them otherwise. ## Baselines -`baselines.json` holds a ceiling per gated metric. `--record` regenerates -them by multiplying a fresh measurement by that metric's margin. Only metrics -named in `margins` are gated; anything else a case returns is recorded for -context (e.g. `expanded_nnz_mb`, which says what a per-nonzero temporary -*would* have cost, so the gated number next to it can be read in proportion). - -The checked-in ceilings were recorded on `main` before the v0.2 memory fixes -landed, so the memory ones are deliberately generous. Measured on a -4M-nonzero array, for results of length `n_minor`: - -| metric | on `main` | with the fix | why it's high | -|---|---|---|---| -| `minor_sum_peak_mb` | 66 MB | 0.8 MB | `np.repeat` then `np.bincount` | -| `minor_extrema_peak_mb` | 66 MB | 1.6 MB | `np.repeat` then `ufunc.at` | -| `minor_getnnz_peak_mb` | 32 MB | 0.8 MB | `np.bincount` promotes int32 → intp | -| `minor_selection_peak_mb` | 62 MB | — | `_select_minor`, still open (ISSUE-30) | -| `misaligned_matmul_peak_mb` | 204 MB | 115 MB | full opposite-format copy | +`baselines.json` holds a ceiling per gated metric, regenerated with +`--record` by multiplying a fresh measurement by that metric's margin. Only +metrics named in `margins` are gated; anything else a case returns is +recorded for context. -**Re-record once those land** — until then these gate against getting worse, -not against the current numbers being good. +The checked-in ceilings were recorded before the memory fixes landed, so the +memory ones are deliberately generous and should be re-recorded as those +merge. On a 4M-nonzero array, for results of length `n_minor`: -Three of those five are operations added *after* this suite was designed, -which is the case for having it: the expand-then-scatter pattern reappeared -in new code because nothing measured it. A correctness test can't see it — -every one of those operations returns the right answer. +| metric | recorded | with the fix | +|---|---|---| +| `minor_sum_peak_mb` | 66 MB | 0.8 MB | +| `minor_extrema_peak_mb` | 66 MB | 1.6 MB | +| `minor_getnnz_peak_mb` | 32 MB | 0.8 MB | +| `minor_selection_peak_mb` | 62 MB | 4.8 MB | +| `misaligned_matmul_peak_mb` | 204 MB | 115 MB | ## Adding a case -Write a function returning `{metric: value}` in `cases.py` and decorate it -with `@fast` (runs on every PR — keep it well under a minute) or `@slow`. -Add any new gated metric name to `margins` in `baselines.json`, then -`--record`. - -## The larger suite - -`--set slow` isn't wired into the PR gate: those cases build datasets an -order of magnitude bigger and take minutes, which is the wrong thing to put -in front of every push. They're meant for a scheduled workflow or a manual -run before a release. Wiring up that scheduled job is deliberately left as a -follow-up rather than guessed at here. +Write a function returning `{metric: value}` in `cases.py`, decorated with +`@fast` (runs on every PR, keep it under a minute) or `@slow`. Add any new +gated metric to `margins` in `baselines.json`, then `--record`. diff --git a/benchmarks/cases.py b/benchmarks/cases.py index ff5edc6..eb3be8b 100644 --- a/benchmarks/cases.py +++ b/benchmarks/cases.py @@ -1,15 +1,3 @@ -"""The benchmark cases themselves. - -Each case is a function returning ``{metric_name: value}``. Cases marked -``fast`` run on every pull request; the rest are for the scheduled job and -for running by hand on real data. - -What's here is chosen to cover the claims that are easy to regress silently: -the size of the stored layout, and whether an operation allocates a second -copy of the data. A correctness test won't catch either -- the answers stay -right while the cost quietly doubles. -""" - from __future__ import annotations from collections.abc import Callable @@ -41,12 +29,7 @@ def slow(fn): @fast def layout_bytes_per_nonzero() -> dict[str, float]: - """Total bytes the VCS layout holds, per stored nonzero. - - Deterministic, so this is an exact gate rather than a noisy one. It - moves if index dtypes widen, if deduplication stops working, or if a - new array joins the layout. - """ + """Bytes the layout holds per stored nonzero, and the same for ``indices`` alone.""" from vsparse import VCSRArray mat = integer_counts_csr(20_000, 2_000, density=0.05) @@ -65,11 +48,7 @@ def layout_bytes_per_nonzero() -> dict[str, float]: @fast def minor_sum_peak_mb() -> dict[str, float]: - """Memory allocated by a minor-axis reduction. - - A reduction produces an ``n_minor``-sized result; anything approaching - the size of the data means an nnz-sized temporary crept back in. - """ + """Memory allocated by a minor-axis sum.""" from vsparse import VCSRArray v = VCSRArray.from_scipy(integer_counts_csr(40_000, 2_000, density=0.05)) @@ -82,11 +61,7 @@ def minor_sum_peak_mb() -> dict[str, float]: @fast def misaligned_matmul_peak_mb() -> dict[str, float]: - """Memory allocated by the matmul direction the storage isn't aligned for. - - The failure mode is a full opposite-format copy of the array, so this - should stay far below the array's own footprint. - """ + """Memory allocated by the matmul direction the storage isn't aligned for.""" from vsparse import VCSCArray v = VCSCArray.from_scipy(integer_counts_csr(40_000, 2_000, density=0.05)) @@ -102,13 +77,7 @@ def misaligned_matmul_peak_mb() -> dict[str, float]: @fast def minor_extrema_peak_mb() -> dict[str, float]: - """Memory allocated by a minor-axis max/min. - - Added because #22's `max`/`min` shipped with the same expand-then-scatter - temporary the reduction case above exists to catch, which is the clearest - evidence available that this gate is worth having: the pattern reappears - in new code unless something measures it. - """ + """Memory allocated by a minor-axis max/min.""" from vsparse import VCSRArray v = VCSRArray.from_scipy(integer_counts_csr(40_000, 2_000, density=0.05)) @@ -120,12 +89,7 @@ def minor_extrema_peak_mb() -> dict[str, float]: @fast def minor_getnnz_peak_mb() -> dict[str, float]: - """Memory allocated by a per-minor-index stored-element count. - - np.bincount is the obvious implementation and promotes an int32 - ``indices`` to intp first, so this is nnz-sized for an n_minor-sized - answer -- invisible in a correctness test. - """ + """Memory allocated by a per-minor-index stored-element count.""" from vsparse import VCSRArray v = VCSRArray.from_scipy(integer_counts_csr(40_000, 2_000, density=0.05)) @@ -137,12 +101,7 @@ def minor_getnnz_peak_mb() -> dict[str, float]: @fast def minor_selection_peak_mb() -> dict[str, float]: - """Memory allocated by a minor-axis selection (#23's `_select_minor`). - - Currently carries an nnz-sized temporary of its own (ISSUE-30); recorded - so the number is tracked rather than assumed, and so the ceiling drops - visibly when that is fixed. - """ + """Memory allocated by a minor-axis selection.""" from vsparse import VCSRArray v = VCSRArray.from_scipy(integer_counts_csr(40_000, 2_000, density=0.05)) @@ -185,7 +144,7 @@ def matmat_vs_scipy() -> dict[str, float]: @slow def large_layout_and_matmul() -> dict[str, float]: - """The same shape of measurement an order of magnitude up.""" + """The same measurements an order of magnitude up.""" from vsparse import VCSRArray mat = integer_counts_csr(200_000, 3_000, density=0.02) diff --git a/benchmarks/harness.py b/benchmarks/harness.py index 000930e..7a9627d 100644 --- a/benchmarks/harness.py +++ b/benchmarks/harness.py @@ -1,27 +1,5 @@ -"""Measurement primitives for the benchmark suite. - -Two kinds of metric, chosen so that a CI job can compare them against a -checked-in threshold without the result depending on which runner it landed -on: - -*Memory and layout* numbers are deterministic -- bytes per nonzero is a -property of the data structure, and peak RSS above the data is a property of -what an operation allocates. Both are directly comparable across machines, -which makes them the strongest gates here. - -*Timing* numbers are not, so they're never recorded as absolute seconds. -Each timing case measures the same work through scipy in the same process -and reports the **ratio**, which cancels most of the difference between a -fast laptop and a noisy shared CI runner. - -Every case runs in its own subprocess (see ``run.py``): ``ru_maxrss`` is a -high-water mark for the life of a process, so cases measured together would -contaminate each other. -""" - from __future__ import annotations -import resource import time import tracemalloc from collections.abc import Callable @@ -31,23 +9,12 @@ import scipy.sparse as sp -def peak_rss_mb() -> float: - """Process peak resident set size, in MB. Monotonic for the process's life.""" - return resource.getrusage(resource.RUSAGE_SELF).ru_maxrss / 1024 - - def peak_alloc_mb(fn: Callable[[], Any]) -> float: - """Peak memory *allocated during* ``fn``, in MB. - - Not RSS: ``ru_maxrss`` is a high-water mark for the whole process, so an - operation that stays under the peak set while building its input reports - zero no matter how much it allocates. ``tracemalloc`` measures the - allocations themselves and can be reset, which is what makes this - sensitive enough to gate on. + """Peak memory allocated during ``fn``, in MB. - numpy allocations are traced; numba's internal (NRT) ones are not. That - suits the thing being guarded here -- the failure mode is a numpy-level - temporary the size of the data, not a kernel's own scratch. + Not RSS, which is a process-lifetime high-water mark and so reports zero + for anything staying under the peak set while building its input. numpy + allocations are traced, numba's internal ones are not. """ fn() # JIT compile / warm caches outside the measurement tracemalloc.start() @@ -73,21 +40,12 @@ def best_time(fn: Callable[[], Any], repeat: int = 7) -> float: def ratio_vs_scipy(ours: Callable[[], Any], theirs: Callable[[], Any], repeat: int = 7) -> float: - """``our time / scipy's time`` for the same work. Below 1.0 means we're faster. - - Still the noisiest thing measured here even as a ratio, which is why the - timing gates carry a much looser margin than the memory ones. - """ + """``our time / scipy's time`` for the same work.""" return best_time(ours, repeat) / best_time(theirs, repeat) def integer_counts_csr(n_rows: int, n_cols: int, density: float, seed: int = 0) -> sp.csr_array: - """Integer-valued sparse counts -- the input this package exists for. - - Deliberately integer and heavily repeated: value deduplication is the - whole premise of the layout, so benchmarking it on unique floats would - measure a case that never occurs in practice. - """ + """Integer-valued sparse counts, repeated enough for the layout to dedupe.""" rng = np.random.default_rng(seed) mat = sp.random_array((n_rows, n_cols), density=density, format="csr", random_state=seed) mat.data = np.round(rng.integers(1, 8, size=mat.data.shape[0])).astype(np.float64) diff --git a/benchmarks/run.py b/benchmarks/run.py index 5bd7814..e1143e6 100644 --- a/benchmarks/run.py +++ b/benchmarks/run.py @@ -4,11 +4,8 @@ python -m benchmarks.run --set fast --record # rewrite baselines.json python -m benchmarks.run --case matvec_vs_scipy # one case -Exits nonzero if any metric regresses past its recorded ceiling, so it can -be wired straight into CI as a gate. - -Each case runs in its own subprocess. ``ru_maxrss`` only ever goes up within -a process, so cases sharing one would report each other's peaks. +Exits nonzero if any metric regresses past its recorded ceiling. Each case +runs in its own subprocess. """ from __future__ import annotations @@ -36,7 +33,7 @@ def _run_one_in_subprocess(name: str) -> dict[str, float]: def _compare(results: dict[str, dict[str, float]], baselines: dict) -> list[str]: - """Metrics that exceeded their ceiling, as human-readable failure lines.""" + """Metrics that exceeded their ceiling.""" failures = [] for case, metrics in results.items(): limits = baselines.get("cases", {}).get(case, {})