Repository navigation
feat(bench): carry the host-lock verdict in every CPU benchmark, from one definition - #1950
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1950 +/- ##
==========================================
- Coverage 80.42% 80.40% -0.02%
==========================================
Files 420 424 +4
Lines 202834 207288 +4454
Branches 202834 207288 +4454
==========================================
+ Hits 163128 166680 +3552
- Misses 34143 34973 +830
- Partials 5563 5635 +72
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Review of #1950 found the module's central claim false: `Report`'s fields were `pub`, so any caller could write `Report { field: Mine("me"), .. }` and get a protected, un-warned verdict derived from no reading at all -- the flattering answer, available by accident. The doc said that was impossible and a test named `a_report_cannot_be_had_from_one_reading` said so too, while the test beside it constructed one exactly that way. - `Report`'s fields are private with accessors. The claim is now asserted by `compile_fail` doctests. Those are weak alone -- rustdoc does not enforce the error code, and a `Reportt` typo passes; measured, not assumed -- so they carry a positive control that must compile, and a mutation that makes the fields `pub` and must be killed. - Closing takes the self-owner as an argument. Two tests read `HOSTLOCK_OWNER` from the ambient environment and asserted the arm that only occurs when it is unset, so the suite failed in the one shell this subsystem is used from, and the mutation harness could not reach a green baseline there. - `warning()` names the owner the verdict was judged against rather than re-reading the environment, which could name one the field never saw. - `Window::open` and `Window::close` -- the only two functions a benchmark calls -- were covered by nothing; every test used the injected form. A child-process probe drives them against the real script, the real `HOSTLOCK_DIR` and the real `HOSTLOCK_OWNER`, asserting `mine:`/`foreign:`/ `held:`/`changed` row text. Three new mutants are killed by it alone. - The changed-hands test now runs both directions; the reason-from-the-second -reading mutant was killed only by the Linux-gated integration test before. 37 mutants, 37 killed. Full crate suite green with `HOSTLOCK_OWNER` set and unset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
numbers that could not say whether anyone had declared the host, which is a worse state than none of them carrying it: a reader who sees the field on one matrix and not on another cannot tell whether the second was unprotected or merely older, and absence reads as "fine". The two-ended read is now a type. `Window::open` takes the first reading, `close` takes the second, and there is no way to obtain a `Report` without both -- a caller wanting a one-ended verdict has to go around the module rather than merely forget something. That matters because the one-ended version is the flattering one: a single end-of-run read names a credible holder for a run that changed hands, and the row looks entirely normal. The row text, the `changed` rule and the `UNPROTECTED` warning have one definition rather than eleven. `decode_gap_park_ab` formatted its own, which was fine for one caller and a drift hazard for ten; a `host_lock=` that means one thing in one matrix and another in the next is worse than an absent field. It is the same argument #1926 is making about `efficiency` naming two quantities, applied to our own instruments. The window opens before warmup, not before the timed region: a warmup that shared cores with somebody else's run leaves caches and frequency in a state the timed region inherits. Verified by mutation, and the mutation run paid for itself immediately. Six new mutants for the window brought the harness to 31, of which two initially survived by not compiling -- and fixing them exposed that `close` itself was untested, because every test reached the logic through `closed_at` and nothing could make the real reader return two different values on demand. `close_with` now takes the second read as a parameter, for the same reason `classify_io` takes its probe as one, and the arm that matters -- the two ends disagreeing -- is reachable. 31 mutants, 31 killed. The integration test drives the real `hostlock.sh`: it reads a scratch directory the script has not touched, has the script acquire it, reads again, and asserts the window reports `changed` with no reason attached. A `changed` row that named the late holder would invite a reader to treat the window as covered after all. It also pins that a reason written with a space in it comes back with an underscore, asserted against a reason the script wrote rather than one built in-process, because that is the path an injected reason would actually travel. Closes #1948 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review of #1950 found the module's central claim false: `Report`'s fields were `pub`, so any caller could write `Report { field: Mine("me"), .. }` and get a protected, un-warned verdict derived from no reading at all -- the flattering answer, available by accident. The doc said that was impossible and a test named `a_report_cannot_be_had_from_one_reading` said so too, while the test beside it constructed one exactly that way. - `Report`'s fields are private with accessors. The claim is now asserted by `compile_fail` doctests. Those are weak alone -- rustdoc does not enforce the error code, and a `Reportt` typo passes; measured, not assumed -- so they carry a positive control that must compile, and a mutation that makes the fields `pub` and must be killed. - Closing takes the self-owner as an argument. Two tests read `HOSTLOCK_OWNER` from the ambient environment and asserted the arm that only occurs when it is unset, so the suite failed in the one shell this subsystem is used from, and the mutation harness could not reach a green baseline there. - `warning()` names the owner the verdict was judged against rather than re-reading the environment, which could name one the field never saw. - `Window::open` and `Window::close` -- the only two functions a benchmark calls -- were covered by nothing; every test used the injected form. A child-process probe drives them against the real script, the real `HOSTLOCK_DIR` and the real `HOSTLOCK_OWNER`, asserting `mine:`/`foreign:`/ `held:`/`changed` row text. Three new mutants are killed by it alone. - The changed-hands test now runs both directions; the reason-from-the-second -reading mutant was killed only by the Linux-gated integration test before. 37 mutants, 37 killed. Full crate suite green with `HOSTLOCK_OWNER` set and unset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9c44f09 to
a4dfd23
Compare
Independent review responseRan an adversarial review of this PR in a separate context ( 1. The PR's central claim was false —
|
…tion Independent review returned REQUEST CHANGES on five findings. All five were reproduced before being fixed; two of them are defects in the section's own reasoning rather than typos. Citations (both MAJOR). The hostmon guard was cited as #2026 and the benchmark harness-bug record as #1628. Both wrong, and wrong by the same mechanism: I used `git log -1 -- <file>`, which resolves to the file's most recent commit, not to the commit that introduced the line. `git log -S` on the exact text gives #1950 for the guard (a7003c9) and #1619 for the record (fb38341); #2026 does not touch the guard at all (0 hunks). For a section whose entire value is that the prior records were unfindable, wrong pointers are the defect itself, so this is now a **Check:** in the text. The `1 passed` claim was backwards (MINOR). I wrote that the substring check false-alarms when a filter selects more than one test -- the safe direction. Measured, it fails in the dangerous one: 11 passing tests -> "11 passed; 0 failed" contains "1 passed" -> accepted 2 selected, 1 failing -> "1 passed; 1 failed" contains "1 passed" -> accepted Both are false greens; the second accepts a run holding a real failure. The section now states this, explains why the check is nonetheless sound in agrees_with_hostlock_sh.rs (window_probe_child is a crate-root `#[test]`, so exactly one test is selected by construction), and draws the conclusion the measurements actually support: the two checks compose. The listing pins the selection to one, which is what makes reading the run output sound afterwards. Neither half is sufficient alone. `--list` counts `#[ignore]`d tests (MINOR). `tests::an_ignored_test` lists with the same `: test` suffix and resolves to n=1 while the run executes nothing (`0 passed; 0 failed; 1 ignored`). The listing proves the name resolves, never that the arm ran -- which is why the run-output half is not optional. Noted in the text. My first probe of this claim returned n=0 and appeared to refute it; the probe had used a bare name and selected nothing, i.e. it fell into the trap being documented, so the refutation was the apparatus failing rather than the claim. Design doc (NIT): "a bare name matches nothing" is false for a crate-root test, the asymmetry §9 is careful about. Qualifier restored. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 participant
Closes #1948.
Every CPU benchmark now carries the advisory host lock's verdict for its own measurement window, from one definition.
scripts/hostlock.shis how agents sharing this box declare "I am measuring, stay off". The reader for it landed in #1925/#1936, but onlydecode_gap_park_abandbench_genericconsulted it — so nine other benchmarks published rows that cannot be told apart from rows taken beside somebody else'scargo test. That is the same failure this campaign keeps hitting: not a wrong number, a number nobody can attribute afterwards.The window is a type, not a convention
crates/onnx-runtime-hostmon/src/window.rs.Window::open()reads the lock,close()reads it again, and the pair reduces to aReport.A single end-of-run read is worse than no read at all: it names a credible holder for a run that changed hands halfway, and the row looks entirely normal. So the two-ended read is not left to each caller to remember —
Reportcannot be obtained from one reading. Fields are private,Windowis the only constructor, and that is asserted bycompile_faildoctests carrying a positive control (rustdoc does not enforce error codes — measured, see the review comment) plus a mutant that makes the fieldspuband must be killed.host_lock=changed lock_reason=acc0, naming a holder for a window the field says had none.host_lock=line is ten chances to drift, and a field meaning one thing in one matrix and another in the next is worse than an absent field.decode_gap_park_ab's hand-rolled copy is deleted in favour of the shared helper — it was already a second vocabulary.HOSTLOCK_OWNERwould assert the shell it ran in.Wiring
open_host_lock_window()/report_host_lock()inbenches/common/mod.rs, two lines per binary:activation_bench,half_decode_gemv_ab,half_prefill_route_ab,int4_acc0_attribution,int4_decode_loop_ab,int4_prefill_route_ab,int8_prefill_route_ab,matmul_nbits_prefill_ab,native_vs_mlas, anddecode_gap_park_abconverted.Unprotected windows warn loudly on stderr but never abort. Refusing to print a matrix because nobody took a lock would mostly teach people to stop taking the lock; an unlocked run on a genuinely idle box is fine. What is not fine is one that cannot be told apart from it afterwards.
Note for log scrapers: the consolidated warning says "this run was not covered end-to-end";
decode_gap_park_ab's deleted copy said "this matrix".Tests
hostlock.sh, including a child-process probe that exercisesWindow::open()/close()against a real lock directory and a realHOSTLOCK_OWNER— asserting the exact row text formine:,foreign:,held:andchanged. A child, not a process-wide env override, so nothing can leak into another test.scripts/hostlock_mutants.py, extended to a second file). Three are killed only by the child probe; one only by the added change direction. Two earlier mutants failed to compile, which is a mutant written wrong rather than a kill — fixing them is what exposed that closing was untested.HOSTLOCK_OWNERset and unset;cargo fmt,clippy -D warnings, andcargo doc(0 warnings) on Linux andaarch64-pc-windows-msvc; all ten bench targets compile.Rebased onto
011fbb284, so this sits on top of #1942's configurable lock directory; its two new agreement tests and this PR's coexist.Not verified
No benchmark binary has been run end-to-end to observe its
host_lock=line print. The row text is now observed from real code reading a real lock via the child probe, so what remains unobserved is the two lines in eachmain. The host has been under another agent's lock or above my own contention gate throughout; I will post an observed row here before merging rather than quietly drop this section.An independent adversarial review of the first revision found five real defects, two serious — see the review-response comment.