fix(ci): address PR #35 review comments
- 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 <noreply@anthropic.com>
This commit is contained in:
@@ -55,6 +55,12 @@ fn read_rss_kb() -> Option<u64> {
|
||||
/// 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<usize> =
|
||||
|
||||
+43
-43
@@ -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
|
||||
}
|
||||
]
|
||||
}
|
||||
|
||||
+13
-4
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user