fix(benches): 🐛 record placement for every benchmark, not just the random ones - #230
fix(benches): 🐛 record placement for every benchmark, not just the random ones#230diagonal-hamiltonian wants to merge 1 commit into
Conversation
…ndom ones The benchmark suite could not tell a fast run from an unpinned one, and several of its instruments were measuring the harness rather than the engine. PLACEMENT. `pinned_thread_summary()` was called from inside `OpMemory.stop()`. The fixed-model benchmarks never open a memory window, so they recorded no placement at all -- and an unpinned multi-rank run is not a slow run, it is a meaningless one: co-located ranks that collapse onto the same cores report timings that look like code. Placement is a property of the run, not of the memory window that happened to observe it, so it moves to `_record_placement` at module scope and is called from `record_model_stats` as well. It still has to be sampled while the engine's partition threads are ALIVE, which is why it stays inside an instrumentation hook rather than moving to configure time. min/max across ranks, because a PARTIAL failure -- some ranks placed, some not -- is the interesting case and the shape a batch-system cpuset confinement actually takes. TIMING. `barriered()` was timing its own entry barrier, charging each rank with the skew accumulated in the PRECEDING setup rather than with the operation. That was worth ~1.44x of a 2.86x observed spread on `build_graph`. The residual is still unexplained, so this does not make the wall a trustworthy instrument -- it makes it less untrustworthy. MEMORY. `PssSampler` reads /proc/self/smaps_rollup, whose cost is O(address space) and which takes mmap_lock; above a few GiB it perturbs the run it is measuring, and because it only samples when the GIL is released, the FASTER side of an A/B is sampled less and reports a lower peak. It is now opt-in behind --bench-pss-sampler. `HighWaterMark` is exact, needs no thread, and is what to quote. Also adds a per-operation memory window (`op_memory`) whose span is the timed call alone -- the autouse `record_memory` window includes `setup`, so for a benchmark with a setup it reports the construction transient, not the operation -- and a `test_random_propagate` benchmark. Adding a benchmark is safe for Bencher's history; nothing here renames one. AGENTS.md records the traps: pedantic's `setup=` builds round k+1 before releasing round k (so rounds > 1 doubles peak memory), pytest's fd-level capture hides C++ diagnostics without `-s`, and asserting an instrument's emission count at the point of collection is what stops "the arms measured the same" and "the instrument never fired" from being the same observation. Assisted-by: ClaudeCode:claude-opus-5
github-actions
Bot
added
the
documentation
|
Docs preview: https://pr-230.monoprop-docs.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #230 +/- ##
=======================================
Coverage 97.70% 97.70%
=======================================
Files 14 14
Lines 742 742
Branches 98 98
=======================================
Hits 725 725
Misses 12 12
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
|
🤖 AI text below 🤖 Closing this stack to rebuild it around a single-path design. Not abandoned — the work returns as fresh PRs once it is ready. Why. The stack accumulated runtime knobs as a way to A/B competing paths, and an A/B knob is not a shipping knob. Against
What the measurements decided. Several paths were being kept behind default-off knobs on the strength of stale or never-run comparisons:
What returns. The sparse query record together with the positions-based resolve (this PR's successor), the self-resolve simplification, and the layer profiler. Roughly 1,800 lines are deleted rather than added, and every PR in the new stack is a pure addition — the intermediate record format is never introduced, so nothing is added and then removed. Reopening as new PRs once the local work is verified. |
1 participant
🤖 AI text below 🤖
Summary
The benchmark suite could not tell a fast run from an unpinned one, and several of its instruments were measuring the harness rather than the engine. This fixes three of them. No engine code changes.
The one that matters most is placement.
pinned_thread_summary()was called from insideOpMemory.stop(), and the fixed-model benchmarks (bench_models.py) never open a memory window — so they recorded no placement at all. An unpinned multi-rank run is not a slow run, it is a meaningless one: co-located ranks collapsed onto the same cores produce timings that look like code changes. Placement is a property of the run, not of the memory window that happened to observe it, so it moves to module scope and is called fromrecord_model_statstoo. It still has to be sampled while the engine's partition threads are alive, which is why it stays inside an instrumentation hook rather than moving to configure time.Changes
_record_placement(comm)at module scope inbenches/conftest.py, called from bothOpMemory.stop()andrecord_model_stats. min/max across ranks, because a partial failure — some ranks placed, some not — is the interesting case and the shape a batch-system cpuset confinement actually takes.barriered()no longer times its own entry barrier. It was charging each rank with the skew accumulated in the preceding setup rather than with the operation — worth ~1.44× of a 2.86× observed spread onbuild_graph. The residual is still unexplained, so this makes the wall less untrustworthy, not trustworthy.PssSampleris now opt-in behind--bench-pss-sampler. It reads/proc/self/smaps_rollup, whose cost is O(address space) and which takesmmap_lock; above a few GiB it perturbs the run it is measuring, and because it only samples when the GIL is released, the faster side of an A/B is sampled less and reports a lower peak.HighWaterMarkis exact, needs no thread, and is what to quote.op_memory) spanning the timed call alone — the autouserecord_memorywindow includessetup, so for a benchmark with a setup it reports the construction transient rather than the operation.test_random_propagatebenchmark. Adding a benchmark is safe for Bencher's history key; nothing here renames or moves one.AGENTS.mdrecords the traps:pedantic'ssetup=builds round k+1 before releasing round k (sorounds > 1roughly doubles peak memory), pytest's fd-level capture hides C++ diagnostics without-s, and asserting an instrument's emission count at the point of collection is what stops "the two arms measured the same" and "the instrument never fired" from being the same observation.Verification
pytest benches/bench_random.pyandbenches/bench_models.pyboth run and now recordpinningon either path (checked at 2 threads and at 8 ranks × 16 partitions,16..16 of 19undersrun --cpu-bind=cores).prek run --from-ref origin/main --to-ref HEADclean.Checklist
docs/,CONTRIBUTING.md) if neededCHANGELOG/ release notes updated if applicable — release notes are generated from the PR titleAI/LLM disclosure