perf(graph,mpi): ⚡ derive the exchange layouts and store only occupied world slots - #270
Merged
Conversation
github-actions
Bot
added
documentation
|
Docs preview: https://pr-270.monoprop-docs.pages.dev |
diagonal-hamiltonian
force-pushed
the
perf/sparse-slots-and-funnel
branch
from
August 23, 2026 23:52
13eb4b2 to
cb3f4d3
Compare
diagonal-hamiltonian
changed the title
diagonal-hamiltonian
force-pushed
the
perf/sparse-slots-and-funnel
branch
from
August 23, 2026 23:54
cb3f4d3 to
66b791b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #270 +/- ##
=======================================
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 ready for review
August 24, 2026 07:44
diagonal-hamiltonian
requested review from
adamglos92,
fpietra,
ludmilaasb and
robertodr
as code owners
August 24, 2026 07:44
diagonal-hamiltonian
force-pushed
the
perf/sparse-slots-and-funnel
branch
from
August 24, 2026 08:01
6a1ada9 to
e25bbfb
Compare
Inside the engine `rank_count` is `mpi::size(comm)`, and on a partitioned run the comm is Hybrid, whose size() is the FLAT world P = ranks x partitions. Every per-rank array in a layer is therefore P long, each MPI rank holds one per partition, and the graph retains one per layer -- so a per-slot record costs O(P^2) across the job. Measured on pauli c14 at 91,273,861 terms, the graph goes 3.81 GB at P=16 to 61.94 GB at P=512 while the operator stays flat near 6.5 GB. Fitting graph = a + b*P^2 on each adjacent pair gives b = 235,709 / 223,891 / 220,345 B/P^2 -- three independent pairs agreeing to 7%, the upper two to 1.6%. CrossRankPartnerRange carried an offset and a count for each of B and D. They were always equal: GraphSink::finalize resizes both vectors from the same P + Q, so the counts match per slot and their prefix sums match with them. B and D are the two endpoints of the same rotation set. Keeping one pair drops the record from 32 to 16 bytes with no padding either way, pinned by a static_assert. The equality is now a checked precondition rather than a comment. Unchecked, a skew would not throw: cross_rank_sin_recv_index would mis-derive Q and read a wrong-but-valid endpoint, and Evolution's self-slot snapshot would run off the end of a B-sized buffer. Three consumers already bet on it silently. Also adds graph_memory_breakdown(). The operator partitions and the graph does not, and one total could not say which. It splits the fields, reports the slot occupancy that decides whether a sparse layout would pay, and counts two things total_bytes() never has: the resolve_recv transpose cache and the lazily retained derivative layout. Those stay as diagnostics rather than joining total_bytes, so graph_memory_bytes() means the same thing before and after and an A/B against an older build still compares one quantity. Assisted-by: ClaudeCode:claude-opus-5
A graph layer retained two `int[P]` arrays for the evolution exchange and, after the first gradient, two more for the derivative round. P is the FLAT WORLD SIZE (ranks x partitions), each MPI process holds one set per partition, and the graph holds one per layer -- so those arrays cost O(P^2) across a job for content that is a prefix sum of what the slot records already say. `counts[r]` is `(r == my_rank ? 0 : cross_rank.sin_send_size(r))` and `displs` is its running prefix. Both are now derived into the per-thread scratch that already owns the send and recv buffers, for the transfer being posted. That takes the retained slot-proportional footprint from 32 B/slot to 16 B/slot, and from 48 to 16 once a gradient has run: only the slot records survive. The derivative round needs no collective of its own. Its counts are the evolution counts at a hardcoded scale of 2, applied identically on every rank, and displacements are prefix sums of counts -- so scaling commutes with the transpose and the derivative recv layout is 2x the evolution recv layout. One `resolve_recv` per layer per evaluation now serves both rounds. The transpose cache stays retained (8 B/slot). It is the one piece that cannot be derived locally, and dropping it would cost an MPI_Alltoall per layer per evaluation. Sharing scratch across layers is only sound once `resolve_recv` can tell one send pattern from another. Its predicate was `comm_size == comm_size && counts.size() == n` -- effectively "have we ever resolved anything for a communicator this size", which is true for every layer after the first. Correct only while each cache belonged to the one layout that produced it; silently wrong the moment two patterns share a cache. The fix is NOT a checksum of the counts. A miss runs `alltoall_counts`, a collective, so two ranks disagreeing about validity is a distributed HANG rather than a wrong answer, and any rank-local key can collide on one rank and not on another. The cache now carries a `generation` assigned per LayerCore at build time. Build order is identical on every rank, so every rank misses on a layer's first resolve and hits afterwards -- the DECISION is uniform even though the id values are not. A `LayerCore` copy made by `set_parameter_mapping` now inherits that cache rather than dropping it. Relabelling changes which parameter drives the rotation, never which endpoints cross to which slot, so the cached transpose is still correct; clearing it would have cost one collective per layer (5,420 at the anchor) to rebuild an identical answer. Resolving a world slot is an index into the P-sized `ranges` array, and the per-element accessors were doing it per ENDPOINT -- three times per term on the recv side, four times per rotation pair in the self-slot gradient loop. `cross_rank_slot()` resolves it once and the element accessors take that view, so walking a slot's endpoints pays for the P-sized lookup once. No behaviour change, and it is the precondition for ever storing slots sparsely, where resolving one stops being an array index. `build_layer_exchange_layout` now has no production caller and is kept deliberately, as the reference the derivation is tested against: the new equivalence case asserts derived == built elementwise for every my_rank and both scales. Checking a derivation against an independent construction beats checking it against literals. `exchange_layout_bytes` and `derivative_layout_bytes` now report 0 rather than being removed from the breakdown, so an A/B against a build that did retain them shows the drop instead of losing the row. Build-time derivation is retained purely as eager validation: an int overflow has to throw from build_graph, not from inside the exchange where peers are already blocked in the count round.
… transpose The previous commit stopped retaining the send layout but kept a RecvLayoutCache per layer -- 8 B per world slot, 10.59 GiB at P=512 -- on the grounds that a transpose is the one thing a rank cannot work out alone. That was wrong: this transpose carries data both sides already have. Layer build gives slot r on rank m the queries r sent m, followed by the queries m sent r; rank r's slot for m holds those two swapped. The counts are therefore equal, and displacements are prefix sums of counts, so the recv layout IS the send layout. MPI reads recvcounts/recvdispls rather than writing them, so the same two arrays now serve both sides of the alltoallv. What goes with the cache: the alltoall_counts on its miss path, and the rank-uniform `exchange_generation` that existed only to keep that miss rank uniform. The hazard the previous commit documented so carefully -- a split reuse decision hanging the job -- is removed rather than managed, because there is no longer a collective on any cache-miss path. sizeof(LayerCore) 248 -> 168 B. Symmetry is an invariant of the routing, not of this file, so it is checked where it can actually break: MONOPROP_CHECK_EXCHANGE_SYMMETRY=1 re-adds the alltoall and throws naming the slot and both counts. Unguarded, a future routing change that broke it would surface as a peer blocked in MPI_Alltoallv against a size nobody sends -- a hang with no line number. Evidence: a probe comparing derived counts against a real alltoall on every resolve saw 0 mismatches in 550M slot comparisons at world 32 and 256, over the full MPI suite and a pauli c12 energy+gradient run. Gate 1826413: 214 ctest serial, and 625 Python tests on each of four geometries TWICE -- once on the production path, once with the assertion live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… breakdown
The graph does not partition. Its per-layer arrays are indexed by rank, and on a
partitioned run that index space is the FLAT world P = ranks x partitions, so they
grow with a P the MPI rank count never shows. `graph_memory_bytes` is a single
scalar and cannot say how much of it is that.
Split the two growth laws so a measurement can separate them:
d_slot_record_bytes the slice of cross_rank_bytes that is one record per
world slot, carried whether or not the slot has traffic
d_slot_records P per layer core; / d_layer_cores recovers P
d_occupied_slots slots carrying any traffic; / d_slot_records is occupancy
d_cross_rank_endpoints the traffic itself, and the ceiling on d_occupied_slots
The last one is the point of the exercise. An occupied slot holds at least one
endpoint, so endpoints bound occupied slots from above -- and endpoints do not
depend on P at all. Together the two say how much of the slot array is information
and how much is reserved-and-empty.
All of them sit OUTSIDE total_bytes(): each is a count or a slice of a field
already summed there, so adding them would double-count. Behaviour is unchanged;
this only reports.
The graph's last array indexed by the flat world size P. Each layer held one record
per POSSIBLE partner, so with P participants each holding a P-length array the job
carried L x P-squared records whether or not anything was ever sent between them. At
L=5,420 and P=512 that is 22.7 GB of slot records against 3.7 GB of actual traffic --
6.1 bytes of addressing per byte of data.
Store the occupied slots instead, ascending by slot id. That is bounded by something
with no P in it: an occupied slot holds at least one endpoint, so
occupied_slots <= total cross-rank endpoints
and the endpoint count is a property of the operator and the circuit, measured flat
in P to 0.096% across a 4x change in it. The quadratic is not merely smaller, it is
capped by the traffic it describes.
The record is 12 B, and two things are absent from it by design:
* the D range, already dropped -- B and D are one endpoint set in two orders;
* the B/D offset, which is the running prefix over stored entries in ascending
order. Empty slots contributed zero to the dense prefix, so the derived value
equals the stored one exactly. A size_t offset would have padded the record to
24 B, so deriving it is worth 2x on its own.
Access changes shape rather than getting slower. Every partner sweep in production
was already `for r in 0..P { if empty continue }` -- walking the whole world to find
the part of it with anything in it -- and becomes for_each_occupied_slot, which
carries the derived offset and never visits an empty slot. The self slot keeps O(1)
through a position resolved once at build: it is read per rotation pair in the
innermost gradient loop and cannot afford a search.
Converted: the four packing loops and the snapshot pass in Evolution.cpp, both totals
in MPGraphLayers.h, endpoint marking in PareGraph.cpp, and the layer export in
MonomialPropagator.inl (still dense in its output, since callers index it by rank,
but now scattered into rather than interrogated for).
graph_encoding_slot_record_bytes_track_the_world_not_the_traffic asserted precisely
the property being removed, so it is inverted rather than repaired: quadrupling the
world must now leave the record array byte-identical.
213/213 serial.
…slots Only needed once the two halves coexist, which is why neither branch carries it. #237 derives counts[r] by asking cross_rank.sin_send_size(r) for every r < P. That was O(1) against the dense range array it was written for. Under the sparse storage sin_send_size is a binary search over the occupied slots, so the same loop became O(P log occupied) -- per layer, per exchange -- to fill an array that is ~82% zeros at P=512 by construction, and whose zero fraction only grows with P. So fill it the other way round: zero the counts, walk the slots that actually carry traffic via for_each_occupied_slot, and scatter. O(P) + O(occupied) with no search at all. The displacement prefix stays dense because MPI_Alltoallv wants an entry per rank and an empty slot still needs a valid, repeated displacement. assign() rather than resize() for the counts: `out` is scratch reused across layers, and a slot carrying nothing this layer must read zero rather than inherit the last layer's count. graph_encoding_derived_layout_reuses_its_scratch pins exactly that. The self slot is skipped by slot id, not by the old r == my_rank test on the loop variable: under sparse storage this rank's own slot is simply one of the stored entries, and it may or may not be present at all. Equivalence is asserted elementwise against build_layer_exchange_layout, for every my_rank and both scales, by graph_encoding_derived_layout_matches_the_layout_it_replaces.
On main, resolve_recv refused a send-count vector whose width was not mpi::size(comm) on every exchange, and its comment records the case as reachable: layouts outlive propagator copies and pare rebuilds, so a graph built for one communicator can be replayed on another of a different size. This branch moved that check into check_exchange_symmetry, which begins with `if (!enabled) return;` on a MONOPROP_CHECK_EXCHANGE_SYMMETRY probe. The default path therefore checked nothing, and begin_flat_exchange went on to hand MPI_Alltoallv a counts array shorter than the rank count -- MPI reads one count and one displacement per rank whatever the span holds, so a thrown exception had degraded into an out-of-bounds read. Hoist it above the gate. The width of the layout is a precondition of posting the transfer at all, not a diagnostic about it, so it holds on every build and every path; only the symmetry audit below it -- which costs a real collective, and exists to falsify an invariant rather than to protect memory -- stays optional. The comment at both the declaration and the definition now says which is which, since sharing one entry point is what let the two be confused. exchange_layout_width_is_checked_even_with_the_symmetry_audit_off pins it: a ShmComm sized 4 driven by one thread, so a layout of the wrong width must throw before any collective is entered. If the check ever slides back under the gate the case hangs or fails rather than passing quietly. Assisted-by: ClaudeCode:claude-opus-5
MONOPROP_CHECK_EXCHANGE_SYMMETRY had four defects, all confirmed in the tree before changing anything: - presence-tested, so MONOPROP_CHECK_EXCHANGE_SYMMETRY=0 turned it ON; - screaming prefix, where every other knob here is monoprop_ (monoprop_NUM_THREADS, monoprop_PARTITION_PINNING, monoprop_PARTITIONS); - read with a bare std::getenv, bypassing config::detail::parse_flag in detail/EnvConfig.h, which is the single home for this; - and it decided whether a COLLECTIVE runs. The last one is why the answer is not "fix the parse". The audit calls alltoall_counts. A variable read on some ranks and not others does not make the job misreport, it makes the ranks disagree about whether the collective happens at all, and the job hangs with no diagnostic. A per-rank environment variable is precisely the mechanism by which they come to disagree, and no amount of parsing care removes it: the value is per process by construction. So the decision moves into the binary, where every rank of a job necessarily agrees: a CMake option monoprop_CHECK_EXCHANGE_SYMMETRY, default OFF, named to match monoprop_ENABLE_MPI and monoprop_WIDE_TERM_INDEX, propagated as a compile definition on monoprop-objs (PUBLIC, so the unit-test target sees it too) and reported in the configure summary alongside the other build switches. No replacement environment variable is introduced. The audit itself is unchanged, and so is where it sits: below the unconditional width precondition added in the previous commit, which is not part of it. Assisted-by: ClaudeCode:claude-opus-5
build_layer_exchange_layout has no production caller: the engine derives the layout from the slot records (derive_exchange_layout) and nothing else calls it. Its only job is to be the independent reference that derivation is asserted against, and it was compiled into libmonoprop.so to do it. That placement voids what it is for. An oracle that ships inside its subject is edited by whoever edits the subject, refactored by the same refactor and broken by the same mistake; a check that travels with the thing it checks is not evidence about it. Moved to cpp/tests/ExchangeLayoutOracle.h, where changing it means changing a test, and the comment there says so. checked_mpi_int stays in the library and the oracle keeps calling it. It is the shared narrowing guard for every MPI count in the tree, not part of the layout rule under test, so copying it would only mean asserting our own copy of an overflow policy. No CMake change: cpp/tests/CMakeLists.txt GLOBs the directory, and this is a header included by graph_encoding_tests.cpp. Assisted-by: ClaudeCode:claude-opus-5
d_recv_cache_bytes and d_derivative_layout_bytes are new on this branch, are outside total_bytes(), and are assigned a literal 0 by construction: the recv layout IS the send layout, so nothing is cached, and the 2x derivative layout is derived on demand rather than retained. They can never report anything else. The comments justified keeping them as a way for an A/B to see the memory leave. That reading does not hold: main emits no graph_memory_breakdown() dictionary at all -- the whole method is new here -- so there is no older build whose output a zero row lines up against. A key that is always 0 tells a reader nothing except that a field they cannot use exists, and main's convention is to emit no key rather than a zero one. exchange_layout_bytes is NOT touched. It is a real field on main (MPGraphViews.h), it is summed by total_bytes() there, and it is fed from MPGraph.cpp, so its value on this branch is a comparable measurement and the comment explaining why it now reads 0 stands. Assisted-by: ClaudeCode:claude-opus-5
It measures the resident size of a LayerExchangeLayout: counts.capacity() + displs.capacity(), in ints. That was worth reporting while a layout was retained per layer per partition. This branch stopped retaining one -- the layout is derived into per-thread scratch for the exchange being posted and dropped -- so the function survives only as a sizeof over a transient nobody holds. The reason it is dead is this branch's own doing, not a pre-existing condition: on main it had a caller. Its last caller here was a BOOST_CHECK_GT(..., 0U) asserting that a freshly built layout occupies more than zero bytes, which restates std::vector rather than testing this codebase, so it goes with it. Nothing else referenced either the function or the key it used to feed. Assisted-by: ClaudeCode:claude-opus-5
… widths
cross_rank_partner_range_counts_track_term_index_width asserted
sizeof(CrossRankOccupiedSlot) == sizeof(uint32_t) + 2 * sizeof(TermIndex)
unguarded, so it runs in both builds. It is only true in one. The record is
{uint32_t slot; TermIndex sin_send_count; TermIndex in_count;}: narrow that is
4 + 4 + 4 = 12 and the sum agrees, but wide TermIndex is 8-byte aligned, so the
u32 is followed by four bytes of padding and the record is 8 + 8 + 8 = 24 against
an expected 20. Measured, not reasoned: a standalone probe compiled at both
widths reports sizeof/alignof 12/4 and 24/8, with in_count at offset 8 and 16.
Nothing else catches it, which is why it has stood. The static_assert beside the
struct is written `sizeof(TermIndex) != sizeof(uint32_t) || sizeof(...) == 12` --
it switches ITSELF off on exactly the build where the check fails, and
monoprop_WIDE_TERM_INDEX has its own `just test-wide` recipe precisely because
that configuration is otherwise never compiled.
Assert the layout rule instead: the u32 slot id occupies a whole
TermIndex-alignment slot, so the record is max(sizeof(u32), alignof(TermIndex))
+ 2 * sizeof(TermIndex), plus alignof(record) == alignof(TermIndex). Both hold at
both widths. The comment states where the padding is, so the expression does not
look like arithmetic waiting to be tidied back into a sum.
Assisted-by: ClaudeCode:claude-opus-5
…t list slot_sin_recv_index computes Q = sin_send_count - in_count in unsigned width. in_count larger than sin_send_count therefore does not underflow into a negative that a later comparison would reject: Q wraps to ~2^64, `idx < out_count` is true for every index, and each D read addresses B at in_count + idx -- past the end of sin_send_indices, for every endpoint of the slot. Reachability, checked rather than assumed. The only production producer is GraphSink::finalize (layer_build/Engine.h), which sets in_count = P after sizing sin_send_indices to P + Q from the same two values, so the engine cannot violate it. But build_packed_cross_rank_storage takes CrossRankPartnerData from any caller and validated only the B/D length equality; nothing in the type, and nothing at the choke point, said the in-block had to fit. That is the same status the B/D skew had before it was checked there, and the comment on that check already gives the reason: at a choke point an assumption becomes a precondition. So it is checked in the same loop, with the same std::logic_error, and the reader keeps its branch-free subtraction behind an assert that names the invariant. The boundary case in_count == B.size() -- an all-in slot with an empty out-block -- stays legal, and the new test pins both sides of it. Assisted-by: ClaudeCode:claude-opus-5
Three accessors survived the sparse-slot rewrite with no caller left.
Verified by `grep -rn` over cpp/ src/ packages/ tests/ (git grep is broken
on this login node); each matched exactly once, at its own definition:
cross_rank_sin_recv_index_at 1 hit (the definition)
cross_rank_sin_recv_phase_at 1 hit (the definition)
LayerTraversal::cross_rank_slot(size_t)
1 hit for the member; the 19 other
`cross_rank_slot` hits are the free
detail::cross_rank_slot(storage, rank)
and detail::cross_rank_slot_record_bytes,
neither reached through the member.
for_each_cross_rank_sin_send_range / _recv_range are NOT touched: they have
one caller each, in large_cosine_storage_tests.cpp. Test-only is not dead.
<stdexcept> in MPGraphEncodingStorage.h became unused when
ExchangeLayoutRankMismatch was removed; dropped after replaying every TU's
compile line with -fsyntax-only in all three configurations (default,
-Dmonoprop_WIDE_TERM_INDEX, -Dmonoprop_CHECK_EXCHANGE_SYMMETRY), each
clean with zero FAIL and zero WARN.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The diff carried 598 added comment lines under cpp/ + src/ against 1611 added lines (37.1%). Comment lines only; no code is touched. Three things went, in order of volume: - Five arguments each restated four to seven times, reduced to one canonical copy: the audit is a build option because it is a collective (kept in detail/mpi/Exchange.h and docs/building.mdx, pointers elsewhere); an in-block past the end wraps unsigned (kept at the throw site in MPGraphEncoding.cpp); the graph is indexed by the flat world, so O(P) per layer is O(P^2) per job (kept on CrossRankOccupiedSlot); the oracle lives outside the library or it drifts (kept in cpp/tests/ExchangeLayoutOracle.h); the u32 slot id costs a whole TermIndex alignment slot (the static_assert message already says it, so the prose went). - Three harness campaign findings that had leaked into shipped source as prose. None is checkable from the tree; they belong in the PR body. - Two essays: hybrid_comm_tests.cpp's mutation-modelling header, reduced to the one fact a reader needs (a consistently transposed tiling is indistinguishable to any black-box test), and HybridComm.h's bit-identity proof, reduced to the claim and its reason. Kept, compressed rather than cut, because they say why the code is correct: the Phase P0 lifetime / barrier-ordering argument, the occupied-slot rationale, and every static_assert message. Also fixed: hybrid_comm_tests.cpp credited `recv` with a reuse property that belongs to stage_recv_'s high-water mark. And cpp/tests/README.md's suite inventory was missing exchange_layout_precondition_tests.cpp; the bullet is appended at the end of that list, leaving the existing entries untouched. Verified with -fsyntax-only over every TU's real compile line in all three configurations (default, -Dmonoprop_WIDE_TERM_INDEX, -Dmonoprop_CHECK_EXCHANGE_SYMMETRY) and with clang-format. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hat cannot Nine cases in this suite open with `if (world_size() < 2) return;` and report Passed having executed zero assertions in a world-1 variant. monoprop_MPI_TEST_PROCS defaults to 2 so the coverage exists, but -Dmonoprop_MPI_TEST_PROCS=1 turned nine green lines into nine empty ones silently; configure now fails instead. Empty stays legal because boostAddTests substitutes 2 for it. The audit's throw was reached by no test at all: both existing cases cover the width check, which sits above the #ifndef and runs in every build. The new case needs two real ranks -- at one participant the alltoall returns this rank's own counts, so the mismatch is unreachable -- and carries a symmetric negative control, so a function that threw unconditionally would still fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lising it The int-overflow case built 2^30 real endpoints to reach a count that fits int at 1x but not 2x, peaking at 19 GB RSS. Free on a compute node, fatal on a 16 GB CI runner, and never executed before because the draft matrix was skipped. derive_exchange_layout reads only sin_send_count and rank_count(), so one declared occupied slot reaches the same boundary: 286 MB peak, 2 of 2 assertions passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The symmetry is a theorem about the count matrix, not a runtime risk, so the audit bought a per-exchange collective for a property the derivation cannot violate. No CI job built it ON, so nothing automated covered it either. check_exchange_symmetry did two things. Its width check is not this branch's invention: on main it lives inside resolve_recv, which this branch deletes, and was relocated above the gate rather than added. It stays, and the function is renamed to check_exchange_layout_width for what it now does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The codebase throws its own type 36 times against six generic ones, and all six were in this file. These two say a slot's B and D sides disagree, or that its in-block escapes its endpoint list -- both invariants of the build, so the type should name them. Derived from std::logic_error, so the cases asserting on that still hold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Capture lists spelled out on the seven occupied-slot lambdas, matching what the neighbouring lambdas in both files already do. find() takes the ranges overload and std::to_address. Two that are not cosmetic: - build_layer_storage_unified took its partner vector by value, but the move into build_packed_cross_rank_storage was a no-op -- that overload takes a const reference -- so the by-value bought a move and consumed nothing. Now a const reference, and the inert std::move at both call sites is gone. - Exchange.h keeps an explicit MPI_Request rather than the suggested `auto *`: the pointer spelling compiles only where MPI_Request is a pointer typedef, and is an int handle under MPICH. This is what the rest of detail/mpi already does. Drops the S5414 suppression for MPGraphEncodingTypes.h. It was justified by LayerCore's private cache member and named reset_derivative_exchange_layout(); this branch deletes both, and the header now has no private members at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ten blocks called out in review, plus the same treatment applied to the rest of the diff, the binding and the Python test rather than only where a comment happened to land. Every shouted word is gone: 33 sites of ALL-CAPS emphasis across 12 files, now none outside the acronym MPI. Added comment lines go 264 -> 183 against 942 of code, 21.8% -> 16.3%. Not the under-10% I aimed at: the remainder is mostly HybridComm.h's barrier and lifetime rules and the test rationale, and cutting to a number from there would delete the reasoning rather than the prose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AGENTS.md, docs/building.mdx and docs/testing.mdx go back to base exactly, and the README paragraph about the removed build option goes with them. Two are partial on purpose, and I would rather be told to finish them than do it silently: - features/parallelism.mdx keeps "Graph memory at large world sizes" and loses only the two lines about the audit. The rest is the O(P^2) rule this branch exists to fix, and the review comment sat on the audit sentence. - cpp/tests/README.md reverts the rewritten graph-encoding paragraph but keeps the two bullets naming ExchangeLayoutOracle.h and exchange_layout_precondition_tests.cpp. Both files are new here, so a full revert would leave them undocumented. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Asked in review whether we need it. We do not: no configuration checked into the repo can trip it. The cache default is "2", and justfile / vscode set monoprop_MPI_TEST_PROCS only as a shell variable for mpiexec -n, which never reaches CMake. It fires only for a hand-passed -Dmonoprop_MPI_TEST_PROCS=1, so it guarded a value someone types rather than one we ship. The hole it aimed at is real and stays open: cases that early-return at world size 1 report Passed having asserted nothing. Declining to catch that here is a choice, not a fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`end-of-file-fixer` failed the `lint` job on 4807e41: deleting the dead `layercore` suppression left its separating blank line, so the file ended with two newlines instead of one. Audited the other 29 changed files for the same class of problem — final newline, trailing blank lines, CRLF, trailing whitespace — and they are clean. `clang-format` passed on 4807e41; its "Formatting" lines are verbose output, not diffs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
diagonal-hamiltonian
force-pushed
the
perf/sparse-slots-and-funnel
branch
from
August 24, 2026 10:29
9e2ac0c to
4491fc8
Compare
Removed section on graph memory at large world sizes from the parallelism documentation. Signed-off-by: Aaron Miller <61472721+diagonal-hamiltonian@users.noreply.github.com>
robertodr
reviewed
Aug 25, 2026
robertodr
reviewed
Aug 25, 2026
| } | ||
|
|
||
| // Derives both sides at once: the count matrix is symmetric, so the recv layout is the send layout. | ||
| auto derive_layer_exchange(const LayerTraversal &layer, const mpi::Comm &comm, int scale, FlatExchangeBuffers &buffers) |
Member
There was a problem hiding this comment.
why FlatExchangeBuffers &buffers (IO param) instead of returning a FlatExchangeBuffers object?
Collaborator
Author
There was a problem hiding this comment.
Because buffers is thread_local scratch — acquire_flat_exchange_buffers() hands back the same object on every call, and the point is to keep its three allocations across layers and steps. Returning a FlatExchangeBuffers would reallocate counts/displs (sized by world) and both payload buffers on every layer of every verb.
That said, the parameter was wider than the function: it only ever writes buffers.layout. It now takes LayerExchangeLayout &.
While there, resize_flat_exchange_buffers is gone — it was a one-line wrapper around a single resize. The send-buffer sizing is now inline at the call site, the same shape begin_flat_exchange already uses for the recv buffer:
derive_layer_exchange(layer, comm, scale, buffers.layout);
buffers.send_buffer.resize(buffers.layout.total_count == 0 ? 1 : buffers.layout.total_count);
robertodr
reviewed
Aug 25, 2026
robertodr
reviewed
Aug 25, 2026
robertodr
previously approved these changes
Aug 25, 2026
robertodr
left a comment
robertodr
left a comment
Member
There was a problem hiding this comment.
Got some comments, but overall LGTM. This is a complex piece of code in need of complete rewrite though...
- `derive_layer_exchange` takes `LayerExchangeLayout &` rather than the whole `FlatExchangeBuffers &`: it only writes the layout. The out-param stays because the buffers are thread_local scratch reused across every layer; returning by value would reallocate per call. - Drop `resize_flat_exchange_buffers`, a one-line wrapper. The send-buffer resize is inline at the call site, matching how `begin_flat_exchange` already sizes the recv buffer. - Replace the `grow` lambda in `snapshot_remote_endpoints` with four `resize(std::max(size(), occupied))` calls. - Cut the comment blocks in `layer_storage_memory_usage` and above `CrossRankOccupiedSlot`. `exchange_layout_bytes = 0` went with them: the field is `= 0` in the struct and `breakdown` is default-initialised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
🤖 AI text below 🤖 Gate on
|
| gate | result | job |
|---|---|---|
ctest -L unit |
238 / 238 passed, 0 failed | 1850563 |
ctest -L serial |
237 / 237 passed, 0 failed | 1850563 |
ctest -L serial (2 nodes) |
237 / 237 passed, 0 failed | 1850573 |
ctest -L mpi |
1 / 1 passed | 1850573 |
pytest tests --with-mpi, ranks 2 x partitions 1 (world 2) |
593 passed | 1850573 |
pytest tests --with-mpi, ranks 2 x partitions 16 (world 32) |
593 passed | 1850573 |
pytest tests --with-mpi, ranks 4 x partitions 8 (world 32) |
593 passed | 1850573 |
pytest tests --with-mpi, ranks 16 x partitions 16 (world 256) |
593 passed | 1850573 |
ALL SUITES PASSED, no failures at any layout.
The counts are higher than the ones in the body (230 / 229 / 593) because main merged in since: the C++ suite grew, the Python suite did not.
Not re-measured. The performance tables in the body stand on the pr6v2 campaign at 446d60c. 3a87c66 is comment removal plus three mechanical changes — a narrowed parameter, an inlined one-line wrapper, and a std::max in place of an if — so it does not touch what those tables measure, but it is also not the binary they were measured on.
|
robertodr
approved these changes
Aug 25, 2026
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
Per-layer graph memory no longer grows with the flat world size
P = ranks x partitions x nodes.CrossRankPartnerRangewas a dense array overP(32 B narrow / 40 B wide).
CrossRankOccupiedSlotis 12 B / 24 B and exists only where there istraffic, with offsets from a running prefix. One such array per layer made an
O(P)costO(P^2)per job.
O(P + occupied)into per-thread scratch.RecvLayout.handresolve_recvare gone: the count matrix is symmetric, so the recv layout isthe send layout. That also removes a hazard —
resolve_recvcould skip a collective on a cachehit, letting ranks disagree about whether it ran. Nothing in the replacement can split ranks. Its
width precondition is not lost: it survives as
mpi::check_exchange_layout_width, on the pathevery exchange takes.
HybridCommstaging matrix is re-indexed peer-outermost, two owner-written tables publishbefore barrier B1, and
scatter_off_becomes a cursor. Still 4 barriers per payload verb, and theoffset tables are elementwise equal — integer sums reassociated, not an exchange reordered.
Partition 0 stays
Theta(R*S^2): this is not a fix for the funnel.ContractSink::finalizereturnsnullptr, sopropagatebuilds noLayerCore— its win comesentirely from mechanism 3, while
build_graph/energy/gradientare where mechanism 1 shows up.Also:
graph_memory_breakdowninbinder.h(five counters, each outsidetotal_bytes()and insideoperator+=), andExchangeLayoutOracle.hkept outside the library so it cannot drift into agreeingwith the code it checks.
Reviewing this PR
Where the risk is, for anyone coming to this cold:
resolve_recvis onlysafe if the exchange count matrix is symmetric, so a rank can derive what it will receive from what
its peers send. If that is wrong anywhere, the failure is a hang or silent corruption, not a failed
assert.
cpp/tests/ExchangeLayoutOracle.hchecks the derivation independently, from outside thelibrary.
HybridComm.h(mechanism 3) is the hardest file to review, and the place where a reorderingthat looks equivalent may not be. The claim is unchanged barrier count and elementwise-equal offset
tables, with reassociation of integer sums only.
find()is nowstd::ranges::lower_boundover a sorted array. It requiresoccupiedto stay sorted byslot; a builder that appends out of order breaks lookup silently.Measurement
These figures supersede the tables posted earlier. Those were measured at
e25bbfb, on a binarythat still carried the since-dead PR 5 and PR 7. What follows is a fresh 12-cell campaign on
main.Setup: 10 interleaved reps, one Slurm allocation per cell holding both arms with the order flipped per
(rep, cell). Ratios are formed per rep then medianed, judged by a two-sided sign test with Holmstep-down within each family. Peak RSS is
VmHWMfrom/usr/bin/time -v, summed over nodes. LayoutA = 1 rank x 128 partitions, B = 8 ranks x 16 partitions, each at 1 and 2 nodes. Harness
ce49174,campaign row
pr6v2.1250a27eded39f1639a6913ca3d8e101@668c90b— 216/216 unit, 215/215 serial (job 1840045),592 passed x 4 MPI layouts (1840046)
97b9a114a6f0cac97eded7fe5c0d7b97@446d60ca— 230/230, 229/229,-L mpi1/1 (1839668),593 passed x 4 layouts (1839669)
Checked before any timing was read: term counts identical to the term in every cell
(278,722,051 / 96,981,051 / 91,273,861, zero spread), thread placement symmetric across arms in all 12
cells (128/rank at A, 16/rank at B, zero mismatches), arms provably distinct binaries, and no MISSING
or FAILED rep anywhere.
Wall time. 13 of 24 tests resolve at the 10-rep sign-test floor, all of them improvements and none
a regression, split A=9 / B=4 so this is not one layout.
grid-pauli-Agrid-pauli-Agrid-pauli-Agrid-hubbard-graph-Agrid-hubbard-Agrid-pauli-Agrid-pauli-Bgrid-pauli-Bgrid-pauli-Bgrid-pauli-Agrid-pauli-Agrid-hubbard-graph-Agrid-pauli-B2 propagate[pauli] /grid-hubbard-B2 propagateThe remaining tests do not resolve at 10 reps, which means indistinguishable rather than a small win
in either direction.
Peak RSS, node sum. The graph cells are the headline:
grid-pauli-Agrid-pauli-Bgrid-pauli-Bgrid-pauli-Ahubbard-graphmoves less (0.89x and 0.91x at N=2, ~0.97x at N=1) and plainhubbardis flat(1.00x), as expected:
propagatereleases each layer as it contracts, so there is little retainedgraph to shrink.
Why every large win is at N=2. The flat world is
ranks x partitions x nodes, so 1 node to 2doubles
P, and what this PR removes isP^2-scaled waste in the per-layer arrays. Layout A at N=2(
P=256) is where the graph work is mostP^2-dominated and it carries the biggest ratios; layout Bat the same
Pshows the same signs, smaller. The N=1 cells landing at 0.91-0.97x are the same effectat
P=128. This is an explanation consistent with all 24 points, not an isolated mechanism.Direction and rough magnitude match the superseded tables —
build_graph[hubbard]A/N=2 at 0.53x hereagainst 0.43x there, pauli graph peak RSS 0.40x against 0.39x. The timing absolutes are somewhat
smaller, which is what you would expect once the two dead PRs leave the baseline.
Caveats.
propagate[hubbard]at A/N=1 andB/N=1,
build_graph[hubbard]at graph-B/N=2,propagate[pauli]at A/N=1. The fix is more reps, nota re-read of the ratio.
attributed to the stack as a whole and to no named commit.
staging_sis not on this branch, so the funnel cannot be read asstaging_s + mpi_sand theCOMMPROF residual is invalid here — it goes negative at layout B. These are wall time and peak RSS.
Gate on the current head
Re-run on
3a87c66, after themainmerges and after the review fixes, because #267 and #268 bothtouch files this PR changes:
ctest -L unitand-L serial, and-L mpiAll green. Counts and the tested binary's md5 are in a comment on this PR rather than inlined, so the
body cannot drift from what was run.
Next, not here: most world slots carry no traffic (28.0% occupancy at
P=16, 16.6% atP=128), so aneighbourhood collective would retire
MPI_Alltoallv,world_sizeand the dense prefix sum, andpack_off_is now the dominant serial term. Both false-share belowS=16and need their owncampaigns.
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.