perf(evolution): ⚡ halve the follower-marking epoch stamp - #259
Conversation
|
Docs preview: https://pr-259.monoprop-docs.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #259 +/- ##
=======================================
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. |
f1f3067 to
b01e6fa
Compare
b01e6fa to
2be9966
Compare
`detail::MatchedEpochSet` carried a `uint32_t` stamp per term for a counter that never leaves the struct -- never serialised, never exchanged, only ever compared to `cur_`. This narrows it to `uint16_t`, keeps the wrap discipline explicit, and does NOT shrink the array anywhere. Two mechanisms: - the stamp width, with the wrap branch now live (once per 65535 gates) and the `std::fill` the thing that keeps it correct -- `epoch_` retains stale stamps for row indices reused after a truncation, so deleting the fill aliases marks; - `matched_scratch_bytes` joins `MPOperatorMemoryBreakdown` inside `total_bytes()`, with the stamp array being propagator-owned, so only `MonomialPropagator::operator_memory_usage()` can fill it in. The partitioned path sums per-partition breakdowns, so each picks up its own array. NO `shrink_to_fit()` ANYWHERE, not even an uncalled method. An earlier revision called `matched_scratch_.shrink_to_fit()` from `initialize_operator_caches_()` on the premise that the array "grows only when the term count does". That is false on Hubbard: the function runs after EVERY `build_graph()` and `propagate()`, so 29 Trotter steps reach it 29 times against a term count that grows at each step. Its A/B read +0.2821 GiB peak RSS (+3.12 B/term), 6/6, non-overlapping, at the 1x16 hubbard cell; removing it read -0.24 GiB, 6/6. And the shrink cannot free resident bytes at all -- `resize(n, 0)` never writes past `n`, so the capacity it releases was never faulted in. A retained-but-uncalled method plus a comment asking the next reader not to call it is a trap; the rejected experiment lives in the PR body instead. Re-cut twice and rebased onto main: the original was authored on a base carrying the profiling instrument, then briefly on the graph-memory PR whose key-set test is not on main. The mechanism never changed across any of it. It contains no profiling code: monoprop_PROF, profile:: and Profile.h appear zero times in the diff. MEASURED, campaign pr7v2off, both arms ENABLE_PROFILE=OFF, 12 cells x 10 interleaved reps in one allocation with order flipped per cell: kernel peak RSS (`/usr/bin/time -v`, node-sum) falls in 16 of 16 memory cells, 14 at unanimous 10/10, 15 of 16 clearing Holm across the memory family at largest adjusted p = 0.043. Hubbard `propagate` 0.97-0.99x, the hubbard `build_graph` rung 0.975-0.980x, pauli 0.989-0.999x -- hubbard beating pauli is what a per-term saving must do. Timing is a null: 1 of 24 tests resolved (`gradient[pauli]`, A 1x128, N=1, 0.9836x, 10/10, adjusted p = 0.0469, on an operation this diff cannot reach), the other 23 inside 0.964-1.014x. 2 B/term is a floor on the ARRAY's saving, not on the peak-RSS delta: peak RSS is a maximum over time and the stamp array need not be at its own maximum then. Hubbard `propagate` at N=1 implies 0.54 and 1.05 B/term of node-sum delta, below the array's own floor, which says something about WHEN the peak lands and nothing about the width. SPLIT: an earlier cut of this branch also released `init_op_map` in `get_operator()`. That is now a separate change so this PR carries one story, which means the measured binaries contain a release this branch does not. It was worth 1,189 B (8357 -> 7168 B in this campaign's own ledger) against cells of 9.5-40 GiB -- order 1e-7, far below the smallest resolved effect -- so the figures stand. Re-gated, not re-measured. An earlier instrumented pair (both arms ENABLE_PROFILE=ON) agrees on the `build_graph` rung and on pauli but read hubbard `propagate` at 0.96-0.97x -- it overstated exactly the number being advertised. The uninstrumented pair above is what certifies the shipping code, and is what is quoted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2be9966 to
8dbe2a5
Compare
|
🤖 AI text below 🤖 Measured at Full timing breakdown — all 24 tests, OFF/OFFCampaign
Holm across all 24 leaves one survivor, Worst-rank peak RSS, for the OOM questionNode-sum is the only cross-layout figure and is what the body tables; worst-rank is what an OOM is decided by. At layout A the two are equal (1 rank/node). The direction holds on worst-rank everywhere except two pauli cells, which reverse: |
🤖 _AI text below_ 🤖 ## Summary Bounds the epoch-stamp mark in `resolve_self_queries` by `combined_size` so it can no longer write past the end of `epoch_`. `fix/epoch-stamp-bounds-guard` `92e352b`, 1 commit on `e0c528e`, 2 files +69/−2. 1. **The guard.** `resolve_self_queries` marks under `found[j] < op_size` — the store size read at `resolve_range_` entry — but `epoch_` is sized to `combined_size`, the store size at engine construction. Once the store has grown, `op_size > combined_size` and the mark writes past the end of `epoch_`, into vector slack. Silently. This matches what the cross-rank twin `resolve_incoming` (`Resolve.h:153`) already does. 2. **Latent, not live.** The only marking pass is `is_leader_pass`, and `run_exchange(true, …)` resolves self queries before both insert paths, so today the two sizes coincide. Reordering the passes breaks it. Pre-dates #259 and is kept out of it deliberately. 3. **Dropping such a mark is result-neutral, not merely in-bounds.** Both `is_marked` readers (`:406`, `:482`) read scan-source indices out of `fused`, built at `:562-565` *before* the engine is constructed at `:592-598` — so every index reaching `is_marked` is `< combined_size`, and a mark at or above it is unreadable. 4. **The test constructs the out-of-bounds ordering rather than asserting today's accident.** `begin_gate(6)` then `combined_size=4`, so query 2 resolves to `found=5` against `op_size=6` and the unfixed code writes `epoch_[5]`. Mutation-verified — reverting the guard fails the case deterministically. ## Changes - Bound the epoch-stamp mark in `resolve_self_queries` by `combined_size`, matching `resolve_incoming`'s existing guard. - No behavior change today: the only marking pass, `is_leader_pass`, never presents `op_size > combined_size`; the guard protects against reordering the passes, not against a live bug. - Add a regression test that constructs the out-of-bounds ordering directly (`begin_gate(6)`, `combined_size=4`) instead of relying on today's coincidental sizing. ## Checklist - [x] Tests added or updated to cover the changes - [ ] Documentation updated (docstrings, `docs/`, `CONTRIBUTING.md`) if needed - [ ] `CHANGELOG` / release notes updated if applicable ## AI/LLM disclosure - [ ] I did not use LLM tooling, or used it only privately for ideation - [x] I used the following tool to help write this PR description: Claude Code - [x] I used the following tool to generate or modify code: Claude Code > [!IMPORTANT] > By opening this PR I confirm that I have read [CONTRIBUTING.md](https://github.com/Algorithmiq/monoprop/blob/fa820c3e5a90f773417c367fedbf2bd50683b496/CONTRIBUTING.md) and I agree to the terms of the [Contributor License Agreement](https://github.com/Algorithmiq/monoprop/blob/fa820c3e5a90f773417c367fedbf2bd50683b496/CLA.md). > [!WARNING] > If you're contributing on behalf of your employer, contact [cla@algorithmiq.fi](mailto:cla@algorithmiq.fi) to arrange a Corporate CLA. Co-authored-by: Ludmila Botelho <39570941+ludmilaasb@users.noreply.github.com>
Three conflicts, all additive:
- `MPOperatorMemoryBreakdown`: main modernised the member initialisers to `{0uz}` (#268); keep
that form and add `matched_scratch_bytes{0uz}` in it.
- `evolution_detail_tests.cpp`: both sides appended a case at the same point --
`matched_epoch_stamp_wrap_reached_by_gate_count` (this branch) and
`self_resolve_mark_bounded_by_combined_size` (#267). Keep both.
- `mp_operator_tests.cpp`: same shape -- the two `matched_scratch_bytes` cases here and
`mp_operator_breakdown_keeps_init_operator_entries_out_of_total` from main. Keep both.
#268 landed the `init_op_map` release the measured binaries also carried, so `get_operator()` is
main's again with that mechanism now in the base rather than split out. #267's `combined_size`
bound is orthogonal to the stamp width: it constrains the index marked, not the stamp stored.
| // matched_scratch_ is propagator-owned, so estimate_memory_usage() cannot see it; on a facade the | ||
| // partitioned path above already picks it up per partition. | ||
| auto breakdown = detail::estimate_memory_usage(mp_op_); | ||
| breakdown.matched_scratch_bytes = matched_scratch_.memory_bytes(); | ||
| return breakdown; |
There was a problem hiding this comment.
I really don't understand what is happening here?
There was a problem hiding this comment.
🤖 AI text below 🤖
Fair — it was unexplained, and it looks like the line above it is being overwritten. Comment rewritten in 1f350ff.
What it is: matched_scratch_ is a member of this class, not of the operator. detail::estimate_memory_usage(mp_op_) takes the operator, so it has no way to see the stamp array and returns matched_scratch_bytes == 0; this level is the only one that holds both the number and the struct to put it in, so it fills the field in after the fact.
Why the field lives in the operator's breakdown struct at all, rather than being returned separately: the array is one stamp per operator row, so it belongs with the other per-term figures, and total_bytes() has to include it or the total under-reports resident memory. The partitioned branch two lines up needs no such patch — partitioned_operator_memory_usage_() sums each partition's own breakdown, and each partition is a propagator with its own array, so each summand already carries its own bytes.
The two tests pinning it are mp_operator_breakdown_counts_matched_scratch_in_total_and_sum (the field is summed by total_bytes() and accumulated by operator+=, and an operator on its own reports 0) and mp_operator_breakdown_matched_scratch_nonzero_after_a_gate (0 before any gate, nonzero after one, and the delta in total_bytes() equals the field).
Signed-off-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com>
robertodr
left a comment
There was a problem hiding this comment.
I just don't understand one bit, but otherwise seems harmless. It's good to be thrifty with type widths
Review comment on #259: the assignment in `operator_memory_usage()` was unexplained. It reads as an after-the-fact patch of a field the line above it just computed, which is what it is, and the reason is ownership -- `matched_scratch_` is a member of the propagator while `estimate_memory_usage()` takes the operator, so the field comes back 0 and only this level knows the number.
|
|
Are you satisfied |
Main's side of every C++ conflict was the pre-NTTP-removal code, so its changes were ported onto this branch's runtime-width structure rather than either side being taken whole. Main's three functional imports are preserved: - the `found[j] < combined_size` guard on the self-resolve mark (#267), which auto-merged into this branch's `with_store`-bound engine; - the `init_op_map` bucket release (#268). Its `rehash(0)` supersedes this branch's `init_op_map = MonomialMap{}`, so this branch's partial-drain test relaxes from `==` to `<=` on the bucket count -- main's version shrinks on any erase, where this branch's released only on a full drain; - `matched_scratch_bytes` (#259), beside this branch's `inverted_index_columns_bytes`. Main's nanobind 3 split mode (`BACKEND_MODULE nanobind_backend`, hence no `STABLE_ABI`/`NB_STATIC`) applies to this branch's real `bindings.cpp`. With `bindings.cpp.in` deleted there is nowhere to substitute `@nanobind_VERSION@`, so `__nanobind_version__` -- which `src/monoprop/__init__.py` imports -- now arrives as a `monoprop_NANOBIND_VERSION` compile definition from CMake, and pyproject's build cache-keys point at `bindings/*.cpp` instead of the two gone template files. Main's repo-wide `class` -> `typename` style (#240) is applied to the 15 branch-only template sites the merge left behind, leaving the same single `template <class>` main keeps in MPICompat.h. Verified byte-identical to the pre-merge tip d37c408 (`capture-baseline`, 38 records over 10 cases), so the merge moves no term and no energy. `.baseline-capture/golden` predates 9b43667 and d37c408 and differs from both in `majorana_lattice_layer_30` by term order alone; it needs a refresh. ctest 590/590 (295 under the sparse-rows label), pytest 620 passed, sparse-row pytest 583 passed, sparse-vs-dense baselines agree to rtol 1e-10. Assisted-by: ClaudeCode:claude-opus-5



🤖 AI text below 🤖
Halve the follower-marking epoch stamp (u32 → u16)
Narrows
detail::MatchedEpochSet's per-term stamp touint16_t— 2 B/term of operator row storage — with wrap handled explicitly and no shrink of the array anywhere.perf/epoch-stamp-noshrink-v21f350ff: one commit of substance (8dbe2a5) plus merges ofmainand two review commits, 6 files +103/−11 againstorigin/main.cur_. The wrap branch is now live (once per 65535 gate applications instead of once per 2^32) and thestd::fillis what keeps it correct;matched_epoch_stamp_wrap_reached_by_gate_countcycles a whole period rather than assigning tocur_, which is the only way to prove the fill runs rather than that the branch is reachable.matched_scratch_bytesjoins the memory breakdown, insidetotal_bytes(). The stamp array is propagator-owned, soestimate_memory_usage()cannot see it and onlyMonomialPropagator::operator_memory_usage()can fill it in — the partitioned path sums per-partition breakdowns, so each picks up its own array.Memory: main
48cadcbvs this branch, bothENABLE_PROFILE=OFFKernel
/usr/bin/time -vpeak RSS, summed over the ranks on the node, produced outside the code under test. 16 cells × 10 interleaved reps in one allocation, order flipped per (rep, cell); paired per-rep ratios, median of ratios;agree= reps pointing the same way.1x128· N=1 ·fresh1x128· N=2 ·fresh8x16· N=1 ·fresh8x16· N=2 ·fresh1x128· N=1 ·fresh1x128· N=2 ·fresh8x16· N=1 ·fresh8x16· N=2 ·fresh1x128· N=1 ·fresh1x128· N=1 ·graph1x128· N=2 ·fresh1x128· N=2 ·graph8x16· N=1 ·fresh8x16· N=1 ·graph8x16· N=2 ·fresh8x16· N=2 ·graphPeak RSS falls in 16 of 16 cells, 14 at unanimous 10/10, and 15 of 16 clear Holm across the memory family — largest adjusted p = 0.043 (
pauli-B-N1 fresh, 9/10); the only failure ispauli-B-N2 fresh(8/10), the smallest effect present. That unanimity is node-sum only: on worst-rank RSS two cells reverse direction (pauli-A-N2 fresh1.0000,pauli-B-N2 fresh1.0027). Hubbard beating pauli is what a per-term saving must do — but 2 B/term is a floor on the array's saving, not on the peak-RSS delta, because peak RSS is a maximum over time and the stamp array need not be at its own maximum at that instant: hubbardpropagateat N=1 implies only 0.54 and 1.05 B/term of node-sum delta, below the array's own floor, which is a statement about when the peak lands and not about the width.Timing: a null
1 of 24 tests resolved; the other 23 span 0.964–1.014x. The one survivor is
gradient[pauli], layout A1x128, N=1: 11355.2 → 11172.4 ms, 0.9836x, 10/10, Holm-adjusted p = 0.0469 — exactly at the boundary, and on an operation that reaches only the allreduce and never touches a table this diff changes, so it reads as a null either way. Absolute medians at N=1 below; the N=2 half and the full 24-row breakdown will follow in a comment. Grid: layout A = 1 rank/node × 128 partitions, B = 8 × 16, N = 1 and 2 nodes; hubbard--hubbard-cutoff=10 --hubbard-lower-atol=1.25e-05(96,981,051 terms), pauli--pauli-cutoff=14 --pauli-lower-atol=5e-05(91,273,861 terms), plus abuild_graph[hubbard]rung at--hubbard-trotter-steps=2with--hubbard-lower-atol=1e-04, not the main grid's1.25e-05.1x128propagate[hubbard]8x16propagate[hubbard]1x128build_graph[hubbard]8x16build_graph[hubbard]1x128build_graph[pauli]1x128propagate[pauli]1x128energy[pauli]1x128gradient[pauli]8x16build_graph[pauli]8x16propagate[pauli]8x16energy[pauli]8x16gradient[pauli]Caveats and scope
total_bytes()is not comparable across this commit: a build withoutmatched_scratch_bytesreports a total lower by roughly the stamp array while holding the same or more resident memory, so subtract the field or re-measure the baseline.1250a27e…, port6f14ecb1…).mainnow carries it too. They were built from a cut that also releasedinit_op_mapinget_operator(). That mechanism shipped separately as perf(operator): ⚡ release the init_op_map buckets once its terms bind #268 and is in this branch's base as of the merge below, soget_operator()here ismain's in both arms. Either way it is worth 1,189 B (8357 → 7168 B in this campaign's own ledger) against cells of 9.5–40 GiB — order 1e-7, far below the smallest resolved effect. Re-gated, not re-measured.Gates
Re-gated on the merged tree (
09c5e67, extension md54a32d5c3…; the two commits since are a doc line and a code comment, neither reaching the binary,ENABLE_PROFILE=OFF):ctest -L unit224/224 and-L serial223/223, the four Python MPI layouts 1×1, 1×16, 2×8 and 8×16 at 592 passed each, andclang-format --output-replacements-xml0 replacements on every changed C++/binding file.mainhas moved under this branch since the48cadcbmeasurement, and two of those commits are C++ rather than docs and build config: #267 bounds the epoch-stamp mark bycombined_size, and #268 is theinit_op_maprelease named in the caveat above. Neither is in the measuredmainarm. #268 is the 1,189 B already bounded there; #267 adds one compare per self-resolve hit and allocates nothing. The figures stand.Merging
maintook three conflicts, all additive. Main modernisedMPOperatorMemoryBreakdown's member initialisers to{0uz}, somatched_scratch_bytesjoins in that form; and each side had appended a test case at the same point inevolution_detail_tests.cppand inmp_operator_tests.cpp, so both survive in each file. The wrap test and #267's guard test both pass on the merged build.