diff --git a/doc/benchmarks.md b/doc/benchmarks.md index 447c0887d..ca92772f6 100644 --- a/doc/benchmarks.md +++ b/doc/benchmarks.md @@ -736,6 +736,8 @@ host doesn't record, such as a performance tier or bursting setting, reset at le affected metrics by removing them from the series' history files, such as `data/windows-whp-virtual-machine-microvm-v2-1vcpu.csv`, so that the gate restarts their ten-point warmup instead of comparing the new storage with the old baseline. +Results measured before such a reset commit are not persisted into the reset +series, so a `dev` run that finishes after the reset cannot refill it. The regression gate compares the target p50 with the median of the latest 10 p50 values on the pull request's base branch and requires all 10 diff --git a/scripts/nvx_tools/performance.py b/scripts/nvx_tools/performance.py index 23785bbea..f4386812e 100644 --- a/scripts/nvx_tools/performance.py +++ b/scripts/nvx_tools/performance.py @@ -9,9 +9,10 @@ import os import re import statistics +import subprocess import sys import tempfile -from collections import defaultdict, deque +from collections import Counter, defaultdict, deque from collections.abc import Callable, Iterable, Sequence from dataclasses import dataclass, replace from pathlib import Path @@ -2248,11 +2249,161 @@ def _verify_result_hosts( ) +def _git(checkout: Path, *args: str) -> str: + completed = subprocess.run( + ["git", "-C", str(checkout), *args], + check=False, + capture_output=True, + text=True, + ) + if completed.returncode != 0: + raise PerformanceError( + f"git {' '.join(args)} failed: {completed.stderr.strip()}" + ) + return completed.stdout + + +def _committed_results(top: Path, revision: str, relative: str) -> list[Result]: + shown = subprocess.run( + ["git", "-C", str(top), "show", f"{revision}:{relative}"], + check=False, + capture_output=True, + ) + if shown.returncode != 0: + return [] + with tempfile.TemporaryDirectory() as temporary: + # Keep the basename: a legacy CSV takes its platform from the file name. + path = Path(temporary) / Path(relative).name + path.write_bytes(shown.stdout) + return read_results(path) + + +def _dimension_counts( + top: Path, + revision: str, + relative: str, +) -> Counter[tuple[str, int, int, str]]: + return Counter( + _dimension_key(result, "") + for result in _committed_results(top, revision, relative) + ) + + +def _reset_dimensions_since( + git_checkout: Path, + history_path: Path, + commit: str, +) -> set[tuple[str, int, int, str]]: + """Return the dimensions whose committed history lost rows after ``commit``. + + A result measured at ``commit`` predates any later commit that removes the + dimension's rows, so it must not be appended onto the reset series. + """ + top = Path(_git(git_checkout, "rev-parse", "--show-toplevel").strip()).resolve() + try: + relative = history_path.resolve().relative_to(top).as_posix() + except ValueError as error: + raise PerformanceError(f"{history_path} is outside the git checkout") from error + ancestor = subprocess.run( + ["git", "-C", str(top), "merge-base", "--is-ancestor", commit, "HEAD"], + check=False, + capture_output=True, + ) + if ancestor.returncode != 0: + raise PerformanceError( + f"result commit {commit} is not an ancestor of the target history" + ) + # Compare each commit with its own parents: a side branch forked before + # ``commit`` can hold fewer rows than ``commit`` without having reset them. + later = _git( + top, + "log", + "--reverse", + "--full-history", + "--sparse", + "--format=%H %P", + f"{commit}..HEAD", + "--", + relative, + ).splitlines() + resets: set[tuple[str, int, int, str]] = set() + for line in later: + revision, *parents = line.split() + # Ignore commits on side branches that are not part of the history + # leading from the measurement to HEAD, even if later merged/reverted. + if not _on_measurement_line(top, revision, commit): + continue + current = _dimension_counts(top, revision, relative) + for parent in parents: + # A branch that neither predates nor follows the measurement can drop rows + # that were already reset before it, so only the measurement's line counts. + if not _on_measurement_line(top, parent, commit): + continue + before = _dimension_counts(top, parent, relative) + resets.update(key for key, count in before.items() if current[key] < count) + return resets + + +def _on_measurement_line(top: Path, revision: str, measured: str) -> bool: + """Return whether ``revision`` is on the path from ``measured`` to HEAD.""" + return all( + subprocess.run( + [ + "git", + "-C", + str(top), + "merge-base", + "--is-ancestor", + ancestor, + descendant, + ], + check=False, + capture_output=True, + ).returncode + == 0 + for ancestor, descendant in ((measured, revision), (revision, "HEAD")) + ) + + +def _without_reset_rows( + git_checkout: Path, + history_path: Path, + results: Sequence[Result], +) -> list[Result]: + resets: dict[str, set[tuple[str, int, int, str]]] = {} + kept: list[Result] = [] + for result in results: + if result.commit not in resets: + resets[result.commit] = _reset_dimensions_since( + git_checkout, history_path, result.commit + ) + if _dimension_key(result, "") in resets[result.commit]: + print( + f"Skipped {result.metric} measured at {result.commit[:12]}: " + f"{history_path.name} was reset after that commit" + ) + continue + kept.append(result) + return kept + + +def _git_checkout_of(path: Path) -> Path | None: + """Return ``path`` when it lies inside a git checkout, which CI always does.""" + probe = subprocess.run( + ["git", "-C", str(path), "rev-parse", "--is-inside-work-tree"], + check=False, + capture_output=True, + text=True, + ) + return path if probe.returncode == 0 and probe.stdout.strip() == "true" else None + + def persist_results( source_dir: Path, history_dir: Path, excluded_metrics: Iterable[str] = (), require_host: bool = False, + git_checkout: Path | None = None, ) -> None: source_files = sorted(source_dir.glob("*.csv")) if not source_files: @@ -2275,6 +2426,14 @@ def persist_results( current = [result for result in source_results if result.metric not in excluded] history_path = history_dir / source_path.name existing = read_results(history_path) if history_path.exists() else [] + existing_keys = {_result_identity(result) for result in existing} + current = [ + result + for result in current + if _result_identity(result) not in existing_keys + ] + if git_checkout is not None: + current = _without_reset_rows(git_checkout, history_path, current) metadata = { ( result.platform, @@ -2300,12 +2459,7 @@ def persist_results( f"{expected} -> {actual}" ) - existing_keys = {_result_identity(result) for result in existing} - new_results = [ - result - for result in current - if _result_identity(result) not in existing_keys - ] + new_results = current if not new_results: print(f"No new performance rows to persist for {source_path.name}") continue @@ -2916,6 +3070,7 @@ def command_performance(args: argparse.Namespace) -> int: args.history_dir, args.exclude_metric, require_host=args.require_host, + git_checkout=_git_checkout_of(args.history_dir), ) return 0 except (PerformanceError, OSError, csv.Error) as error: diff --git a/scripts/test_performance.py b/scripts/test_performance.py index d1dd16bcd..d75c0ad3b 100644 --- a/scripts/test_performance.py +++ b/scripts/test_performance.py @@ -7,6 +7,7 @@ import json import re import statistics +import subprocess import sys import tempfile import unittest @@ -2389,6 +2390,305 @@ def test_rejects_lifecycle_workload_processor_mismatch(self): lifecycle_input=lifecycle_path, ) + def test_reset_check_ignores_side_branch_history_that_never_reset_a_row(self): + # A side branch forked before the measurement can hold fewer rows than the + # measurement without resetting them. Comparing it with the previous commit + # in log order would flag the throughput row, which it never removed. + with tempfile.TemporaryDirectory() as temporary: + repo = Path(temporary) + + def git(*args: str) -> str: + completed = subprocess.run( + ["git", "-C", str(repo), *args], + check=True, + capture_output=True, + text=True, + ) + return completed.stdout.strip() + + git("init", "-q") + git("config", "user.email", "test@example.invalid") + git("config", "user.name", "test") + history_file = repo / "data" / "linux-kvm.csv" + performance.write_results( + history_file, + [performance.Result("s0", "latency", "ms", "lower", 10.0)], + ) + git("add", "data") + git("commit", "-q", "-m", "seed") + main = git("rev-parse", "--abbrev-ref", "HEAD") + git("checkout", "-q", "-b", "side") + performance.write_results( + history_file, + [performance.Result("x", "latency", "ms", "lower", 11.0)], + ) + git("commit", "-q", "-am", "side change") + git("checkout", "-q", main) + performance.write_results( + history_file, + [ + performance.Result("s0", "latency", "ms", "lower", 10.0), + performance.Result("A", "throughput", "MB/s", "higher", 20.0), + ], + ) + git("commit", "-q", "-am", "measured") + measured = git("rev-parse", "HEAD") + # The merge keeps the side branch's latency change and A's throughput row. + git("merge", "-q", "--no-commit", "--no-ff", "-X", "ours", "side") + performance.write_results( + history_file, + [ + performance.Result("x", "latency", "ms", "lower", 11.0), + performance.Result("A", "throughput", "MB/s", "higher", 20.0), + ], + ) + git("commit", "-q", "-am", "merge side") + + self.assertEqual( + performance._reset_dimensions_since(repo, history_file, measured), + set(), + ) + + def test_reset_check_ignores_a_merged_branch_that_predates_an_earlier_reset(self): + # Dev resets latency before measurement A. A branch forked before that reset + # keeps the old latency rows and adds throughput; merging it into dev keeps the + # reset, so the merge drops latency relative to that branch. The reset preceded A, + # so A's latency row must not be discarded. + with tempfile.TemporaryDirectory() as temporary: + repo = Path(temporary) + + def git(*args: str) -> str: + completed = subprocess.run( + ["git", "-C", str(repo), *args], + check=True, + capture_output=True, + text=True, + ) + return completed.stdout.strip() + + git("init", "-q") + git("config", "user.email", "test@example.invalid") + git("config", "user.name", "test") + history_file = repo / "data" / "linux-kvm.csv" + performance.write_results( + history_file, + [performance.Result("s0", "latency", "ms", "lower", 10.0)], + ) + git("add", "data") + git("commit", "-q", "-m", "seed") + main = git("rev-parse", "--abbrev-ref", "HEAD") + git("checkout", "-q", "-b", "branch") + performance.write_results( + history_file, + [ + performance.Result("s0", "latency", "ms", "lower", 12.0), + performance.Result("b", "throughput", "MB/s", "higher", 5.0), + ], + ) + git("commit", "-q", "-am", "branch work") + git("checkout", "-q", main) + performance.write_results(history_file, []) + git("commit", "-q", "-am", "reset latency before the measurement") + git("commit", "-q", "--allow-empty", "-m", "measured") + measured = git("rev-parse", "HEAD") + git("merge", "-q", "--no-commit", "--no-ff", "-X", "ours", "branch") + performance.write_results( + history_file, + [performance.Result("b", "throughput", "MB/s", "higher", 5.0)], + ) + git("commit", "-q", "-am", "merge branch") + + self.assertEqual( + performance._reset_dimensions_since(repo, history_file, measured), + set(), + ) + + def test_persist_drops_rows_measured_before_a_later_history_reset(self): + # A row measured before a commit that reset its metric must not refill the reset + # series, or nine post-reset rows would already gate the metric. Unaffected metrics + # measured at the same commit still persist. + with tempfile.TemporaryDirectory() as temporary: + repo = Path(temporary) + + def git(*args: str) -> str: + completed = subprocess.run( + ["git", "-C", str(repo), *args], + check=True, + capture_output=True, + text=True, + ) + return completed.stdout.strip() + + def commit(message: str) -> str: + git("add", "data") + git("commit", "-q", "--allow-empty", "-m", message) + return git("rev-parse", "HEAD") + + git("init", "-q") + git("config", "user.email", "test@example.invalid") + git("config", "user.name", "test") + history = repo / "data" + source = repo / "source" + history_file = history / "linux-kvm.csv" + source_file = source / "linux-kvm.csv" + performance.write_results( + history_file, + [ + performance.Result("seed", "latency", "ms", "lower", 10.0), + performance.Result("seed", "throughput", "MB/s", "higher", 20.0), + ], + ) + measured = commit("seed") + + # Commit B resets only the latency metric; throughput keeps its history. + performance.write_results( + history_file, + [performance.Result("seed", "throughput", "MB/s", "higher", 20.0)], + ) + commit("reset latency") + + for number in range(9): + sha = commit(f"post-reset {number}") + performance.write_results( + source_file, + [performance.Result(sha, "latency", "ms", "lower", 100.0)], + ) + performance.persist_results(source, history, git_checkout=repo) + + # The measurement of A arrives after the reset. It must not refill latency. + performance.write_results( + source_file, + [ + performance.Result(measured, "latency", "ms", "lower", 10.0), + performance.Result(measured, "throughput", "MB/s", "higher", 20.0), + ], + ) + performance.persist_results(source, history, git_checkout=repo) + + rows = performance.read_results(history_file) + latency = [row for row in rows if row.metric == "latency"] + throughput = [row for row in rows if row.metric == "throughput"] + self.assertEqual(len(latency), 9) + self.assertNotIn(measured, {row.commit for row in latency}) + self.assertEqual({row.commit for row in throughput}, {"seed", measured}) + + def test_persist_detects_merged_reset_and_ignores_reverted_side_reset(self): + with tempfile.TemporaryDirectory() as temporary: + repo = Path(temporary) + + def git(*args: str) -> str: + return subprocess.run( + ["git", "-C", str(repo), *args], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + def commit(message: str) -> str: + git("add", "data") + git("commit", "-q", "--allow-empty", "-m", message) + return git("rev-parse", "HEAD") + + git("init", "-q") + git("config", "user.email", "test@example.invalid") + git("config", "user.name", "test") + history_file = repo / "data" / "linux-kvm.csv" + original = [performance.Result("seed", "latency", "ms", "lower", 10.0)] + performance.write_results(history_file, original) + commit("seed") + main = git("rev-parse", "--abbrev-ref", "HEAD") + + # A side branch forked before the measurement resets and restores + # rows; its temporary reset is outside the measured ancestry. + git("checkout", "-q", "-b", "reverted") + unrelated = repo / "data" / "unrelated.txt" + unrelated.write_text("x", encoding="utf-8") + git("add", "data/unrelated.txt") + git("commit", "-q", "-m", "unrelated") + performance.write_results(history_file, []) + commit("temporary reset") + performance.write_results(history_file, original) + commit("restore rows") + git("checkout", "-q", main) + measured = commit("measurement") + git("merge", "-q", "--no-ff", "reverted", "-m", "merge restored branch") + + source = repo / "source" + performance.write_results( + source / "linux-kvm.csv", + [performance.Result(measured, "latency", "ms", "lower", 11.0)], + ) + performance.persist_results(source, repo / "data", git_checkout=repo) + rows = performance.read_results(history_file) + self.assertIn(measured, {row.commit for row in rows}) + + # A second branch performs a multi-commit reset. The integration + # merge must expose the loss even when git's path simplification + # would otherwise omit an intermediate commit. + git("checkout", "-q", "-b", "reset-branch") + performance.write_results(history_file, []) + commit("begin reset") + commit("continue reset") + git("checkout", "-q", main) + git("merge", "-q", "--no-ff", "reset-branch", "-m", "merge reset") + performance.write_results( + source / "linux-kvm.csv", + [performance.Result(measured, "latency", "ms", "lower", 12.0)], + ) + performance.persist_results(source, repo / "data", git_checkout=repo) + self.assertEqual(performance.read_results(history_file), []) + + def test_persist_filters_stale_metadata_before_validation(self): + with tempfile.TemporaryDirectory() as temporary: + repo = Path(temporary) + + def git(*args: str) -> str: + return subprocess.run( + ["git", "-C", str(repo), *args], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + def commit(message: str) -> str: + git("add", "data") + git("commit", "-q", "-m", message) + return git("rev-parse", "HEAD") + + git("init", "-q") + git("config", "user.email", "test@example.invalid") + git("config", "user.name", "test") + history_file = repo / "data" / "linux-kvm.csv" + performance.write_results( + history_file, + [ + performance.Result("seed", "latency", "ms", "lower", 10.0), + performance.Result("seed", "throughput", "MB/s", "higher", 20.0), + ], + ) + measured = commit("measurement") + performance.write_results( + history_file, + [ + performance.Result("seed", "throughput", "MB/s", "higher", 20.0), + ], + ) + commit("reset latency") + source = repo / "source" + performance.write_results( + source / "linux-kvm.csv", + [ + performance.Result(measured, "latency", "seconds", "higher", 1.0), + performance.Result(measured, "throughput", "MB/s", "higher", 21.0), + ], + ) + performance.persist_results(source, repo / "data", git_checkout=repo) + rows = performance.read_results(history_file) + self.assertEqual( + [(row.metric, row.commit) for row in rows], + [("throughput", "seed"), ("throughput", measured)], + ) + def test_persist_and_gate_isolate_processor_counts(self): with tempfile.TemporaryDirectory() as temporary: root = Path(temporary)