From d0596d27631cb4f0c1b1e0975790463cd7c61ede Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Wed, 18 Feb 2026 13:07:46 +0100 Subject: [PATCH] fix(ci): address PR #35 review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Document non-blocking receive contract in perf_bench.rs docstring, confirming no TCP deadlock race (Hoshe #1, critical) - Make shadowcast parser order-independent — flush on new config header instead of requiring Recursive after Symmetric (Hoshe #2) - Fix p95 calculation: use floor(0.95*(N-1)) nearest-rank instead of ceil(0.95*N)-1 which was off-by-one at N=50 (Hoshe #4) - Error on --compare when no baseline file exists (Hoshe #5) - Add D-031 10tps assumption comment to TICK_BUDGET_US (Tyre #1) - Strengthen snapshot assertion: require warmup + half measurement window instead of warmup + 1 (Tyre #2) - Regenerate baseline with corrected p95 (356µs, was 526µs) Hoshe #3 (.PHONY) was already addressed — perf-baseline is in the .PHONY declaration on Makefile line 10. Co-Authored-By: Claude Opus 4.6 --- server/tests/perf_bench.rs | 21 +++++++--- tests/perf/baseline.json | 86 +++++++++++++++++++------------------- tooling/perf-baseline | 17 ++++++-- 3 files changed, 72 insertions(+), 52 deletions(-) diff --git a/server/tests/perf_bench.rs b/server/tests/perf_bench.rs index 788ea311a..a11f2f6eb 100644 --- a/server/tests/perf_bench.rs +++ b/server/tests/perf_bench.rs @@ -55,6 +55,12 @@ fn read_rss_kb() -> Option { /// Boots the server with production content, runs WARMUP_TICKS to stabilize, /// then measures MEASURE_TICKS of app.update() wall-clock time. Reports entity /// counts from observer snapshots and process RSS. +/// +/// Protocol contract: BridgePlugin uses non-blocking receive (WouldBlock → +/// empty input vec), so the server always advances even if the client hasn't +/// sent input yet. The server sends a snapshot each tick; the client blocks +/// on read until one arrives, then responds with (empty) input. No deadlock +/// possible — see TcpBridge::receive_inputs and send_snapshot in bridge/tcp.rs. #[test] #[ignore] fn perf_tick_timing() { @@ -145,12 +151,16 @@ fn perf_tick_timing() { } } - // Must have received enough data past warmup for meaningful results + // Must have received enough measured snapshots for meaningful results. + // Require all warmup ticks plus at least half the measurement window. + let min_snapshots = WARMUP_TICKS + MEASURE_TICKS / 2; assert!( - entity_counts.len() > WARMUP_TICKS, - "Only received {} snapshots, need at least {} (warmup) + 1", + entity_counts.len() >= min_snapshots, + "Only received {} snapshots, need at least {} ({} warmup + {} measured)", entity_counts.len(), - WARMUP_TICKS + min_snapshots, + WARMUP_TICKS, + MEASURE_TICKS / 2 ); drop(reader); @@ -174,7 +184,8 @@ fn perf_tick_timing() { let mut sorted = measured_us.clone(); sorted.sort(); - let p95_idx = ((sorted.len() as f64 * 0.95).ceil() as usize).saturating_sub(1); + // Nearest-rank p95: index = floor(0.95 * (N-1)) for 0-based indexing. + let p95_idx = ((sorted.len() - 1) as f64 * 0.95).floor() as usize; let p95 = sorted[p95_idx.min(sorted.len() - 1)]; let entity_counts_measured: Vec = diff --git a/tests/perf/baseline.json b/tests/perf/baseline.json index 04824102e..1266639a7 100644 --- a/tests/perf/baseline.json +++ b/tests/perf/baseline.json @@ -1,15 +1,15 @@ { - "timestamp": "2026-02-18T11:39:46.913254+00:00", + "timestamp": "2026-02-18T12:07:18.546680+00:00", "git": { - "commit": "b4a3784", + "commit": "c1d7c07", "branch": "ci" }, "tick_timing": { - "max_us": 537, - "mean_us": 366, + "max_us": 363, + "mean_us": 332, "measured_ticks": 50, - "min_us": 314, - "p95_us": 526, + "min_us": 310, + "p95_us": 356, "warmup_ticks": 5 }, "entities": { @@ -17,7 +17,7 @@ "max_per_snapshot": 21 }, "memory": { - "rss_kb": 6972 + "rss_kb": 6900 }, "shadowcast": { "configs": [ @@ -26,90 +26,90 @@ "density": "open field", "range": 20, "iterations": 1000, - "symmetric_total_ms": 62.2, - "symmetric_per_call_us": 62.2, - "recursive_total_ms": 107.25, - "recursive_per_call_us": 107.25 + "symmetric_total_ms": 57.86, + "symmetric_per_call_us": 57.86, + "recursive_total_ms": 97.97, + "recursive_per_call_us": 97.97 }, { "map_size": 32, "density": "moderate corridors", "range": 20, "iterations": 1000, - "symmetric_total_ms": 60.15, - "symmetric_per_call_us": 60.15, - "recursive_total_ms": 211.61, - "recursive_per_call_us": 211.61 + "symmetric_total_ms": 52.29, + "symmetric_per_call_us": 52.29, + "recursive_total_ms": 189.99, + "recursive_per_call_us": 189.99 }, { "map_size": 32, "density": "dense rooms", "range": 20, "iterations": 1000, - "symmetric_total_ms": 24.28, - "symmetric_per_call_us": 24.28, - "recursive_total_ms": 107.95, - "recursive_per_call_us": 107.95 + "symmetric_total_ms": 21.31, + "symmetric_per_call_us": 21.31, + "recursive_total_ms": 98.47, + "recursive_per_call_us": 98.47 }, { "map_size": 64, "density": "open field", "range": 20, "iterations": 1000, - "symmetric_total_ms": 75.38, - "symmetric_per_call_us": 75.38, - "recursive_total_ms": 108.22, - "recursive_per_call_us": 108.22 + "symmetric_total_ms": 60.5, + "symmetric_per_call_us": 60.5, + "recursive_total_ms": 98.33, + "recursive_per_call_us": 98.33 }, { "map_size": 64, "density": "moderate corridors", "range": 20, "iterations": 1000, - "symmetric_total_ms": 48.72, - "symmetric_per_call_us": 48.72, - "recursive_total_ms": 202.49, - "recursive_per_call_us": 202.49 + "symmetric_total_ms": 47.41, + "symmetric_per_call_us": 47.41, + "recursive_total_ms": 184.87, + "recursive_per_call_us": 184.87 }, { "map_size": 64, "density": "dense rooms", "range": 20, "iterations": 1000, - "symmetric_total_ms": 14.5, - "symmetric_per_call_us": 14.5, - "recursive_total_ms": 90.33, - "recursive_per_call_us": 90.33 + "symmetric_total_ms": 14.35, + "symmetric_per_call_us": 14.35, + "recursive_total_ms": 79.6, + "recursive_per_call_us": 79.6 }, { "map_size": 150, "density": "open field", "range": 20, "iterations": 1000, - "symmetric_total_ms": 59.7, - "symmetric_per_call_us": 59.7, - "recursive_total_ms": 103.09, - "recursive_per_call_us": 103.09 + "symmetric_total_ms": 56.65, + "symmetric_per_call_us": 56.65, + "recursive_total_ms": 97.8, + "recursive_per_call_us": 97.8 }, { "map_size": 150, "density": "moderate corridors", "range": 20, "iterations": 1000, - "symmetric_total_ms": 43.64, - "symmetric_per_call_us": 43.64, - "recursive_total_ms": 178.09, - "recursive_per_call_us": 178.09 + "symmetric_total_ms": 40.49, + "symmetric_per_call_us": 40.49, + "recursive_total_ms": 158.81, + "recursive_per_call_us": 158.81 }, { "map_size": 150, "density": "dense rooms", "range": 20, "iterations": 1000, - "symmetric_total_ms": 12.63, - "symmetric_per_call_us": 12.63, - "recursive_total_ms": 79.87, - "recursive_per_call_us": 79.87 + "symmetric_total_ms": 12.88, + "symmetric_per_call_us": 12.88, + "recursive_total_ms": 77.98, + "recursive_per_call_us": 77.98 } ] } diff --git a/tooling/perf-baseline b/tooling/perf-baseline index 2b9079976..c5355f0af 100755 --- a/tooling/perf-baseline +++ b/tooling/perf-baseline @@ -22,7 +22,8 @@ ROOT = Path(__file__).resolve().parent.parent PERF_DIR = ROOT / "tests" / "perf" BASELINE_FILE = PERF_DIR / "baseline.json" -# Tick budget from D-026 +# Tick budget from D-026: 100ms per tick at 10 tps floor (D-031). +# If tick rate changes, update this constant. TICK_BUDGET_US = 100_000 # 100ms @@ -95,6 +96,9 @@ def run_shadowcast_benchmark(): line, ) if m: + # New config block — flush previous if complete + if current.get("map_size"): + configs.append(current) current = { "map_size": int(m.group(1)), "density": m.group(3), @@ -113,11 +117,12 @@ def run_shadowcast_benchmark(): if m: current["recursive_total_ms"] = float(m.group(1)) current["recursive_per_call_us"] = float(m.group(2)) - if current.get("map_size"): - configs.append(current) - current = {} continue + # Flush last config + if current.get("map_size"): + configs.append(current) + return {"configs": configs} if configs else None @@ -252,6 +257,10 @@ def main(): if compare_mode and regressions: print(f"\n{len(regressions)} regression(s) detected.") return 1 + elif compare_mode: + print(f"\nERROR: --compare requires a saved baseline at {BASELINE_FILE.relative_to(ROOT)}") + print("Run `make perf-baseline` first to create one.") + return 1 if compare_mode: return 0