diff --git a/product/skills/measure-then-optimize/SKILL.md b/product/skills/measure-then-optimize/SKILL.md deleted file mode 100644 index 31998b6d..00000000 --- a/product/skills/measure-then-optimize/SKILL.md +++ /dev/null @@ -1,14 +0,0 @@ ---- -name: measure-then-optimize -description: >- - Measure and verify work that makes something faster, cheaper, or lighter. - Use before diagnosing or changing performance, cost, resource use, latency, - throughput, startup time, or build time. ---- - -# Measure then optimize - -Follow [Measure, identify, fix, verify](playbooks/measure-identify-fix-verify.md) -before making a performance change. A performance fix is done only when the -user's symptom was measured before and after under the same real conditions by -a committed, rerunnable program, and the ledger's `judge` verdict is `pass`. diff --git a/product/skills/measure-then-optimize/playbooks/measure-identify-fix-verify.md b/product/skills/measure-then-optimize/playbooks/measure-identify-fix-verify.md deleted file mode 100644 index b224228d..00000000 --- a/product/skills/measure-then-optimize/playbooks/measure-identify-fix-verify.md +++ /dev/null @@ -1,77 +0,0 @@ -# Measure, identify, fix, verify - -Rob Pike's rule 2 in [*Notes on Programming in C* (1989)](https://users.ece.utexas.edu/~adnan/pike.html) -is the starting constraint: "Measure. Don't tune for speed until you've -measured." Brendan Gregg's [Performance Analysis Methodology](https://www.brendangregg.com/methodology.html) -provides methods for moving from the observed problem toward measured causes -instead of changing things at random. - -## 1. Measure - -Define the metric for the user's own symptom, not a convenient proxy. Exercise -the real path with the real workload, configuration, hardware, and other -settings that matter. Write a program that reruns that measurement and commit -it as the lever before changing the system; [Build the Lever](../../../../corpus/skills/principle-build-the-lever/SKILL.md) -explains why the program must remain reviewable and rerunnable. - -Record the baseline with [perf_ledger.py](../scripts/perf_ledger.py), using at -least three runs and naming every relevant setting: - -```sh -python3 product/skills/measure-then-optimize/scripts/perf_ledger.py record \ - --ledger perf-ledger.json \ - --phase baseline \ - --metric boot_seconds \ - --runs 5 \ - --setting workload=production \ - --symptom-metric boot_seconds \ - --lever-path scripts/measure_boot.py \ - -- python3 scripts/measure_boot.py -``` - -Use `--from-stdout` when the program prints the metric; otherwise the ledger -records wall-clock seconds. Preserve the baseline ledger with the change. - -## 2. Identify - -Name a cause only after a measurement attributes time or cost to it. Break the -symptom into measured components, then rank candidates by their measured share -of the total. Profiles, traces, counters, and time-division measurements can -support attribution; intuition and code proximity cannot. - -Investigate the largest measured candidate first. If the measurements cannot -distinguish candidates, improve the measurement before choosing a cause. - -## 3. Fix - -Change one measured cause at a time so the next measurement can attribute any -difference to that change. Keep the measurement program, workload, and settings -fixed. - -Raising a timeout, memory ceiling, retry budget, batch limit, or other limit is -not a performance fix unless the before and after symptom numbers justify it. -Treat a limit increase without that evidence as moving the boundary, not -removing the cost. - -## 4. Verify - -Rerun the same committed program with the same settings and record the result as -the after phase: - -```sh -python3 product/skills/measure-then-optimize/scripts/perf_ledger.py record \ - --ledger perf-ledger.json \ - --phase after \ - --metric boot_seconds \ - --runs 5 \ - --setting workload=production \ - -- python3 scripts/measure_boot.py - -python3 product/skills/measure-then-optimize/scripts/perf_ledger.py judge \ - perf-ledger.json -``` - -Report the `judge` output verbatim, including its verdict and reason set. Only a -`pass` verdict completes the performance fix. `unchecked` is not done: repair -the missing or unreadable measurement and rerun it rather than treating the -absence of evidence as success. diff --git a/product/skills/measure-then-optimize/scripts/perf_ledger.py b/product/skills/measure-then-optimize/scripts/perf_ledger.py deleted file mode 100644 index aae7614f..00000000 --- a/product/skills/measure-then-optimize/scripts/perf_ledger.py +++ /dev/null @@ -1,130 +0,0 @@ -import argparse -import json -import subprocess -import sys -import time -from pathlib import Path - - -def number(value): - return isinstance(value, (int, float)) and not isinstance(value, bool) - - -def valid_entry(entry): - if not isinstance(entry, dict): - return entry is None - required = ("metric", "cmd", "settings", "runs") - if any(key not in entry for key in required): - return False - return (isinstance(entry["metric"], str) and isinstance(entry["cmd"], str) - and isinstance(entry["settings"], dict) and isinstance(entry["runs"], list) - and bool(entry["runs"]) and all(number(value) for value in entry["runs"])) - - -def judge(ledger): - if not isinstance(ledger, dict) or "symptom_metric" not in ledger: - return "unchecked", ["unreadable-entry"] - baseline = ledger.get("baseline") - after = ledger.get("after") - if not valid_entry(baseline) or not valid_entry(after): - return "unchecked", ["unreadable-entry"] - symptom = ledger["symptom_metric"] - reasons = [] - if baseline is None: - reasons.append("no-baseline") - if after is None: - reasons.append("no-real-after") - for entry in (baseline, after): - if entry is not None and entry["metric"] != symptom: - reasons.append("proxy-metric") - if baseline is not None and after is not None and baseline["metric"] == symptom and after["metric"] == symptom: - if baseline["cmd"] != after["cmd"] or baseline["settings"] != after["settings"]: - reasons.append("different-conditions") - for entry in (baseline, after): - if entry is not None and len(entry["runs"]) < 3: - reasons.append("single-run") - limit_change = ledger.get("limit_change") - if (isinstance(limit_change, dict) and number(limit_change.get("old")) - and limit_change.get("old") is not None and number(limit_change.get("new")) - and limit_change.get("new") is not None and limit_change["new"] > limit_change["old"]): - reasons.append("limit-loosened") - if not ledger.get("lever_path"): - reasons.append("no-lever") - reasons = sorted(set(reasons)) - return ("fail" if reasons else "pass"), reasons - - -def record(args): - values = [] - for _ in range(args.runs): - started = time.monotonic() - result = subprocess.run(args.program, capture_output=True, text=True) - if result.returncode: - sys.stderr.write(result.stderr) - return 2 - if args.from_stdout: - value = None - for line in reversed(result.stdout.splitlines()): - try: - candidate = float(line.strip()) - except ValueError: - continue - value = int(candidate) if candidate.is_integer() else candidate - break - if value is None: - sys.stderr.write("no numeric stdout value\n") - return 2 - else: - value = time.monotonic() - started - values.append(value) - path = Path(args.ledger) - if path.exists(): - data = json.loads(path.read_text()) - else: - data = {} - if not isinstance(data, dict): - data = {} - data[args.phase] = {"metric": args.metric, "cmd": " ".join(args.program), "settings": dict(item.split("=", 1) for item in args.setting), "runs": values} - if args.symptom_metric is not None: - data["symptom_metric"] = args.symptom_metric - if args.lever_path is not None: - data["lever_path"] = args.lever_path - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(json.dumps(data, indent=2) + "\n") - return 0 - - -def main(argv=None): - parser = argparse.ArgumentParser() - subparsers = parser.add_subparsers(dest="command", required=True) - judge_parser = subparsers.add_parser("judge") - judge_parser.add_argument("ledger") - record_parser = subparsers.add_parser("record") - record_parser.add_argument("--ledger", required=True) - record_parser.add_argument("--phase", choices=("baseline", "after"), required=True) - record_parser.add_argument("--metric", required=True) - record_parser.add_argument("--runs", type=int, default=5) - record_parser.add_argument("--setting", action="append", default=[]) - record_parser.add_argument("--from-stdout", action="store_true") - record_parser.add_argument("--symptom-metric") - record_parser.add_argument("--lever-path") - record_parser.add_argument("program", nargs=argparse.REMAINDER) - args = parser.parse_args(argv) - if args.command == "judge": - try: - data = json.loads(Path(args.ledger).read_text()) - except (OSError, ValueError, TypeError): - verdict, reasons = "unchecked", ["unreadable-entry"] - else: - verdict, reasons = judge(data) - print(json.dumps({"verdict": verdict, "reasons": reasons}, separators=(",", ":"))) - return {"pass": 0, "fail": 1, "unchecked": 2}[verdict] - if args.program and args.program[0] == "--": - args.program = args.program[1:] - if not args.program or args.runs < 1 or any("=" not in item for item in args.setting): - parser.error("record requires a program, positive runs, and key=value settings") - return record(args) - - -if __name__ == "__main__": - raise SystemExit(main()) diff --git a/product/skills/measure-then-optimize/tests/cases.json b/product/skills/measure-then-optimize/tests/cases.json deleted file mode 100644 index 4537c5d8..00000000 --- a/product/skills/measure-then-optimize/tests/cases.json +++ /dev/null @@ -1,21 +0,0 @@ -[ -{"id":"s01-palette-open","note":"Palette open lag; timed only in a fake browser, limit later raised 50 to 150.","ledger":{"symptom_metric":"palette_open_ms_real_app","baseline":null,"after":{"metric":"palette_open_ms_jsdom","cmd":"vitest command-palette","settings":{"env":"jsdom"},"runs":[31,29,33]},"limit_change":{"metric":"palette_open_ms_jsdom","old":50,"new":150},"lever_path":"packages/ui/src/__tests__/command-palette.test.tsx"},"expected":{"verdict":"fail","reasons":["no-baseline","proxy-metric","limit-loosened"]}}, -{"id":"s02-dag-click","note":"DAG click beachball; cause matched to an old bug, proof on a synthetic 20k-event fixture.","ledger":{"symptom_metric":"dag_click_stall_ms_real_app","baseline":null,"after":{"metric":"get_events_ms_synthetic","cmd":"vitest dag-click-get-events-cost","settings":{"events":20000},"runs":[12,11,13]},"limit_change":null,"lever_path":"packages/app/src/__tests__/dag-click-get-events-cost.test.ts"},"expected":{"verdict":"fail","reasons":["no-baseline","proxy-metric"]}}, -{"id":"s03-focus-switch","note":"Focus-switch beachball; real repro did not reproduce, fix proven on a synthetic snapshot, limit raised 100 to 150.","ledger":{"symptom_metric":"focus_switch_stall_ms","baseline":{"metric":"focus_switch_stall_ms","cmd":"playwright focus-switch","settings":{"app":"electron"},"runs":[34,30,36]},"after":{"metric":"action_graph_snapshot_ms","cmd":"vitest snapshot","settings":{"fixture":"synthetic"},"runs":[8,8,9]},"limit_change":{"metric":"focus_switch_stall_ms","old":100,"new":150},"lever_path":"packages/app/e2e/main-process-hitch-responsiveness.spec.ts"},"expected":{"verdict":"fail","reasons":["proxy-metric","limit-loosened"]}}, -{"id":"s04-worker-start-stop","note":"Worker start/stop beachball; fixed from reading code, 200ms timing added only after the fix.","ledger":{"symptom_metric":"worker_toggle_ack_ms","baseline":null,"after":{"metric":"worker_toggle_ack_ms","cmd":"playwright ui-action-responsiveness-battery","settings":{"app":"electron"},"runs":[120,115,130]},"limit_change":null,"lever_path":"packages/app/e2e/ui-action-responsiveness-battery.spec.ts"},"expected":{"verdict":"fail","reasons":["no-baseline"]}}, -{"id":"s05-terminal-lag","note":"Terminal lag; measured upserts per chunk outside the app, never keystroke latency.","ledger":{"symptom_metric":"terminal_keystroke_latency_ms","baseline":{"metric":"sqlite_upsert_ms_per_chunk","cmd":"node upsert-harness","settings":{"cap_kb":64},"runs":[0.35,0.36,0.34]},"after":{"metric":"upserts_per_chunk","cmd":"vitest terminal-upsert","settings":{"cap_kb":64},"runs":[1,1,1]},"limit_change":null,"lever_path":"packages/app/e2e/terminal-upsert-hitch-responsiveness.spec.ts"},"expected":{"verdict":"fail","reasons":["proxy-metric"]}}, -{"id":"s07-drag-lag","note":"Drag lag; real slow-query logs used, but drag was never re-timed after the fix.","ledger":{"symptom_metric":"dag_drag_lag_ms","baseline":{"metric":"events_query_ms","cmd":"slow-query log","settings":{"source":"live"},"runs":[1591,1402,1510]},"after":null,"limit_change":null,"lever_path":"scripts/repro/repro-get-events-by-types-temp-btree.sh"},"expected":{"verdict":"fail","reasons":["proxy-metric","no-real-after"]}}, -{"id":"s09-boot-vacuum","note":"10-minute boot; one boot timed, fix measured by database file size, next boot still stalled.","ledger":{"symptom_metric":"boot_seconds","baseline":{"metric":"boot_seconds","cmd":"owner restart","settings":{"host":"do1"},"runs":[739]},"after":{"metric":"db_file_mb","cmd":"stat invoker.db","settings":{"host":"do1"},"runs":[1450]},"limit_change":null,"lever_path":null},"expected":{"verdict":"fail","reasons":["single-run","proxy-metric","no-lever"]}}, -{"id":"s10-boot-reload","note":"Boot should be under 5s; fix proven by reload call count, boot never re-timed.","ledger":{"symptom_metric":"boot_seconds","baseline":null,"after":{"metric":"reload_call_count","cmd":"vitest reload-count","settings":{"fixture":"unit"},"runs":[4,4,4]},"limit_change":null,"lever_path":"packages/app/src/__tests__/reload-count.test.ts"},"expected":{"verdict":"fail","reasons":["no-baseline","proxy-metric"]}}, -{"id":"s12-relaunch-timeout","note":"Relaunch failed at 90s; timeout raised to 20 minutes from two boots; proof was the constant's value.","ledger":{"symptom_metric":"boot_seconds","baseline":{"metric":"boot_seconds","cmd":"owner relaunch","settings":{"host":"do1"},"runs":[90,780]},"after":{"metric":"timeout_constant_ms","cmd":"vitest launch-health-timeout","settings":{"fixture":"unit"},"runs":[1200000]},"limit_change":{"metric":"boot_seconds","old":90,"new":1200},"lever_path":null},"expected":{"verdict":"fail","reasons":["single-run","proxy-metric","limit-loosened","no-lever"]}}, -{"id":"s14-token-simulator","note":"Real fix-ci sessions burned 1.24B tokens; the after number came from a simulator.","ledger":{"symptom_metric":"tokens_per_fixci_session","baseline":{"metric":"tokens_per_fixci_session","cmd":"codex-session-audit","settings":{"source":"real_sessions"},"runs":[21313030,19800000,22400000]},"after":{"metric":"tokens_per_fixci_session","cmd":"fix-ci-token-bench","settings":{"source":"simulator"},"runs":[966000,970000,960000]},"limit_change":null,"lever_path":"scripts/fix-ci-token-bench.mjs"},"expected":{"verdict":"fail","reasons":["different-conditions"]}}, -{"id":"s16-runaway-spend","note":"Runaway Codex spend; a cap was claimed but never checked on a real session, spend recurred.","ledger":{"symptom_metric":"tokens_per_session","baseline":{"metric":"tokens_per_session","cmd":"fleet_cost_report","settings":{"source":"real_sessions"},"runs":[99500000,41000000,39458756]},"after":null,"limit_change":null,"lever_path":"scripts/codex-session-audit.py"},"expected":{"verdict":"fail","reasons":["no-real-after"]}}, -{"id":"s17-sqljs-oom","note":"Out of memory; the only win used a 15s window against a 20s baseline window.","ledger":{"symptom_metric":"peak_rss_mb","baseline":{"metric":"peak_rss_mb","cmd":"run-oom-repro","settings":{"timeout_sec":20},"runs":[582.8,579.1,584.0]},"after":{"metric":"peak_rss_mb","cmd":"run-oom-repro","settings":{"timeout_sec":15},"runs":[477.6,480.2,475.9]},"limit_change":null,"lever_path":"scripts/run-oom-benchmark-matrix.mjs"},"expected":{"verdict":"fail","reasons":["different-conditions"]}}, -{"id":"s18-cold-start-bytes","note":"UI cold start; measured bundle bytes, never startup time; size budget set between before and after.","ledger":{"symptom_metric":"ui_cold_start_ms","baseline":{"metric":"entry_chunk_bytes","cmd":"repro-ui-startup-bundle-size","settings":{"build":"prod"},"runs":[1770000]},"after":{"metric":"entry_chunk_bytes","cmd":"repro-ui-startup-bundle-size","settings":{"build":"prod"},"runs":[340000]},"limit_change":{"metric":"entry_chunk_bytes","old":null,"new":1300000},"lever_path":"scripts/repro/repro-ui-startup-bundle-size.sh"},"expected":{"verdict":"fail","reasons":["proxy-metric","single-run"]}}, -{"id":"s19-submission-storm","note":"The good run: real submission path timed before and after with the same script, committed and wired into the proof gate.","ledger":{"symptom_metric":"workflow_submit_ms","baseline":{"metric":"workflow_submit_ms","cmd":"bench-workflow-submission-storm","settings":{"path":"real","submissions":20},"runs":[61000,350,352,348]},"after":{"metric":"workflow_submit_ms","cmd":"bench-workflow-submission-storm","settings":{"path":"real","submissions":20},"runs":[900,340,351,347]},"limit_change":null,"lever_path":"scripts/bench-workflow-submission-storm.sh"},"expected":{"verdict":"pass","reasons":[]}}, -{"id":"s20-image-cache-first","note":"Image disk cache counted done for 8 days because its class had a passing check; never switched on.","ledger":{"symptom_metric":"images_downloaded_on_cold_start","baseline":null,"after":{"metric":"disk_cache_class_exists","cmd":"flutter check image_loader","settings":{"device":"none"},"runs":[1,1,1]},"limit_change":null,"lever_path":null},"expected":{"verdict":"fail","reasons":["no-baseline","proxy-metric","no-lever"]}}, -{"id":"s21-hourly-scan","note":"Hourly job too slow; real scheduled run before and after, but one run each and no timing script kept.","ledger":{"symptom_metric":"scan_runtime_seconds","baseline":{"metric":"scan_runtime_seconds","cmd":"launchd hourly job","settings":{"path":"production"},"runs":[104]},"after":{"metric":"scan_runtime_seconds","cmd":"launchd hourly job","settings":{"path":"production"},"runs":[5]},"limit_change":null,"lever_path":null},"expected":{"verdict":"fail","reasons":["single-run","no-lever"]}}, -{"id":"s23-ci-shards","note":"Slow CI shards; one run before and after, shard groups copied by hand.","ledger":{"symptom_metric":"ci_job_seconds","baseline":{"metric":"ci_job_seconds","cmd":"github actions run","settings":{"branch":"feature"},"runs":[1157]},"after":{"metric":"ci_job_seconds","cmd":"github actions run","settings":{"branch":"feature"},"runs":[503]},"limit_change":null,"lever_path":null},"expected":{"verdict":"fail","reasons":["single-run","no-lever"]}}, -{"id":"s24-corpus-scan-hang","note":"Corpus search hung on a 146MB file; fix proven with a mocked timeout, search time never measured.","ledger":{"symptom_metric":"corpus_scan_seconds","baseline":null,"after":{"metric":"mocked_timeout_raised","cmd":"unittest corpus_scan","settings":{"fixture":"mock"},"runs":[1,1,1]},"limit_change":null,"lever_path":"engine/skills/reflect/scripts/corpus_scan.py"},"expected":{"verdict":"fail","reasons":["no-baseline","proxy-metric"]}}, -{"id":"x01-unreadable-after","note":"A ledger whose after entry is missing its runs field cannot be judged.","ledger":{"symptom_metric":"boot_seconds","baseline":{"metric":"boot_seconds","cmd":"owner restart","settings":{"host":"do1"},"runs":[739,700,720]},"after":{"metric":"boot_seconds","cmd":"owner restart","settings":{"host":"do1"}},"limit_change":null,"lever_path":"scripts/bench-boot.sh"},"expected":{"verdict":"unchecked","reasons":["unreadable-entry"]}} -] diff --git a/product/skills/measure-then-optimize/tests/test_perf_ledger.py b/product/skills/measure-then-optimize/tests/test_perf_ledger.py deleted file mode 100644 index c79d4aca..00000000 --- a/product/skills/measure-then-optimize/tests/test_perf_ledger.py +++ /dev/null @@ -1,68 +0,0 @@ -import json -import pathlib -import subprocess -import sys -import tempfile -import unittest - - -ROOT = pathlib.Path(__file__).resolve().parents[4] -SCRIPT = ROOT / "product/skills/measure-then-optimize/scripts/perf_ledger.py" -CASES = pathlib.Path(__file__).with_name("cases.json") - - -class PerfLedgerTests(unittest.TestCase): - @classmethod - def setUpClass(cls): - cls.cases = json.loads(CASES.read_text()) - - def run_cli(self, *args): - return subprocess.run([sys.executable, str(SCRIPT), *args], capture_output=True, text=True) - - def judge_case(self, case): - with tempfile.TemporaryDirectory() as directory: - ledger = pathlib.Path(directory) / "ledger.json" - ledger.write_text(json.dumps(case["ledger"])) - result = self.run_cli("judge", str(ledger)) - self.assertEqual(result.returncode, {"pass": 0, "fail": 1, "unchecked": 2}[case["expected"]["verdict"]]) - self.assertEqual(json.loads(result.stdout), {"verdict": case["expected"]["verdict"], "reasons": sorted(case["expected"]["reasons"])}) - - def test_record_wall_clock(self): - with tempfile.TemporaryDirectory() as directory: - ledger = pathlib.Path(directory) / "ledger.json" - result = self.run_cli("record", "--ledger", str(ledger), "--phase", "baseline", "--metric", "boot_seconds", "--runs", "3", "--setting", "host=local", "--", sys.executable, "-c", "pass") - self.assertEqual(result.returncode, 0) - data = json.loads(ledger.read_text()) - self.assertEqual(data["baseline"]["metric"], "boot_seconds") - self.assertEqual(data["baseline"]["settings"], {"host": "local"}) - self.assertEqual(len(data["baseline"]["runs"]), 3) - self.assertTrue(all(isinstance(value, (int, float)) for value in data["baseline"]["runs"])) - - def test_record_from_stdout(self): - with tempfile.TemporaryDirectory() as directory: - ledger = pathlib.Path(directory) / "ledger.json" - result = self.run_cli("record", "--ledger", str(ledger), "--phase", "after", "--metric", "boot_seconds", "--runs", "2", "--from-stdout", "--symptom-metric", "boot_seconds", "--lever-path", "bench.py", "--", sys.executable, "-c", "print('ignored'); print(12.5)") - self.assertEqual(result.returncode, 0) - self.assertEqual(json.loads(ledger.read_text())["after"]["runs"], [12.5, 12.5]) - - def test_record_failure_leaves_ledger_untouched(self): - with tempfile.TemporaryDirectory() as directory: - ledger = pathlib.Path(directory) / "ledger.json" - original = '{"sentinel": true}\n' - ledger.write_text(original) - result = self.run_cli("record", "--ledger", str(ledger), "--phase", "after", "--metric", "boot_seconds", "--runs", "2", "--", sys.executable, "-c", "import sys; print('bad', file=sys.stderr); sys.exit(3)") - self.assertEqual(result.returncode, 2) - self.assertIn("bad", result.stderr) - self.assertEqual(ledger.read_text(), original) - - -def make_case_test(case): - return lambda self: self.judge_case(case) - - -for case in json.loads(CASES.read_text()): - setattr(PerfLedgerTests, "test_case_" + case["id"], make_case_test(case)) - - -if __name__ == "__main__": - unittest.main()