perf(graph,mpi): ⚡ derive the exchange layouts and store only occupied world slots - #270
Open
diagonal-hamiltonian wants to merge 45 commits into
Open
perf(graph,mpi): ⚡ derive the exchange layouts and store only occupied world slots#270diagonal-hamiltonian wants to merge 45 commits into
diagonal-hamiltonian wants to merge 45 commits into
Conversation
github-actions
Bot
added
documentation
|
Docs preview: https://pr-270.monoprop-docs.pages.dev |
Collaborator
Author
Campaign — re-measured on this cutFresh 12-cell campaign, 10 interleaved reps, one allocation per cell holding both arms with the
Preconditions checked before any timing was read: term counts identical to the term within every Timing — 13 of 24 tests at the sign-test floor, and not one regression
Read the labels carefully: a bare Memory — peak RSS, kernel truthThe graph cells are the headline, and they replicate the withdrawn figure almost exactly:
The The N=2 pattern is the mechanism, not a coincidenceEvery large win is at N=2. That is exactly what this change predicts: the flat world is I am labelling that an explanation consistent with all 24 points, not an isolated mechanism — see the Versus the withdrawn numbersThe old writeup claimed 16 of 24 resolved with Caveats, unchanged
Not reproducible from the diff: the benchmark harness does not ship. |
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
Collaborator
Author
|
Working through the 34 comments. Read the replies above with this caveat: the changes are not on this branch yet. They are six commits on a separate local branch, because another session currently owns Six commits off
Gate, on the final commit, arm
Three notes where I did not do what was literally asked, each with its own reply on the thread: Two things I did that nobody asked for, so they are easy to reject: the On comment volume I did not reach what I aimed for. 16.3% is a 31% cut, not the under-10% I wanted. What is left is mostly The measurement tables in the body were taken at |
Collaborator
Author
|
🤖 AI text below 🤖 Correction to my earlier comment: this is pushed now. That comment said the changes sat on an Two things worth flagging rather than leaving you to find them. 1. The comment-density numbers in my thread replies are wrong. I quoted "264 → 183 added comment
So a 36% cut, not 31%. It is still not the under-10% I was aiming for — what is left is mostly the 2. Two of your revert requests I did not do literally, both flagged on their own threads: Also: On the deleted symmetry audit — the width check it shared a function with was not this PR's The tables in the body were measured at I have left every thread unresolved for you to close. |
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
…ition 0 alltoallv's B1->B2 window is partition 0 alone, and size_staging_impl_ spent it in three r_*s_*s_ loops. In two of them the SOURCE-partition index su was the innermost loop, so every iteration did two dependent loads that strided across a different thread's array. Measured at layout A (r_=2, s_=128, P=256, pauli, 5420 layers) that serial non-MPI term is ~500 us per collective, ~24-27 s per job, by the barrier_peers_s - barrier_p0_s - mpi_s identity. At layout B (r_=16, s_=16) it is 1-8 us: the term is s_-squared, not intrinsic. ShmComm::alltoallv does the same job with two barriers and no staging pass at all. Three changes, none of which adds a barrier -- the verbs stay at exactly 4: * Phase P0, before B1, on every partition: publish its own row of counts_matrix_ and its per-rank row totals row_send_/row_recv_, reading only its own arguments. No peer state is touched, so no barrier is needed to make it safe; the lifetime argument that lets a fast peer write row u for verb k+1 while a slow peer is still in verb k's tail is written out above publish_counts_row_ and MUST survive future edits. This is where the cross-partition traffic is now paid: by S owners in parallel, each on its own core, touching only memory it just wrote. Rows are line-padded and the base realigned so the concurrent publish cannot false-share; the constructor asserts the padding. * Phase P1, partition 0, between B1 and B2: every pass is contiguous. The per- rank message sizes come from the published row totals (R*S reads, not R*S*S); the column sums and the exclusive prefix that build pack_off_ are both u-OUTER and g-INNER, so they sweep counts_matrix_ and the accumulator in address order. pack_count_matrix_ is transposed the same way: it now streams a contiguous P-int row per partition and takes the stride on the write into this rank's own counts_send_. * scatter_off_ is DELETED. scatter_off_(a,t,su) was a prefix over su at fixed (a,t) over partition t's OWN published recv row, so it never needed to be on partition 0 at all; the post-B4 loop already walks (a,su) in the order those offsets accumulate, and re-derives them from base_recv_[a*S+t] and its own counts. The fused resolve verb gets the same treatment on the recv side: contiguous per-(a,t) block sums over counts_recv_ instead of the strided closure. Bit-identical by construction, and the argument is in the header above size_staging_send_: the staging BLOCK ORDER is unchanged, and the new base_send_/pack_off_ split is elementwise EQUAL to the old running cursor, because the column sum W[b*S+t] is exactly what the old inner u-loop accumulated before moving to t+1. Every changed quantity is an integer prefix sum reassociated over an exact operation; no floating-point accumulation order is touched anywhere. pack_off_ is re-indexed from (b,t,u) to (u,g) -- same element count, transposed so the row a partition reads is contiguous where it used to stride by s_. block_idx_ is split rather than reused: counts_idx_(b,t,su) is the count message's wire layout, pack_idx_(u,g) is the payload offset table, and the two no longer share a shape. Slot::counts and Slot::recv_counts are dropped, now that no partition reads a peer's count pointer. checked_mpi_count moves from one whole-rank accumulator to s_ subtotals with its six message strings unchanged, so the same failure still names itself the same way. s_ == 1 degenerates to one memcpy and 2*r_ adds on the single partition. Assisted-by: ClaudeCode:claude-opus-5
Re-derivation of the ordering expectations against the restructured staging
tables, plus the two cases the restructuring showed were missing.
The re-derivation first, because the answer is not what the size of the diff
suggests: hybrid_comm_alltoallv_source_order_and_tags does not move, and the
reason is now stated at the top of the file. Two independent facts.
1. Delivery order is fixed by recv_displs -- caller-supplied in alltoallv, the
ascending prefix over global source in the fused verb -- and the staging
layout never reaches the caller. No delivery expectation can depend on it.
2. The staging order did not change either. The message to rank b still runs
destination partition t ascending with source partitions u ascending inside
each t; only the arithmetic producing a block start changed, and it is
elementwise EQUAL to the old running cursor. Checked by hand on R=2, S=2 for
all four blocks on each side. So the wire format is identical and there is no
mixed-version interop hazard between ranks.
What the existing cases genuinely cannot pin is the tiling. In every one of them
a partition sends the same length to every destination, so the send-count matrix
is constant along each row -- and under that shape a (dest, source)-transposed
offset table tiles the staging buffer just as validly and delivers every byte to
the right place. So:
* hybrid_comm_alltoallv_pairwise_counts drives counts that vary along BOTH
indices, with a tag unique per (source, destination, index) so a block landing
at the right offset from the wrong source is caught rather than coincidentally
matching. It is also the only DIRECT coverage of HybridComm::alltoallv: this
file only ever reached the fused verb, and the caller-supplied-recv-layout path
-- the one that sizes staging from the recv rows the partitions publish before
B1 -- was covered only end to end, through Engine.h's response round in
mpi_distributed_layer_equivalence. It would fail against a transposed or
mis-based table.
* hybrid_comm_alltoallv_resolve_pairwise_counts runs the same counts through the
fused verb, whose recv side is sized from the count matrix on partition 0
instead of from published rows and is therefore a separate path. rc/rd are
outputs there, so they are checked rather than supplied.
Both sweep S in {1, 2, 3}, which covers the S == 1 degenerate case, and both run
12 rounds with a periodic no-zero high-water round so the smaller rounds after it
execute over stale staged bytes.
Both also count the assertions they actually reach and BOOST_CHECK_GT that count:
their checks sit inside two nested count-dependent loops, which is precisely the
shape that passes having asserted nothing.
Assisted-by: ClaudeCode:claude-opus-5
Three comment defects from review. No behaviour change; the code is untouched apart from the asserts. 1. The instrument's accounting block still described the pre-P0 timing geometry, and that block is what a reader trusts when sizing this problem. Two identities silently stopped holding. `barrier_peers_s ~= mpi_s` held only while the sole thing peers did between B1 and B4 was wait: they reached B2 immediately after B1 and were parked through partition 0's whole sizing pass, so that pass landed in their barrier_ns. It is now paid in Phase P0, before B1, where no timer covers it. The residual barrier_peers_s - barrier_p0_s - mpi_s -- the metric this whole problem was sized with -- measured partition 0's serial non-MPI time only because the peers' wait mirrored it. Both quantities now fall for two reasons at once: work that got faster, and work that moved OUT of the timed window. Reading the drop as the speedup would overstate it. Says so, and says what to trust instead: wall time per verb, and mpi_s, which is unchanged by construction. Notes that the timers that would make the split legible again -- one around the B1->B2 sizing block, one around Phase P0 -- do not exist yet. 2. "This is where the R*S*S cross-partition traffic is paid, by the owners, in parallel" overstated what moved, and the overstatement had already been repeated upstream. Partition 0 still makes two Theta(R*S^2) passes over other partitions' rows inside B1->B2, and a third in pack_count_matrix_ for the fused verb. The volume it pulls across cores is unchanged at S rows x P ints. What changed is the access pattern and the pass count: three strided passes, each dereferencing a different thread's separate array per iteration, become two sequential sweeps of one contiguous matrix (three for the fused verb, the third sequential too). Only the row TOTALS genuinely moved to the owners, and they are O(R*S) of it. Also softens "touches no peer argument array" above size_staging_send_, which was true only on the technicality that counts_matrix_ is not an argument -- it is still another thread's memory. 3. Both constructor asserts were tautologies: round_up_ makes the stride a multiple of the line for every geometry including p == 0, so they asserted a property of round_up_, never of this geometry. They also compile out of every configuration anyone benchmarks -- cmake/compiler_flags/GNU.CXX.cmake sets -DNDEBUG for both Release and RelWithDebInfo. Replaced with the two things that could actually be wrong (the realigned base landed on a line; the one-line over-allocation covered the shift), plus the arithmetic proof of both in a comment, since that is what has to carry them under NDEBUG. Assisted-by: ClaudeCode:claude-opus-5
The justification I wrote for the two pairwise-count cases was false, and review caught it. Re-derived with a black-box model of the staging over 8 geometries with R, S in [1, 4], counting blocks delivered with wrong content. What I claimed -- that the old uniform-count cases cannot pin the tiling because a (dest, source)-transposed table tiles the buffer just as validly -- is wrong twice over: * A CONSISTENTLY transposed layout mismatches 0 blocks at every geometry under every count pattern, old or new. It is a genuinely valid alternative tiling and no black-box test can distinguish it at any count pattern, mine included. Only the bit-identity comment above size_staging_send_ pins that choice. * A send-side-ONLY transposition is caught by the OLD cases at least as well as by the new ones: 63 of 81 blocks wrong at R=3, S=3, against 46-48 for the pairwise pattern. I also could not reproduce the replacement justification offered in review, that the cases catch a flat index built as t*R+b instead of b*S+t with 0 mismatches under the old pattern. When it is the STORE index that slips, 6 of the 9 destination bases move even under uniform counts (base_send_ becomes [0,18,36,6,24,42,12,30,48] against [0,6,12,18,24,30,36,42,48] at R=3, S=3), so the old cases catch it too. The real class is narrower and is named now. This change introduces two DERIVED per-destination aggregates that did not exist before -- col_sum_ on the send side and recv_col_ on the recv side; the old code carried one running cursor over the raw counts and never formed a per-destination total. When every partition sends the same length to every destination, both aggregates are CONSTANT along the very index a slip would scramble (col_sum_[g] is one value for all g; recv_col_[a*S+t] depends only on a), so a mis-indexed READ returns the right value. Transposing or dropping the destination index on either aggregate is caught in 0 of 8 geometries by the old patterns and in 6 of 8 (send) and 4 of 8 (recv) by the pairwise counts. That is what these cases are for, and the flat-index slip is invisible only in exactly this form -- when it is the col_sum_ read, not the store, that slips. Also splits the hollow assertion guard in the resolve case. Its `checks` counter was incremented once per source before any payload byte was compared, so BOOST_CHECK_GT(checks, 0) was satisfied by P * rounds even if every resolved count came back zero and no payload element was ever examined -- the exact hole the counter exists to close. Now layout_checks and payload_checks are counted and asserted separately, and the sibling case's counter is renamed payload_checks to match what it actually counts. Assisted-by: ClaudeCode:claude-opus-5
The endpoint count and the in-block size were narrowed to TermIndex with a bare static_cast, two lines below where checked_term_index guards the indices. A truncation there is not local to its own slot: total_b on the next line accumulates the UNTRUNCATED size and so sizes the B/D arrays, while every reader rebuilds a slot's offset as a prefix over the STORED counts -- so one truncated slot shifts the window of every slot after it. Two further consequences. main threw on such a slot because build_layer_exchange_layout was handed the untruncated size, whereas derive_exchange_layout now checked_mpi_ints the truncated count, so the one window that truncates is exactly the one that no longer throws. And the in_count <= sin_send_count invariant checked immediately above can be inverted by truncating the two operands independently. Unreachable today at either width -- it needs a single slot in a single layer past the TermIndex ceiling -- so this is a lost guard, not a live bug. Build-time, once per occupied slot, outside every apply loop.
build_layer_storage_unified compared all_partners.size() against cross_rank.rank_count() and threw ExchangeLayoutRankMismatch on a mismatch. The two are the same number by construction: the std::move into build_packed_cross_rank_storage is a no-op because the callee takes a const reference, and rank_count() returns the world_size that builder assigned from exactly that size. The comment claiming this is "the one place the two can still disagree" was false. Nothing referenced the exception type but its own definition and this throw, so both go. Dead code removal -- no behaviour change at any world size, and no test named the type.
The header comment was byte-identical to main's and named a race through recv_cache and the lazy derivative layout. Neither exists on this branch: detail/mpi/RecvLayout.h is deleted and LayerCore now declares no mutable member at all, so there is no const handle that writes through. Comment only.
derive_exchange_layout built "<what> count" and "<what> displacement" with std::format unconditionally at the top of the function. On main the equivalent work happened once per layer at build; here the function runs once per layer per POSTED exchange, so those two std::strings became two heap allocations on the exchange path. checked_mpi_int takes a const char *, so the label cannot simply be built lazily inside it. checked_exchange_int does the int-range test first and only re-enters checked_mpi_int -- with the formatted label -- on the path that is about to throw, so the message text is unchanged. Two allocations per layer per exchange, against a payload transfer: not expected to be visible in a wall-clock A/B, and no A/B is claimed here.
build_layer_storage_unified derived both exchange scales at build time as eager overflow validation and discarded both results. Scale 2's counts and displacements are exactly 2x scale 1's, so scale 2 crosses the MPI int limit strictly first and scale 1 cannot fail on its own -- the first call could never throw where the second did not. One O(occupied) pass per layer removed at build time, and one less LayerExchangeLayout fill. Validation coverage is unchanged. A graph that would have been reported at scale 1 is now reported with the derivative label, which is accurate: such a graph is unusable for both rounds.
DerivativeSnapshotScratch was the last per-layer array still dense in the flat world P: four std::vector<VecD> sized R, with a 4R clear() sweep per gradient layer before filling only the occupied slots. It is now indexed by OCCUPIED POSITION, so it is bounded by traffic and nothing in it grows with the world. for_each_occupied_slot hands out the position rather than leaving each call site to count: the three snapshot loops each `return` early on the self slot, and a hand-rolled counter placed after that return would skew every later index silently. The index is passed only to a callable that accepts it (if constexpr on is_invocable), so the existing two-argument callers -- MPGraphLayers.h:118 and :126, PareGraph.cpp, and the test sweep -- keep compiling unchanged and get byte-identical codegen. -Wunused-parameter is on, which rules out growing every caller an unused parameter instead. Four sites had to agree and do: the fill in snapshot_remote_endpoints, pack_cross_rank_derivative_payload_impl, the apply pass, and the resize. The MPI layout stays dense in the world (layout.displs[rank], recv_displs[rank]) -- only the snapshot moved. The resize is grow-only. The dense form was resize(R) with R fixed per communicator, so it never freed a buffer; shrinking to a thinner layer's occupancy would free the allocations this scratch exists to reuse. Positions past `occupied` are never visited, because every reader walks the same sweep. HONEST SIZING: 4 x R x sizeof(std::vector<double>) is ~24 KB per thread at R=256, plus ~4R stores per gradient layer. This is thesis consistency -- no per-layer array indexed by P -- not a measurable speedup, and no speedup is claimed.
size_staging_send_ derived mpi_send_counts_[b] by summing row_send_(u)[b] over u while col_sum_, base_send_ and pack_off_ came from counts_row_(u). Both tables are written from the same pointer in Phase P0, so the two are the same integer sum: publish_send_rows_ memcpy'd send_counts into counts_row_(u) and then summed that same pointer into row_send_(u)[b]. Sum W instead. Pass A (the col_sum_ accumulation) moves ahead of the counts/displacements loop and mpi_send_counts_[b] = sum_t col_sum_[b*S+t], which leaves row_send_ with no readers, so the send half of rows_ goes: rows_stride_ drops from round_up_(2R) to round_up_(R), row_recv_ rebases onto rows_ directly, and publish_send_rows_ collapses into publish_counts_row_. The recv half stays -- fill_recv_col_from_rows_ is its live reader. Bit-identity. Old mpi_send_counts_[b] = sum_u sum_t c[u][b*S+t]; new is sum_t sum_u c[u][b*S+t]. Same integer terms, exact addition, so equal elementwise. base_send_ and pack_off_ read col_sum_ exactly as before and are untouched; the staging block order stays t ascending with u ascending inside each t. No floating-point accumulation is involved. Order preserved: the accumulator is long long, so the same checked_mpi_count(long long, ...) overload and the same "Per-rank send count" string are selected, and the throw order is still per-rank b ascending, then displacements, then total. Pass A cannot throw, so hoisting it is unobservable. Access pattern, as a side benefit: the replaced loop ran b outer / u inner and strode rows_stride_ per inner step, whereas sum_t col_sum_[b*S+t] is a contiguous S-run of partition 0's own freshly written array.
pack_send_ took each block's LENGTH from the caller's live slots_[u].send_counts but its OFFSET from pack_off_, which is built from the pre-B1 counts_row_(u) snapshot. Read both from the snapshot. Not a bug on either side. publish_counts_row_ memcpy'd the caller's array into counts_row_(u) before B1 and the owner's thread is blocked inside the verb from before P0 until after B4, so it cannot mutate its own arguments in between: the two reads return the same values, and the peer-visible window is if anything narrower than main's, which read every peer's LIVE send_counts on partition 0 inside B1->B2. Same contiguity in g, and partition u reading its own row is the same ownership rule that makes the P0 write safe. Slot::send_counts then has no readers, so the field and its two writes go; send_displs stays, and Slot is alignas(64) so sizeof is unchanged. pack_send_ is now the second reader of counts_matrix_, so the Phase P0 LIFETIME note is amended: partition 0 remains the only CROSS-partition reader, and partition u's own read in B2->B3 is one thread reading what it wrote, unable to rewrite the row before verb k+1's P0, past B4. No offset, count or block order changes: pack_off_, base_send_ and base_recv_ are untouched and the staged message to rank b still runs t ascending with u ascending inside each t.
Pass B's accumulator ends elementwise equal to col_sum_: run_[g] after the
loop is sum_{u<S} counts_row_(u)[g], which is pass A's definition of
col_sum_[g]. And col_sum_ is dead where pass B starts -- its only readers
are its own accumulation, the per-rank send-count loop and the base_send_
loop, and the last of those enumerates every g in [0,P) exactly once and
closes before pass B's fill. So re-zero col_sum_ in place of run_'s fill,
run the prefix in it, and delete the member and its resize. Nothing else
in the tree referenced run_.
Bit-identity: pack_off_(u,g) = base_send_[g] + sum_{u'<u} c[u'][g] with the
same u-outer/g-inner traversal and the same integer adds; only the storage
holding the running prefix changed. base_send_, base_recv_ and the block
order (t ascending, u ascending within t) are untouched.
Sizing, honestly: this is a tidy-up, not a speedup. It removes one
P-element long long vector -- 4 KiB at P=512 -- and pass B still touches
four arrays either way.
Depends on the col_sum_ hoist in the previous commit, which moves pass A
above the checked counts and still leaves it before pass B's fill.
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
|
Collaborator
Author
|
🤖 AI text below 🤖 Correctness gate on
|
| Suite | Result |
|---|---|
ctest -L unit |
235 / 235 |
ctest -L serial |
234 / 234 |
ctest -L serial (MPI build) |
234 / 234 |
ctest -L mpi / mpi-2 |
1 / 1 |
| Python suite, 4 layouts | 593 passed × 24 runs |
Python layouts: ranks=2/partitions=1 (world 2), ranks=2/partitions=16 (world 32),
ranks=4/partitions=8 (world 32), ranks=16/partitions=16 (world 256). Verdicts
BOTH GATES PASSED and ALL SUITES PASSED; zero FAILED, zero BAD_COMMAND, zero
Errors while running CTest.
The unit and serial counts moved 231/230 → 235/234 across the two main merges, which is #267 adding
one case and #268 adding four. They track what main added, which is the check that the merges brought
in test coverage rather than displacing it.
Slurm jobs 1846481 (build) → 1846482 (unit + serial) → 1846483 (MPI + Python), run strictly in
sequence. An earlier attempt ran the two suites concurrently against one worktree and produced 14
spurious BAD_COMMAND failures — ctest-worktree.sh defaults SYNC=1, so its uv sync rebuilt the
editable tree the other job was executing from. Those were not test failures and that run is not the
one reported here.
What this gate does not cover. It is correctness only. The timing and memory tables in the
description were measured at e25bbfb and have not been re-measured since; the benchmark harness
records time and memory and cannot detect a wrong answer, so it is not evidence either way. Nothing
here re-establishes bit-identity, which this PR does not claim.
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
2 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 scales with the flat world
P = ranks x partitions x nodes.45 commits on
638ee6f(43 changes + 2mainmerges), 25 files +1280/-554. Supersedes #238.CrossRankPartnerRangewas dense overPat 32 B narrow/ 40 B wide;
CrossRankOccupiedSlotis 12 B / 24 B and exists only where traffic does, offsets arunning prefix. One such array per layer made an
O(P)arrayO(P^2)per job.O(P + occupied)into per-thread scratch.RecvLayout.h/resolve_recvare gone: the count matrix is symmetric, so the recv layout is thesend layout. That also removes a hazard —
resolve_recv's cache could skip a collective on a hit,letting ranks disagree about whether it happened; nothing in the replacement can split ranks.
resolve_recv's unconditional width precondition is not lost with it: it survives asmpi::check_exchange_layout_width, on the path every exchange takes.HybridCommstaging matrix is re-indexed peer-outermost, two owner-written tables publishbefore barrier B1,
scatter_off_becomes a cursor. Barriers stay at 4 per payload verb and theoffset tables are elementwise equal — reassociated integer sums, not a reordered exchange.
Partition 0 stays
Theta(R*S^2): this is not a fix for the funnel.ContractSink::finalizereturnsnullptr, sopropagatebuilds noLayerCore— its win is entirelymechanism 3, while
build_graph/energy/gradientare the slots.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 actually is, for anyone coming to this cold:
resolve_recvis onlysafe if the exchange count matrix is symmetric, so that a rank can derive what it will receive from
what its peers send. If that is wrong anywhere, ranks disagree about a collective and the failure is
a hang or silent corruption, not a failed assert.
cpp/tests/ExchangeLayoutOracle.hchecks thederivation independently and deliberately lives outside the library.
HybridComm.h(mechanism 3) is the hardest file to review and the one where a reordering thatlooks equivalent may not be. The claim is that barrier count is unchanged (4 per payload verb) and
the offset tables are elementwise equal; the reassociation is of integer sums only.
find()is nowstd::ranges::lower_boundover a sorted array. Itsprecondition is that
occupiedstays sorted byslot; a builder that appends out of order wouldbreak lookup silently rather than loudly.
Not established by this PR, and not claimed:
bitident-pair.shneeds each arm's ownbenches/_builders.pyandmainmoved those intopackages/monoprop-bench-toolsin refactor(bench): 📦 release the benchmark harness as monoprop-bench-tools #227.evidence for this PR is the test suites, not the tables.
Changes since the first review round
The author's own review left 34 inline comments; commits
e0f813a..5edeb53address all of them.None changes behaviour. Summarised so an external reviewer does not have to reconstruct it from the
threads:
monoprop_CHECK_EXCHANGE_SYMMETRYand its runtime audit are gone, with the CMake option, thecompile definition, the guarded test case and the documentation of all of it. The symmetry is a
property of how the counts are built, not a runtime risk. Only the audit went: the width
precondition described above is unconditional and stays.
boost-test.cmake's rank-list guard is gone. It could not fire from any configuration checkedinto the repo — every non-default
monoprop_MPI_TEST_PROCSin the tree is a shell variable consumedfor
mpiexec -n, never a CMake cache variable — so it took a hand-passed-Dto reach. Its commentalso said "nine cases" where 11 early-return at world size 1.
std::logic_errorthrows becameCrossRankSlotLayoutError, derived fromlogic_errorso theexisting catches still hold. The codebase has 15 dedicated exception types; the generics were the
outlier.
find()onstd::ranges::lower_bound,autoremoved from anMPI_Request, oneconst doubletoconst auto, a 27-line lambda split intotwo named helpers, and one by-value
std::vectorparameter toconst&— that one removes amove-construct, since the
std::moveon it was already a no-op against aconst&callee.only shouted words left in the diff are Apache licence boilerplate). Density over added C++ lines
falls 21.3% -> 14.9%, counting leading
//against non-blank code, licence headers excluded. Thisdid not reach the under-10% originally aimed at; what remains is mostly barrier and lifetime rules in
HybridComm.h. Note for anyone reading the threads: commitf97a09dand several replies quote"264 -> 183" and "21.8% -> 16.3%", from a counter that also caught
#includelines. The figures inthis paragraph are the correct ones.
AGENTS.md,building.mdx,testing.mdxand the rootREADME.mdliterally, and two files partially:parallelism.mdxkeeps theO(P^2)graph-memorysection and loses only the flag lines, and
cpp/tests/README.mdkeeps the bullets describing twofiles this PR adds, which a full revert would leave undocumented.
layercorecpp:S5414suppression is dropped fromsonar-project.properties: it wasjustified by a private cache member this PR deletes, and the header now has no private members.
Measurement
One Slurm allocation per cell holds both arms, arm order flipped per
(rep, cell), 10 reps; ratiosformed per rep then medianed, judged by a two-sided sign test with Holm step-down within each family.
Peak RSS is
VmHWMfrom/usr/bin/time -v, summed over nodes. Layout A = 1 rank x 128 partitions,B = 8 ranks x 16 partitions, each at 1 and 2 nodes.
Wall time, 12 cells — Holm across the 24 tests in this family.
build_graph[hubbard]propagate[hubbard]build_graph[pauli]energy[pauli]gradient[pauli]propagate[pauli]build_graph[hubbard]propagate[hubbard]build_graph[pauli]energy[pauli]gradient[pauli]propagate[pauli]build_graph[hubbard]propagate[hubbard]build_graph[pauli]energy[pauli]gradient[pauli]propagate[pauli]build_graph[hubbard]propagate[hubbard]build_graph[pauli]energy[pauli]gradient[pauli]propagate[pauli]Peak RSS, node sum — Holm across the 16 tests in this family.
hubbard-freshhubbard-freshpauli-freshpauli-graphhubbard-freshhubbard-freshpauli-freshpauli-graphhubbard-freshhubbard-freshpauli-freshpauli-graphhubbard-freshhubbard-freshpauli-freshpauli-graphVerdict. 15 of 24 timing tests resolve, all 15 improvements, 0 regressions, at the 10-rep
sign-test floor (raw
p = 0.0020, Holm-adjusted0.0469), split A=8 / B=7 so this is not one layout.Memory is Holm-corrected separately over its own 16 tests because the harness corrects only timing:
12 resolve at adjusted
p = 0.0313, 10 improvements against 2 regressions of +0.3% and +1.1%. Largestsingle result is peak RSS on pauli
energy+gradient, 2 nodes, layout A: 0.3966x, a 2.52xreduction, 36.70 -> 14.56 GiB.
Caveats.
hubbard-freshB memory cellssit at 5/10 and 6/10, which is indistinguishable, not a small win.
fresh) / 0.7817(
graph) against wall 0.6062 / 0.5801, so roughly a fifth of the win is work removed and the restde-serialisation.
path, not shown by a bisect.
e25bbfb, before the review round and before twomainmerges. Thecommits since are non-functional by the argument above, not by re-measurement. Read the tables as a
result about
e25bbfb; the correctness gate below is what covers the current head.Gate on the current head
Re-run on
5edeb53after the merge ofmainat638ee6f, because #267 and #268 both touch files thisPR changes:
ctest -L unitand-L serial, and-L mpi/mpi-2Counts and the tested binary's md5 are posted in a comment on this PR rather than inlined here, so the
body does not drift from what was actually 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 own campaigns.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.