cargo perf --baseline <report.json>: a diff that refuses a difference the spread cannot support #301
Labels
No labels
agent
agent:ci
agent:done
agent:failed
agent:needs-input
agent:refined
agent:refining
agent:running
agent:shipped
agent:skip
autonomous
autopilot
driven
local
plan
proposal
qa
qa-gap
research
retro
ship
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
jeroen/cartopolis#301
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem
cargo perfmeasures a run and writes a JSON report (crates/cartopolis/src/systems/dev/perf.rs:366, alias at.cargo/config.toml:88), but nothing reads one back. Comparing two runs today means opening two JSON files and subtracting columns by eye — which is precisely how the withdrawn sky-shell A/B happened: three runs of one unchanged configuration reported fill counts of 160 836, 218 436 and 218 436, two of them differing by exactly one full screen, and an A/B differenced against single readings produced a clean-looking table saying that turning the clouds off increased fill (docs/notes/performance-suite.md:71).The report already carries what is needed to refuse that: each window samples
fragment_shader_invocationsevery frame and recordsfrag_min,frag_maxandfrag_samplesper render pass (perf.rs:291, sampled atperf.rs:573), and the field's own doc says a point sample "is not reproducible and will mislead you" (perf.rs:283).ReportMetaexists so "a report says what it is comparable with" (perf.rs:349). Nothing consumes either.Three further facts the diff has to respect, all already written down:
docs/notes/render-performance-survey.md:26-31is an explicit table of what is trustworthy in this container:draws/tris_*yes, asset counts yes,worker_outstandingtiming-dependent (report only),frame_msno,elapsed_gpu"theatre".Window::fpsrepeats it at the field: "Reported, never gated" (perf.rs:306), as doesGpuPass::gpu_ms— "Structure yes, magnitude no" (perf.rs:273).process_cpu_percent/process_mem_percentonly exist under--features perf(lib.rs:1781), and the GPU passes only whenRenderDiagnosticsPluginis added (lib.rs:2209).Option::Noneon one side is not measured, not a delta from zero.steadywindow that is notquietis a streaming measurement wearing the wrong label (perf.rs:344, warned atperf.rs:707), andcitydoes not go quiet on this container (docs/perf.toml, thecitydescription).Approach
One new module,
crates/cartopolis/src/systems/dev/perf_diff.rs, holding a pure function over two already-parsedReports plus a logger for the result. Nothing in it touches Bevy, so it is unit-testable without an App.Data model.
Which verdict each column gets, and why. The classification is a table in the module, each row carrying the citation for its bucket — the survey table above is the authority, and a column it does not name is placed by the closest row it does.
draws,tris_total,entity_count,mesh_slabs,mesh_slab_mb,resident_kb,resident_image_kb,resident_mesh_kbUnchanged/Establisheddocs/notes/performance-suite.md, "Where the gigabyte is")frag_min..frag_maxperf.rs:283fps,frame_time_ms, per-passgpu_ms,cpu_ms,process_cpu_percent,process_mem_percentNotEvidenceperf.rs:273,perf.rs:306,perf.rs:310upload_kb,worker_jobs,mesh_added_per_frame,frames,wall_sNotEvidencequiettest (perf.rs:705), so they are printed for contextfragment_invocations(the smoothed point value)perf.rs:283: differencing it is the withdrawn experimentNotEvidencerows are still printed with their before/after and delta — that is the "structure yes, magnitude no" reading — but the renderer marks them and they are never counted as a difference.The spread test, for each pass path present in both windows:
frag_min/frag_max, orfrag_samples < 2on either side →NotMeasured, note saying a single sample cannot support a difference. This guard is the point: a one-frameloadwindow is reachable (perf.rs:643) and would otherwise produce a zero-width interval and a confident wrong answer.a.min <= b.max && b.min <= a.max) →NotEstablished, note carrying both ranges and both sample counts.Established, withdeltathe conservative gap between the nearest edges (b.min - a.maxwhen the new run is higher,b.max - a.minwhen lower) — the smallest difference consistent with both ranges — andpercentagainst the baseline midpoint. Note that this test is identical to "the midpoint delta exceeds half the combined spread"; it is stated as an interval test because that is what the data is.NotMeasured.Matching and meta. Windows are matched by
(scenario, phase); unmatched ones go intoonly_in_baseline/only_in_currentand are logged by name. A duplicate(scenario, phase)key in either report is an error naming the key rather than a silent first-match (the manifest already refuses duplicate scenario names atperf.rs:146; a report with duplicates is malformed and pairing it silently is the "baseline nobody can reproduce" failureperf.rs:80guards against). EveryReportMetafield that differs becomes one line inmeta_differences— a warning, not an error, becauseCARTO_QUALITY=phone cargo perf --only cityagainst a desktop report is a question somebody legitimately asks.Wiring (
perf.rs,lib.rs):PerfRungainspub baseline: Option<Report>— the parsed report, not a path.perf::build()(perf.rs:444) gains a baseline-path parameter and loads and parses it eagerly, returningErrin the same shape asPerfManifest::load(perf.rs:139,"{path}: {e}"). A typo'd path must fail before the first teleport, not after a suite run; this is the same doctrine as an unknown manifest key being a startup error.perf::finish()(perf.rs:772) callsperf_diff::diff+log_diffafter the report is written, so the diff describes the report on disk.lib.rsgains#[arg(long, alias = "baseline", value_name = "PATH")] perf_baseline: Option<PathBuf>beside--perf-out/--perf-frames(lib.rs:113-120). The canonical spelling is--perf-baseline, matching its two siblings;--baselineis kept as an alias so the spelling in this ticket's title works. Passing it without--perfis an error in the shape oflib.rs:265.tracing, one structured line per row plus a summary, matchinglog_window/"perf gpu pass"(perf.rs:724-768). Noprintln!— nothing insystems/dev/uses one.Docs. A "Comparing two runs" section in
docs/notes/performance-suite.md(it already ends on the fill-counter reproducibility problem, which is what this answers), and thecargo perfcomment block at.cargo/config.toml:83-88gains the flag.Acceptance criteria
cargo perf --perf-baseline <report.json>runs the suite, writes its report as before, and then logs a per-window, per-column diff.--baselineis accepted as an alias.--perf-baselinewithout--perfis a startup error naming the flag.(scenario, phase). A window present in only one report is logged by name under "only in the baseline" / "only in the new report" and is never silently dropped.(scenario, phase)in either report is an error naming the key.ReportMetafields are logged as a warning listing each field with both values; the diff still runs.[frag_min, frag_max]: overlapping intervals giveNotEstablishedwith both ranges in the note; disjoint intervals giveEstablishedwith the conservative edge-to-edge delta.frag_samples < 2on either side givesNotMeasured, not a delta.fragment_invocationspoint value is not diffed.NotEvidenceand are excluded from the "established differences" count in the summary line.quietflag is reported for both sides; asteadypair where either side is not quiet logs the warning that its numbers describe a world being built. Deltas are still printed —citynever goes quiet on this container, and suppressing it would delete the only ground measurement.perf_diff.rs, over fixtureReports built in the test module (inline JSON, so the loader is exercised too — no new files, and no dependence on the repo root the waythe_shipped_manifest_parsesneeds atperf.rs:854):draws— is reported as a delta with the expected sign and magnitude;NotEstablished;only_in_baseline/only_in_current;frag_samples == 1pair isNotMeasured;Noneon one side isNotMeasured, not a delta from zero;Establishedrow.docs/notes/performance-suite.mdgains a section saying which columns can carry a difference and which cannot, and why;.cargo/config.toml'scargo perfcomment mentions the flag.docs/direction.mdthat settled the column classification (value 6, ship the smallest thing a machine can judge — counts and spreads are judgeable, timings on this host are not) — required anyway, since this is afeattouching more than five files.Verification
End to end, on a host that can render (lavapipe here is enough — the measurements in
docs/notes/performance-suite.mdwere taken on this container; the container this refinement pass runs in must not build):orbitis the scenario that reliably goes quiet, so it is the one to smoke-test with. Two unchanged runs of it should produce noEstablishedrow; if they do, the diff is reporting run-to-run noise as a finding and the column that did it needs reclassifying before this lands. A--features perfrun diffed against a non-perfone is the check that absent columns come backNotMeasured.Not checkable here or on this container at all: whether the classification holds on a real GPU. The Steam Deck runner (
deck-gpu, see thesteamdeck-gpu-runnernote) is where a desktop-GPU pair of reports would come from.Out of scope
--expectalready exists for gating a scripted run.--perf-diff a.json b.json). The pure function makes it a few lines later; it is a second entry path intolib.rsand a separate ticket.Window/GpuPassfields#[serde(default)]so an older baseline still parses. Deliberately not done: a missing column would silently read 0 and be reported as a large delta, which is worse than the parse error. A baseline from a build with different columns fails loudly, with the path in the message.docs/perf.toml.Open questions
None.
Branch:
feat/301-perf-baseline-diffOriginal request
From the maintainer's focus for this lane:
cargo perf --baseline <report.json>: a diff that refuses a difference the spread cannot support
Same module as the ticket above and the half that makes it useful. Reads a previous report, matches windows by (scenario, phase), prints per-column deltas, and where a delta is smaller than the combined spread of the two runs reports it as not established rather than as a number. This is the tool every later ticket here is judged with, and it is what stops a fourth repetition of the sky-shell A/B that produced a clean-looking table saying turning the clouds off increased fill. Judged by: unit tests over two fixture reports — a real change reported as a delta, a change inside the spread reported as not established, and a window present in one report and not the other named rather than silently dropped.
This is item 3 of 20 of the lane's queue. It is ordered by a person in the
admin console, so build this one rather than the one you would have picked — and keep it
the size it is. A ticket that turns out to be three tickets is one ticket: finish this
part and say on it what the other two are.
If the item is simply wrong — already done, impossible, or a bad idea against the values
— say so here and close it. That is a result, not a failure.
Decide it yourself. This ticket is not being watched, so a question asked here is a
ticket that stops. The seven values at the top of
docs/direction.mdexist to settleexactly that kind of ambiguity: pick the reading they support, say in the commit body
which one you applied, and build. Only a decision needing something nobody can derive
from the repository — a credential, a licence somebody must accept, a choice about what
the project is for — is a reason to stop.
Filed by the autopilot.
🤖 Refined by the viberfox issue agent. Reply with @agent refine and what is wrong to have this rewritten.
🤖 Refining this before anything gets built — reading the code, no branch, no commits. If it turns out I'd have to guess at the requirements I'll stop and ask instead.
🤖 Refined. The description above is now the spec; I'll pick it up for implementation on the next poll.
If the direction is wrong, say so now — @agent refine with a correction rewrites it, and
agent:skipstops me.🤖 viberfox issue agent on the VPS — 21 turns · $2.31 · session
c7fd21a9-70e2-476e-bb32-4d466ef13b3f. Reply with @agent to continue this session.🤖 Picked this up — working on branch
feat/301-perf-baseline-diffin a fresh worktree on the VPS. I'll report back here when I'm done.Label an issue
agent:skipto keep me off it, ordrivento keep it in a session you are driving.cargo perfcan now compare a run against a previous one, and it refuses differences the two runs' own measured spread cannot support.What changed
crates/cartopolis/src/systems/dev/perf_diff.rs— a pure function over two parsed reports (no Bevy, no filesystem), plus a logger. It matches windows by scenario and phase, and gives every column one of five verdicts.--perf-baseline <path>(--baselineis accepted as an alias). The suite runs and writes its report exactly as before, then logs the diff. The baseline file is read and parsed at startup, so a wrong path fails before the first scenario is flown. Passing it without--perfis refused by name.docs/notes/performance-suite.mdgained a "Comparing two runs" section, and thecargo perfcomment in.cargo/config.tomlmentions the flag.The classification is the substance of it. Draw calls, triangle and entity counts, mesh slabs and the three residency numbers are compared exactly. Per-pass fill is compared as the interval the report already records — smallest and largest fragment count seen across the window — so two ranges that overlap come back as not established rather than as a number, and where they are disjoint the reported delta is the gap between the nearest edges, i.e. the smallest change both runs allow. Fewer than two samples on either side is not measured. Frame rate, frame time, per-pass GPU and CPU times, the process percentages and the per-window streaming traffic are printed with their deltas but marked not evidence and never counted. A column absent on one side is not measured, never a delta from zero. The reason for the split is value 6 of
docs/direction.md— counts and spreads are judgeable on this hardware, timings are not — and the commit body says so.How I verified it
cargo test -p cartopolis— 951 passed, 0 failed. That includes 12 new tests inperf_diff: a real change reported with the right sign and size, a change inside the spread reported as not established, a window present in only one report named rather than dropped, a single-sample pair not measured, an absent column not measured, and a report diffed against itself producing no established row.cargo test -p cartopolis_geo— 255 passed, 0 failed.cargo fmt --checkclean on all five owned crates.One test caught something worth knowing: the theme suite scans every short string literal in the tree for characters the bundled font cannot draw, and it rejected the
→I had used in the "this meta field differs" message. It is->now.What is not verified
I did not run the suite end to end. Doing that means launching the renderer, and this session's standing instruction is not to attempt rendered runs here. So the flag has never actually flown a scenario and diffed a real report — only the pure function is exercised. The check to run on a machine that can render is the one in the ticket:
Two unchanged runs of
orbitshould produce no established row. If they do, a column is reporting run-to-run noise as a finding and needs reclassifying before this is trusted. Whether the classification holds on a real graphics processor is also open; that needs a pair of reports off the Steam Deck runner.For you to decide
Nothing is blocked. Two things the ticket deliberately left out, if you want them as follow-ups: diffing two saved reports without flying anything (
--perf-diff a.json b.json— the pure function makes it a few lines, but it is a second way into the command line), and recording a spread for columns other than fill, which only becomes worth doing once reports come off real hardware.Branch
feat/301-perf-baseline-diff(pushed)🤖 viberfox issue agent on the VPS — 284 turns · $28.70 · session
54cceff7-840b-4846-b360-ad33faa3f323. Reply with @agent to continue this session.🤖 Marked
agent:donewithout a CI verdict on795f02bd— no workflow reported on this commit. The branch is waiting for review; check the run yourself before landing it.🤖 Not shipping this one — the branch conflicts with
main(another ticket landed first).Rebase
feat/301-perf-baseline-diffontomain, resolve, push, and put theshiplabel back to land it.