feat(memory): 📈 name the resident bytes the operator ledger cannot see - #293
Open
diagonal-hamiltonian wants to merge 9 commits into
Open
feat(memory): 📈 name the resident bytes the operator ledger cannot see#293diagonal-hamiltonian wants to merge 9 commits into
diagonal-hamiltonian wants to merge 9 commits into
Conversation
diagonal-hamiltonian
requested review from
fpietra,
ludmilaasb and
robertodr
as code owners
August 26, 2026 15:55
diagonal-hamiltonian
force-pushed
the
perf/memacct
branch
from
August 26, 2026 16:17
4ca8894 to
eecd898
Compare
|
Docs preview: https://pr-293.monoprop-docs.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #293 +/- ##
=======================================
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. |
diagonal-hamiltonian
marked this pull request as draft
August 26, 2026 17:01
At a fixed 1,569,152,761 terms the ledger's own total is flat from 1 to 32 nodes while
the kernel's peak RSS grows 2.15x, so coverage falls 91% -> 40% and the difference is
silent. Measured on Deucalion x86, hubbard c10 / atol 2.6e-06, layout B (8 ranks x 16
partitions per node), jobs 1851543 (N=1) and 1851544 (N=32), _core.so md5
3b2113539aee3ea59a0f0488f990a304.
The gap is three things, not one:
* 79.5 GiB of a 215 GiB job at N=32 is glibc holding freed chunks it never returned.
Those pages were faulted while the chunk was live and stay resident, so every large
transient the engine allocates and frees is charged to the process forever while
belonging to no structure a ledger can name. It is ~0.3 GiB per rank at every width,
flat across a 32x range in terms per rank, so at 256 ranks it reaches 54.4 B/term
against a ledger of 62.0.
* 9.9 GiB of total_bytes() at N=32 is reserved capacity nothing ever wrote, which the
kernel therefore never charges. At N=1 the ledger reads 94.2 GiB against 89.8 GiB
resident: it over-claims, and the old coverage figure hid that by comparing an
end-of-run capacity against a peak-over-time.
* 2.2 GiB is the in-process transport, which no field covered at all.
So the engine now reports:
* transport_bytes, a real field inside total_bytes(), filled once by the partitioned
facade from PartitionGroup::transport_memory_bytes(). A partition reports 0, so the
sum over S cannot multiply it. d_transport_staging_bytes breaks out the payload
funnel and stays outside total_bytes().
* reserved_bytes, rolling up the row-store, inverted-index and coefficient slacks.
Reserved and never written -- but NOT free, and the comment says so: job 1851566
measured peak RSS against that slack directly and found it tracks capacity, not used
bytes (flat to +/-0.073 GiB within a x1.5 branch, stepping -0.650 GiB at the capacity
boundary), because each growth holds the old fully-written buffer and the new one at
once. The corollary is that shrink_to_fit cannot recover it and must cost another
copy; only fewer or earlier growths help.
* d_terms_peak_bytes / d_indexing_peak_bytes, the growth duplicate at the two
monolithic reallocation sites. Capacity at end cannot represent a peak over time.
Both sites are cold paths, so no hot loop changes.
* detail::process_memory(), reading VmRSS/VmHWM and malloc_info(3), bound as d_proc_*.
Coverage is now derivable from one call instead of an external harness, and
d_proc_peak_rss_bytes agrees with `/usr/bin/time -v` to 100.0% at 8 ranks and 99.9%
at 256. It is zero-filled off Linux/glibc and cannot throw, and equally when something
replaces malloc: under ASan or TSan glibc's arenas stay empty, so the byte fields read
zero and the test asserts that absence is coherent rather than skipping the case.
The five identity terms sum to the measured peak exactly at both rungs. What remains is
45 GiB at N=32 that is 256 Python interpreters, mpi4py and MPI's own buffers, which the
engine cannot reach; it is now the named residue rather than an unexplained one.
Assisted-by: claude-code:claude-opus-5
diagonal-hamiltonian
force-pushed
the
perf/memacct
branch
from
August 26, 2026 17:27
eecd898 to
938c103
Compare
Signed-off-by: Aaron Miller <61472721+diagonal-hamiltonian@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends monoprop’s memory reporting so the operator “byte ledger” can be reconciled with observed peak RSS, by adding explicit fields for transport, reserved-but-unwritten capacity, peak-over-time growth duplication, and per-process kernel/allocator diagnostics.
Changes:
- Adds per-process diagnostics via
detail::process_memory()(VmRSS/VmHWM + glibcmalloc_info(3)), surfaced to Python asd_proc_*. - Expands
MPOperatorMemoryBreakdownwithtransport_bytes, reserved/slack metrics, and peak-over-time counters from the two main growth sites. - Wires group-owned transport memory into the partitioned facade and adds C++ tests validating the new accounting behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/monoprop/bindings/binder.h | Exposes new operator memory breakdown fields (including per-process diagnostics) to Python. |
| cpp/tests/mp_operator_tests.cpp | Adds unit tests covering new breakdown fields and process_memory() behavior. |
| cpp/monoprop/detail/ProcessMemory.h | Introduces ProcessMemory struct and the process_memory() API contract. |
| cpp/monoprop/detail/ProcessMemory.cpp | Implements /proc + glibc allocator parsing to populate per-process diagnostics. |
| cpp/monoprop/detail/partition/PartitionGroup.h | Adds group-owned transport memory reporting as {total, staging}. |
| cpp/monoprop/detail/operator/OperatorIndex.h | Adds peak-over-time tracking for row storage and index growth. |
| cpp/monoprop/detail/operator/MPOperator.h | Adds new breakdown fields and computes slack/reserved/peak values in estimate_memory_usage(). |
| cpp/monoprop/detail/operator/InvertedIndex.h | Adds inverted-index slack computation (reserved-but-unwritten capacity). |
| cpp/monoprop/detail/mpi/ShmComm.h | Adds transport memory reporting for the shared-memory comm path. |
| cpp/monoprop/detail/mpi/HybridComm.h | Adds transport memory/staging reporting for the hybrid MPI comm path. |
| cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl | Ensures group transport bytes are added once (not multiplied across partitions). |
| cpp/monoprop/detail/CMakeLists.txt | Adds the new ProcessMemory sources to the build. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ng field Copilot review: - ProcessMemory reads /proc via ifstream, so fopen's "e" mode flag is no longer a portability question that could silently zero VmRSS/VmHWM. - reserve_rows reads the growth duplicate off the POST-reserve capacity; reserve() may hand back more than asked. - Restores the insert_absent_terms contract 20bd33b truncated mid-sentence: currently-absent keys and per_slot-writes-before-bulk_insert are preconditions the code does not enforce. Restores the state_scored_rows_ comment, which the same commit replaced with a wrong one (it is a scored-row watermark, not a cache). Correctness of the peaks. rows_peak_bytes() omitted the overflow map while operator_terms_bytes counts it, so an operator with enough spilled rows reported a peak BELOW its resting bytes and a negative growth duplicate; index_peak_bytes() compared a table-only peak against a figure including sizeof(OperatorIndex). Both now carry their resting field's other terms, and both counters are stored in elements so the two accessors read the same way. Covered by a new test whose keys have popcount > the inline width -- grow_in_batches cannot reach the overflow path, its keys top out at popcount 9. /simplify: - capacity_bytes / capacity_slack_bytes move to detail/MemoryBytes.h, deducing the element size. Replaces HybridComm's private cap_bytes_, three hand-rolled copies in InvertedIndex::slack_bytes(), and the explicit `elem` argument at four sites. - reserved_bytes is derived from the three slacks it rolls up rather than stored, removing a fourth state to keep in sync and an operator+= line. - process_memory() holds its documented "never throws" in code: a noexcept wrapper round an inner reader, so a throw yields a zero-filled result rather than a partially-filled one. malloc_info's open_memstream buffer is now owned, so the string copy throwing cannot leak it. - Named constants for the malloc_info tags and raw string literals for the XML. Gated on Deucalion dev-x86 (job 1853765, _core.so ab0fc4055a03): ctest -L unit 247/247, -L serial 246/246, both including the new case. The prior revision passed the full 2-node sweep (job 1853743): 245 serial, 1/1 mpi, 593 Python x 10 layouts. Assisted-by: ClaudeCode:claude-opus-5
Removed comments explaining the transport_bytes variable. Signed-off-by: Aaron Miller <61472721+diagonal-hamiltonian@users.noreply.github.com>
Corrected grammatical error in comment regarding memory management. Signed-off-by: Aaron Miller <61472721+diagonal-hamiltonian@users.noreply.github.com>
Signed-off-by: Aaron Miller <61472721+diagonal-hamiltonian@users.noreply.github.com>
diagonal-hamiltonian
marked this pull request as ready for review
August 28, 2026 10:02
This reverts commit 88cfebd.
|
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
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
At a fixed 1,569,152,761 terms the operator ledger's total is flat from 1 to 32 nodes while the kernel's peak RSS grows 2.15×. Coverage falls 91% → 40%, and no field names the difference.
It is three things, not one:
With the fields below, the five identity terms sum to the measured peak exactly at both rungs. The remaining 45 GiB is 256 Python interpreters, mpi4py and MPI's own buffers — beyond the engine's reach, but now the named residue.
Reserved bytes are not free: peak RSS follows capacity, because each growth holds the old fully-written buffer and the new one at once. So
shrink_to_fitcannot recover them and must cost another copy — only fewer or earlier growths help.Changes
transport_bytes— insidetotal_bytes(), filled once by the partitioned facade. A partition reports 0, so the sum over S cannot multiply it.reserved_bytes()— derived, not stored: the row-store, inverted-index and coefficient slacks.d_terms_peak_bytes/d_indexing_peak_bytes— the growth duplicate at the two reallocation sites, since capacity-at-end cannot represent a peak over time. Each carries its resting field's other terms, so a peak can never read below the bytes it is a peak of. Both sites are cold; no hot loop changes.detail::process_memory()—VmRSS/VmHWMandmalloc_info(3), bound asd_proc_*, so coverage needs no external harness. Agrees with/usr/bin/time -vto 100.0% at 8 ranks and 99.9% at 256. Zero-filled off Linux/glibc, and cannot throw.detail/MemoryBytes.h—capacity_bytes/capacity_slack_bytes, shared by the operator store, the inverted index and both transports.Verification
Deucalion
dev-x86, commitc828609f,_core.somd5ab0fc4055a03…— jobs 1853830 (1 node) and 1853831 (2 nodes):ctest -L unitctest -L serialctest -L mpiMain's baseline is 241 unit / 240 serial; this adds six cases, all confirmed to actually run rather than be silently absent. The case covering the peak-over-time counters was mutation-tested: reverting just the overflow term makes it fail
4176 <= 10508, the negative growth duplicate it exists to catch.Caveats
_core.so3b2113539aee…. Later revisions changed the accounting code, not the quantities; they have not been re-measured since.-L unit/-L serialdo not reachPartitionGroup::transport_memory_bytes()through a real multi-partition group; the C++ cases test the breakdown arithmetic andprocess_memory()directly.Checklist
docs/,CONTRIBUTING.md) if neededCHANGELOG/ release notes updated if applicableAI/LLM disclosure