fix(evolution): 🐛 bound the epoch-stamp mark by combined_size - #267
Merged
Conversation
resolve_self_queries marked under `found[j] < op_size`, where op_size is 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, which is why nothing caught it. Guard on combined_size, matching the cross-rank twin resolve_incoming (Resolve.h:153). Latent, not live: the sole 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. 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. A mark at or above it is unreadable. The test constructs the out-of-bounds ordering rather than asserting the 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]. Verified by mutation -- reverting the guard fails the case deterministically.
|
Docs preview: https://pr-267.monoprop-docs.pages.dev |
diagonal-hamiltonian
marked this pull request as ready for review
August 24, 2026 08:09
diagonal-hamiltonian
requested review from
fpietra,
ludmilaasb and
robertodr
as code owners
August 24, 2026 08:09
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #267 +/- ##
=======================================
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. |
|
robertodr
approved these changes
Aug 24, 2026
6 tasks
diagonal-hamiltonian
added a commit
that referenced
this pull request
Aug 24, 2026
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.
diagonal-hamiltonian
added a commit
that referenced
this pull request
Aug 24, 2026
Textually clean, but main's #267 test arrived written against the dense query wire this branch removes. Two ports, both in `cpp/tests/evolution_detail_tests.cpp`: - `detail::query_push` no longer exists -- the record is width-adaptive -- so `self_resolve_mark_bounded_by_combined_size` pushes through `QueryCodec<8>::push` instead. - `RecordingSink` gains `querier_layout()`. `resolve_range_` reads it unconditionally now that `Sink::kStride` is gone, so without it the file does not compile; the assert on the query count reads it too, and under NDEBUG that half would have gone missing silently. #267's guard itself merged into the restructured `resolve_range_` loop as `found[j] < combined_size`, which is the same bound on the same index.
This was referenced Aug 24, 2026
robertodr
added a commit
that referenced
this pull request
Aug 24, 2026
🤖 _AI text below_ 🤖 # Halve the follower-marking epoch stamp (u32 → u16) Narrows `detail::MatchedEpochSet`'s per-term stamp to `uint16_t` — 2 B/term of operator row storage — with wrap handled explicitly and no shrink of the array anywhere. `perf/epoch-stamp-noshrink-v2` `1f350ff`: one commit of substance (`8dbe2a5`) plus merges of `main` and two review commits, 6 files +103/−11 against `origin/main`. 1. **The stamp width.** One stamp per term, for a counter that never leaves the struct — never serialised, never exchanged between ranks, never compared against anything but `cur_`. The wrap branch is now live (once per 65535 gate applications instead of once per 2^32) and the `std::fill` is what keeps it correct; `matched_epoch_stamp_wrap_reached_by_gate_count` cycles a whole period rather than assigning to `cur_`, which is the only way to prove the fill runs rather than that the branch is reachable. 2. **`matched_scratch_bytes` joins the memory breakdown, inside `total_bytes()`.** The stamp array is propagator-owned, so `estimate_memory_usage()` cannot see it and only `MonomialPropagator::operator_memory_usage()` can fill it in — the partitioned path sums per-partition breakdowns, so each picks up its own array. ## Memory: main `48cadcb` vs this branch, both `ENABLE_PROFILE=OFF` Kernel `/usr/bin/time -v` peak 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. | cell | main GiB | port GiB | port/main | agree | |---|---:|---:|---:|---:| | hubbard · A `1x128` · N=1 · `fresh` | 9.54 | 9.49 | 0.9948 | 10/10 | | hubbard · A `1x128` · N=2 · `fresh` | 10.33 | 10.02 | 0.9702 | 10/10 | | hubbard · B `8x16` · N=1 · `fresh` | 11.06 | 10.96 | 0.9911 | 10/10 | | hubbard · B `8x16` · N=2 · `fresh` | 14.18 | 13.92 | 0.9813 | 10/10 | | hubbard rung · A `1x128` · N=1 · `fresh` | 26.61 | 26.05 | 0.9788 | 10/10 | | hubbard rung · A `1x128` · N=2 · `fresh` | 31.23 | 30.46 | 0.9750 | 10/10 | | hubbard rung · B `8x16` · N=1 · `fresh` | 28.41 | 27.84 | 0.9797 | 10/10 | | hubbard rung · B `8x16` · N=2 · `fresh` | 34.83 | 34.04 | 0.9772 | 10/10 | | pauli · A `1x128` · N=1 · `fresh` | 10.39 | 10.31 | 0.9922 | 10/10 | | pauli · A `1x128` · N=1 · `graph` | 18.13 | 17.93 | 0.9890 | 10/10 | | pauli · A `1x128` · N=2 · `fresh` | 10.90 | 10.88 | 0.9987 | 10/10 | | pauli · A `1x128` · N=2 · `graph` | 36.70 | 36.53 | 0.9952 | 10/10 | | pauli · B `8x16` · N=1 · `fresh` | 12.17 | 12.03 | 0.9891 | 9/10 | | pauli · B `8x16` · N=1 · `graph` | 19.68 | 19.48 | 0.9902 | 10/10 | | pauli · B `8x16` · N=2 · `fresh` | 14.94 | 14.92 | 0.9969 | 8/10 | | pauli · B `8x16` · N=2 · `graph` | 40.78 | 40.57 | 0.9951 | 10/10 | **Peak 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 is `pauli-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 fresh` 1.0000, `pauli-B-N2 fresh` 1.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: hubbard `propagate` at 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 A `1x128`, 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 a `build_graph[hubbard]` rung at `--hubbard-trotter-steps=2` with **`--hubbard-lower-atol=1e-04`, not the main grid's `1.25e-05`**. | cell | operation | main ms | port ms | port/main | agree | |---|---|---:|---:|---:|---:| | hubbard · A `1x128` | `propagate[hubbard]` | 17624.0 | 17600.3 | 0.9950 | 7/10 | | hubbard · B `8x16` | `propagate[hubbard]` | 19194.4 | 19150.7 | 0.9974 | 6/10 | | hubbard rung · A `1x128` | `build_graph[hubbard]` | 3976.1 | 3917.2 | 0.9928 | 6/10 | | hubbard rung · B `8x16` | `build_graph[hubbard]` | 3093.7 | 3089.3 | 0.9988 | 6/10 | | pauli · A `1x128` | `build_graph[pauli]` | 13947.9 | 14100.9 | 1.0065 | 9/10 | | pauli · A `1x128` | `propagate[pauli]` | 11852.8 | 11755.8 | 0.9937 | 5/10 | | pauli · A `1x128` | `energy[pauli]` | 2473.0 | 2418.7 | 0.9867 | 6/10 | | pauli · A `1x128` | `gradient[pauli]` | 11355.2 | 11172.4 | **0.9836** | **10/10** | | pauli · B `8x16` | `build_graph[pauli]` | 13300.9 | 13270.3 | 0.9984 | 8/10 | | pauli · B `8x16` | `propagate[pauli]` | 10585.8 | 10533.2 | 0.9962 | 8/10 | | pauli · B `8x16` | `energy[pauli]` | 2860.2 | 2860.8 | 1.0046 | 7/10 | | pauli · B `8x16` | `gradient[pauli]` | 11583.9 | 11549.9 | 0.9988 | 7/10 | ## Caveats and scope - **`total_bytes()` is not comparable across this commit**: a build without `matched_scratch_bytes` reports 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. - **Not reproducible from this diff**: Deucalion, 2× AMD EPYC 7742 / 128 cores / SMT off / NPS4 / 242 GiB per node; a private benchmark harness that does not ship; two prebuilt venvs (main `1250a27e…`, port `6f14ecb1…`). - **The measured binaries carry one mechanism this branch's own diff does not — and `main` now carries it too.** They were built from a cut that also released `init_op_map` in `get_operator()`. That mechanism shipped separately as #268 and is in this branch's base as of the merge below, so `get_operator()` here is `main`'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 md5 `4a32d5c3…`; the two commits since are a doc line and a code comment, neither reaching the binary, `ENABLE_PROFILE=OFF`): `ctest -L unit` **224/224** and `-L serial` **223/223**, the four Python MPI layouts 1×1, 1×16, 2×8 and 8×16 at **592 passed each**, and `clang-format --output-replacements-xml` 0 replacements on every changed C++/binding file. `main` has moved under this branch since the `48cadcb` measurement, and two of those commits are C++ rather than docs and build config: #267 bounds the epoch-stamp mark by `combined_size`, and #268 is the `init_op_map` release named in the caveat above. Neither is in the measured `main` arm. #268 is the 1,189 B already bounded there; #267 adds one compare per self-resolve hit and allocates nothing. The figures stand. Merging `main` took three conflicts, all additive. Main modernised `MPOperatorMemoryBreakdown`'s member initialisers to `{0uz}`, so `matched_scratch_bytes` joins in that form; and each side had appended a test case at the same point in `evolution_detail_tests.cpp` and in `mp_operator_tests.cpp`, so both survive in each file. The wrap test and #267's guard test both pass on the merged build. --------- Signed-off-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com>
robertodr
pushed a commit
that referenced
this pull request
Aug 24, 2026
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
diagonal-hamiltonian
added a commit
that referenced
this pull request
Aug 24, 2026
#267 landed on main while #263 was open, and its self_resolve_mark_bounded_by_combined_size fixture feeds the engine through detail::query_push -- the dense record #263 retires. The rebase onto that main left the call in place, so origin/pr/query-wire-v3 does not compile: query_push now lives in cpp/tests/dense_query_reference.h as test_ref::query_push, an oracle for the codec differential and not an engine entry point. Ported rather than renamed. Under the positions-staged self leg the fixture would have tripped resolve_self_queries' own assertion -- "a self-owned query was encoded instead of staged" -- because at my_rank == 0 the engine reads self_stage_ and requires queries_r[my_rank] empty. It now pushes each term's ascending positions onto self_stage_, which is what the scan does, and every assertion the case made about marking past combined_size is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖
Summary
Bounds the epoch-stamp mark in
resolve_self_queriesbycombined_sizeso it can no longer writepast the end of
epoch_.fix/epoch-stamp-bounds-guard92e352b, 1 commit one0c528e, 2 files+69/−2.
resolve_self_queriesmarks underfound[j] < op_size— the store size read atresolve_range_entry — butepoch_is sized tocombined_size, the store size at engineconstruction. Once the store has grown,
op_size > combined_sizeand the mark writes past theend of
epoch_, into vector slack. Silently. This matches what the cross-rank twinresolve_incoming(Resolve.h:153) already does.is_leader_pass, andrun_exchange(true, …)resolves self queries before both insert paths, so today the two sizes coincide. Reordering the
passes breaks it. Pre-dates perf(evolution): ⚡ halve the follower-marking epoch stamp #259 and is kept out of it deliberately.
is_markedreaders(
:406,:482) read scan-source indices out offused, built at:562-565before the engineis constructed at
:592-598— so every index reachingis_markedis< combined_size, and amark at or above it is unreadable.
begin_gate(6)thencombined_size=4, so query 2 resolves tofound=5againstop_size=6andthe unfixed code writes
epoch_[5]. Mutation-verified — reverting the guard fails the casedeterministically.
Changes
resolve_self_queriesbycombined_size, matchingresolve_incoming's existing guard.is_leader_pass, never presentsop_size > combined_size; the guard protects against reordering the passes, not against a livebug.
begin_gate(6),combined_size=4) instead of relying on today's coincidental sizing.Measurement
No campaign and no table here: the only marking pass is
is_leader_pass, once per pass, so oneadded comparison on that path cannot move a timing. Gates:
ctest -L unit217/217,-L serial216/216.
Relationship to the open PRs
Independent of #259 and #263 — no stacking needed. Pre-dates #259 and is deliberately kept out of it
so the stamp-narrowing story there stays one mechanism. Verified per-file against both heads: one
conflict hunk against each, in
cpp/tests/evolution_detail_tests.cpponly, from adjacent testinsertions.
Engine.hitself merges clean against #263 even though that PR edits the same file —different regions.
Checklist
docs/,CONTRIBUTING.md) if neededCHANGELOG/ release notes updated if applicableAI/LLM disclosure
Important
By opening this PR I confirm that I have read CONTRIBUTING.md and I agree to the terms of the Contributor License Agreement.
Warning
If you're contributing on behalf of your employer, contact cla@algorithmiq.fi to arrange a Corporate CLA.