perf(operator): ⚡ release the init_op_map buckets once its terms bind - #268
Conversation
get_operator() bound pending init_op_map terms, collected the bound monomials into a temporary vector, then erased them in a second loop -- one extra hash lookup per bound entry. erase_if erases in place during the single store->find() pass, dropping both. The release is the point. erase/clear keep bucket_count() on unordered_flat_map, and init_operator_bytes reports exactly that, so a fully drained map keeps holding and reporting its whole allocation. rehash(0) on an emptied map releases to bucket_count() == 0. Note this does NOT claim "one find pass instead of two" against main: main already does one find pass plus one erase(key) per bound entry. That claim was true only of an earlier branch whose partial arm re-ran find over every entry. init_operator_entries (Python d_init_operator_entries) exposes the live entries behind init_operator_bytes so a reader can tell held bytes from dead buckets. Outside total_bytes(), like every other d_ diagnostic. The fully-bound test pins bucket_count() == 0; mutation-verified -- removing rehash(0) fails it.
|
Docs preview: https://pr-268.monoprop-docs.pages.dev |
There was a problem hiding this comment.
Pull request overview
This PR optimizes detail::MPOperator::get_operator() by erasing bound init_op_map entries in-place via erase_if and then explicitly releasing the underlying bucket allocation with rehash(0) when the map was drained/changed, ensuring init_operator_bytes reflects freed capacity rather than lingering buckets.
Changes:
- Replace the prior “collect keys then erase” approach in
get_operator()with a single-passerase_ifand conditionalrehash(0)to release drained buckets. - Add new memory diagnostic
init_operator_entries(exposed to Python asd_init_operator_entries) to report live entries separately from bucket-backed byte estimates. - Add targeted C++ unit tests covering fully-bound, partially-bound, and nothing-bound cases, plus aggregation semantics for the new diagnostic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
cpp/monoprop/detail/operator/MPOperator.h |
Implements in-place erasure + conditional rehash(0) in get_operator(), and adds/records init_operator_entries in the memory breakdown. |
cpp/tests/mp_operator_tests.cpp |
Adds white-box tests validating bucket release/shrink behavior and that init_operator_entries stays out of total_bytes(). |
src/monoprop/bindings/binder.h |
Exposes init_operator_entries as a diagnostic entry in operator_memory_breakdown for Python consumers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #268 +/- ##
=======================================
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. |
std::ranges::iota is not available on the appleclang shipped with the macos-15 runner
|
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.
🤖 _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>
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 🤖
Summary
get_operator()now erases boundinit_op_mapentries in place during its singlestore->find()pass via
erase_if, then releases the drained buckets withrehash(0)— 1,148,190,448 B → 896 Bof retained buckets at the one point where the map has ever been measured large.
erase/clearkeepbucket_count()onunordered_flat_map, andinit_operator_bytesreports exactly that — so a fully drained map keeps holding, and reporting,its whole bucket array.
rehash(0)on an emptied map releases tobucket_count() == 0. It iscalled only when the pass actually erased something.
maincollects bound keys into astd::vector<Monomial>and then re-hashes each one throughinit_op_map.erase(mono);erase_ifremoves both the vector and that second hash per bound entry.
init_operator_entries/d_init_operator_entriesexposes the live entries behindinit_operator_bytes, outsidetotal_bytes()like every otherd_diagnostic. Without it,bytes and entries are indistinguishable and dead buckets read as live storage.
(
cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl:155-157), so its size isobs_terms / ranks. A 1-rank layout concentrates the whole map in one rank; wide layouts divideit.
Adversarial review. An independent review traced the form into pinned Boost 1.88 —
erase_ifisan ADL-only hidden friend; foa snapshots each group's occupied mask before walking it and
erasenever reallocates;
rehash(0)reaches 0 buckets because theif (n)guard skipscapacity_for's~29-slot minimum — and did not break it.
Changes
get_operator():erase_ifbinds and erases in one pass, thenrehash(0)on a map whose sizechanged. The deferred-erase key vector is gone.
MPOperatorMemoryBreakdown::init_operator_entries, summed inoperator+=, excluded fromtotal_bytes(), filled frominit_op_map.size()inestimate_memory_usage.d_init_operator_entriesadded to the Python memory-breakdown dict inbinder.h.mp_operator_tests.cppcases: fully bound, partially bound, nothing bound, and one pinninginit_operator_entriesout oftotal_bytes().Measurement
The measurement is a single standalone operator-memory ledger reading on the random-Heisenberg
workload at a 7M-term observable, recorded in the round-3 memory results.
init_op_mapheld1,148,190,448 bytes with
d_init_operator_entries= 0 — a fully drained map still holding itsentire bucket array, because
erasenever shrinksbucket_count()— and 896 bytes after therelease. The same drain also removed a transient
std::vector<Monomial>of keys worth 448 MB ofpeak across 16 partitions.
init_operator_bytes, held for the propagator's lifetimed_init_operator_entriescannot prove the map grew, and should not be read that way.initialize_operator_caches_()runs at construction(
MonomialPropagator.inl:187, callingget_operator()at:500), when every entry binds — so thecounter reads 0 on both arms at every recording point. It proves the drain, not the growth. What
differs between arms is
init_operator_bytes: ~1.15 GB held for the propagator's whole lifetime onmain, ~896 B here.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.