From 13eb46234b01ad0e552982f1822a4f680da7da18 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Wed, 29 Jul 2026 01:23:44 +0200 Subject: [PATCH 01/17] =?UTF-8?q?feat(mpi):=20=F0=9F=93=8A=20opt-in=20per-?= =?UTF-8?q?partition=20profile=20for=20the=20partitioned=20collectives?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit monoprop_COMM_PROFILE=1 accumulates, per partition and per cache-line-isolated slot, where a collective's wall time goes: table_p0 (fills one partition runs alone) vs table_par (fills every partition runs on its own slice) vs table_move (payload memcpy) vs mpi vs barrier wait, plus the barrier's L3-domain group count. Off by default and never allocated then, so the hot path pays one null check per instrumented region. The splits are the ones that decide protocol questions. table_p0 vs table_par separates a serial protocol from a parallel one, and attributing barrier wait per partition is what makes the asymmetry visible -- the master's own wait stays small precisely when the master is the bottleneck. table_par vs table_move separates bookkeeping, which a better protocol shrinks, from data movement, which it cannot. Assisted-by: ClaudeCode:claude-opus-5 --- src/monoprop/detail/EnvConfig.h | 3 + src/monoprop/detail/mpi/CommProfile.h | 134 ++++++++++++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 src/monoprop/detail/mpi/CommProfile.h diff --git a/src/monoprop/detail/EnvConfig.h b/src/monoprop/detail/EnvConfig.h index 5fae134a..67f4fe40 100644 --- a/src/monoprop/detail/EnvConfig.h +++ b/src/monoprop/detail/EnvConfig.h @@ -24,6 +24,7 @@ // monoprop_NUM_THREADS positive int (1..1e6), else ignored → num_threads // monoprop_PARTITION_PINNING bool, default ON; 0/false disables per-core pinning → partition_pinning // monoprop_PARTITIONS int N | "auto" | "off"; parsed where it is used (resolve_partition_count_) +// monoprop_COMM_PROFILE bool, default OFF; per-partition collective accounting to stderr → comm_profile namespace monoprop::config { @@ -57,6 +58,7 @@ inline auto parse_positive_int(const char *text) -> std::optional { struct Settings { std::optional num_threads; bool partition_pinning = true; + bool comm_profile = false; }; // Parse the environment once; the Settings are cached and shared across TUs. @@ -65,6 +67,7 @@ inline auto get() -> const Settings & { Settings s; s.num_threads = detail::parse_positive_int(std::getenv("monoprop_NUM_THREADS")); s.partition_pinning = detail::parse_flag(std::getenv("monoprop_PARTITION_PINNING"), true); + s.comm_profile = detail::parse_flag(std::getenv("monoprop_COMM_PROFILE"), false); return s; }(); return settings; diff --git a/src/monoprop/detail/mpi/CommProfile.h b/src/monoprop/detail/mpi/CommProfile.h new file mode 100644 index 00000000..1575a04a --- /dev/null +++ b/src/monoprop/detail/mpi/CommProfile.h @@ -0,0 +1,134 @@ +// Copyright 2026 Algorithmiq +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include +#include +#include +#include +#include +#include + +// Opt-in accounting for where a partitioned collective's wall time goes (monoprop_COMM_PROFILE=1). Off by +// default and never allocated then, so the hot path pays one null check per instrumented region. +// +// The split that matters is table_p0 vs table_par: the offset/count tables a HybridComm collective +// rebuilds are O(R*S^2), and whether one partition fills them alone (table_p0, the other S-1 parked in a +// barrier) or every partition fills its own slice (table_par) is the difference between a serial and a +// parallel protocol. Barrier wait is attributed per partition to make that asymmetry visible. table_move +// separates payload memcpy, which a better protocol does not shrink, from bookkeeping, which it does. + +namespace monoprop::mpi { + +class CommProfile { +public: + using Clock = std::chrono::steady_clock; + + // One cache-line-isolated accumulator per partition: every counter is written only by its own + // partition thread, so profiling adds no coherence traffic of its own to what it measures. + struct alignas(64) Slot { + uint64_t barrier_ns = 0; + uint64_t table_p0_ns = 0; // fills executed under `if (local_partition == 0)` + uint64_t table_par_ns = 0; // fills every partition runs on its own slice + uint64_t table_move_ns = 0; // payload memcpy (pack into / scatter out of staging), not bookkeeping + uint64_t mpi_ns = 0; // time inside MPI itself + uint64_t n_barriers = 0; + uint64_t n_verbs = 0; + }; + + explicit CommProfile(int n_partitions, int mpi_rank) + : slots_(static_cast(n_partitions)), + mpi_rank_(mpi_rank) {} + + // L3 domains the transport's barrier grouped its partitions into (< 2 ⇒ the flat barrier ran). + int barrier_groups = 0; + + auto slot(int partition) -> Slot & { return slots_[static_cast(partition)]; } + + // Aggregate over partitions, plus partition 0 broken out. Written to stderr at teardown, one + // block per MPI rank; a driver greps `COMMPROF` and sums/compares across ranks. + // + // noexcept because the only caller is a transport destructor, which may run while an exception + // unwinds: a diagnostic print that fails must be dropped, never escalated to a terminate. + auto dump() const noexcept -> void { + try { + dump_(); + } + catch (...) { // std::print can throw (formatting, or a write error on stderr) + } + } + +private: + auto dump_() const -> void { + Slot total; + for (const Slot &s : slots_) { + total.barrier_ns += s.barrier_ns; + total.table_p0_ns += s.table_p0_ns; + total.table_par_ns += s.table_par_ns; + total.table_move_ns += s.table_move_ns; + total.mpi_ns += s.mpi_ns; + total.n_barriers += s.n_barriers; + total.n_verbs += s.n_verbs; + } + const Slot &p0 = slots_.front(); + // Peer barrier wait is the cost of the master's serial phases seen from the other side. + const uint64_t peer_barrier_ns = total.barrier_ns - p0.barrier_ns; + std::print(stderr, + "COMMPROF rank={} partitions={} barrier_groups={} verbs={} barriers={} " + "table_p0_s={:.3f} table_par_s={:.3f} table_move_s={:.3f} mpi_s={:.3f} " + "barrier_p0_s={:.3f} barrier_peers_s={:.3f} barrier_per_sync_us={:.2f}\n", + mpi_rank_, + slots_.size(), + barrier_groups, + p0.n_verbs, + p0.n_barriers, + to_s(p0.table_p0_ns), + to_s(total.table_par_ns) / static_cast(slots_.size()), + to_s(total.table_move_ns) / static_cast(slots_.size()), + to_s(p0.mpi_ns), + to_s(p0.barrier_ns), + to_s(peer_barrier_ns) / static_cast(slots_.size() > 1 ? slots_.size() - 1 : 1), + total.n_barriers == 0 + ? 0.0 + : (static_cast(total.barrier_ns) / static_cast(total.n_barriers)) / 1000.0); + std::fflush(stderr); + } + + static auto to_s(uint64_t ns) -> double { return static_cast(ns) / 1e9; } + + std::vector slots_; + int mpi_rank_; +}; + +// Adds the scope's duration to one counter. Held by value in the instrumented region; `target` must +// outlive it (it is a field of the comm's own CommProfile). +class ScopedNs { +public: + explicit ScopedNs(uint64_t *target) : target_(target), start_(CommProfile::Clock::now()) {} + ScopedNs(const ScopedNs &) = delete; + auto operator=(const ScopedNs &) -> ScopedNs & = delete; + ~ScopedNs() { + if (target_ != nullptr) { + *target_ += static_cast( + std::chrono::duration_cast(CommProfile::Clock::now() - start_).count()); + } + } + +private: + uint64_t *target_; + CommProfile::Clock::time_point start_; +}; + +} // namespace monoprop::mpi From fef1f051fcea9cd183d717ee486b9204dbebfb95 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Wed, 29 Jul 2026 01:24:03 +0200 Subject: [PATCH 02/17] =?UTF-8?q?perf(mpi):=20=E2=9A=A1=EF=B8=8F=20remove?= =?UTF-8?q?=20the=20serial=20O(R*S^2)=20floor=20from=20HybridComm?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A partitioned multi-node run was protocol-bound, not network-bound: every collective rebuilt O(R*S^2) integer offset tables and partition 0 filled all of them alone while the other S-1 partitions spun at a barrier. Measured with monoprop_COMM_PROFILE=1 (2 nodes, 512-site Hubbard, 4 layers, R=2, S=112), partition 0's serial fill was 22.33 s of a 26.74 s run, while MPI itself was 1.58 s -- the wire was never the problem. Four changes, wall 26.74 -> 4.46 s (6.0x), expectation value bit-identical at every step and at every rank x partition split: - Parallel prefix. The offset of block (rank b, dest t, source u) splits into a per-(b,t) base needing global knowledge, which is only O(R*S) and stays on partition 0, and a scan over u that partition t owns outright. The count matrix is relaid source-partition-major so each partition writes a contiguous run; dest-major would put S partitions on every cache line and trade a serial fill for pure false sharing. - alltoallv_reverse. The answer leg travels the query exchange's legs backwards, so its geometry is the query round's, and rebuilding it costs 3*R*S^2 entries and a barrier for nothing. It is a ratio and not an identity: the query leg carries Sink::kStride elements per record and the answer leg one, so every reused offset and per-rank count is divided by the stride (exact -- every forward count is a multiple of it). Reusing them undivided would still deliver correct data while staging and transmitting kStride times the bytes. - Empty-block veto. Each partition publishes a cache-line-padded bitmask of the ranks it sends anything to; the sizing phase then runs source-partition-outer, so a partition that sends nothing costs one load instead of R strided probes into its count array. At early layers nearly every block is empty. - Two-level barrier. PartitionBarrier fans in within an L3 domain and then across domains, so both the arrival fetch_add and the release-store invalidation cost O(S/G) coherence transactions inside one L3 slice instead of O(S) across the socket interconnect. Domains are derived from the partition cpusets rather than from the placement logic, so the two cannot drift apart, and a rank spanning one domain keeps the flat barrier (a root barrier of one is pure overhead). Tests: the pre-existing hybrid_comm cases all sent the same count to every destination per source, so a transposed (b,t,u) index passed unnoticed -- two asymmetric-count cases close that. partition_barrier_tests.cpp tests the two-level path directly, because engaging it through a real comm needs pinning and >=2 L3 domains, which no test host can be relied on to have. Both additions are mutation-verified: an inconsistent receiver index and a skipped root barrier each fail the new cases while the old ones pass. Assisted-by: ClaudeCode:claude-opus-5 --- .../detail/evolution/layer_build/Engine.h | 11 +- src/monoprop/detail/mpi/HybridComm.h | 599 ++++++++++++++---- src/monoprop/detail/mpi/MPICompat.h | 41 +- src/monoprop/detail/mpi/PartitionBarrier.h | 123 +++- src/monoprop/detail/mpi/ShmComm.h | 162 +++-- src/monoprop/detail/partition/CpuTopology.h | 29 + .../detail/partition/PartitionGroup.h | 13 +- tests/cpp/hybrid_comm_tests.cpp | 128 ++++ tests/cpp/partition_barrier_tests.cpp | 161 +++++ 9 files changed, 1055 insertions(+), 212 deletions(-) create mode 100644 tests/cpp/partition_barrier_tests.cpp diff --git a/src/monoprop/detail/evolution/layer_build/Engine.h b/src/monoprop/detail/evolution/layer_build/Engine.h index 63335a35..ce6e7dc3 100644 --- a/src/monoprop/detail/evolution/layer_build/Engine.h +++ b/src/monoprop/detail/evolution/layer_build/Engine.h @@ -381,7 +381,16 @@ struct LayerBuildEngine { auto resp = resolve_incoming(inc_q, local_op, R, is_leader_pass, matched, combined_size, sink); std::vector resp_recv = response_recv_counts(); std::vector> inc_r; - mpi::begin_alltoallv(resp, comm, /*skip_self=*/false, &resp_recv).wait_into(inc_r); + // The answers travel the query exchange's legs backwards, one per query, so the hybrid transport + // reuses that exchange's offset tables; nothing may collectively intervene between the two calls. + // Sink::kStride is the query leg's words per query, the ratio between the two legs' counts. + mpi::begin_alltoallv(resp, + comm, + /*skip_self=*/false, + &resp_recv, + /*reverse_of_previous=*/true, + /*forward_stride=*/static_cast(Sink::kStride)) + .wait_into(inc_r); process_responses(inc_r, src_idx_r, queries_r, R, my_rank, sink); } diff --git a/src/monoprop/detail/mpi/HybridComm.h b/src/monoprop/detail/mpi/HybridComm.h index 71a61a38..a01929d8 100644 --- a/src/monoprop/detail/mpi/HybridComm.h +++ b/src/monoprop/detail/mpi/HybridComm.h @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -29,6 +30,8 @@ #include +#include "monoprop/detail/EnvConfig.h" +#include "monoprop/detail/mpi/CommProfile.h" #include "monoprop/detail/mpi/PartitionBarrier.h" // Composes R MPI ranks x S in-process partitions into one flat P=R*S SPMD world. Global id is rank-major @@ -42,11 +45,13 @@ namespace monoprop::mpi { class HybridComm { public: // n_local_partitions = S, identical on every rank (the facade ctor checks that before constructing). - HybridComm(MPI_Comm parent, int n_local_partitions) + // partition_l3_domains (optional, one entry per partition) makes the barrier two-level; see + // PartitionBarrier. + HybridComm(MPI_Comm parent, int n_local_partitions, const std::vector &partition_l3_domains = {}) : parent_(parent), s_(n_local_partitions), slots_(static_cast(n_local_partitions)), - barrier_(n_local_partitions) { + barrier_(n_local_partitions, partition_l3_domains) { MPI_Comm_size(parent_, &r_); MPI_Comm_rank(parent_, &mpi_rank_); int provided = MPI_THREAD_SINGLE; @@ -66,11 +71,35 @@ class HybridComm { mpi_recv_displs_.resize(static_cast(r_)); pack_off_.resize(rss); scatter_off_.resize(rss); + const size_t rs = static_cast(r_) * static_cast(s_); + col_send_.resize(rs); + col_recv_.resize(rs); + tpre_send_.resize(rs); + tpre_recv_.resize(rs); + rev_send_counts_.resize(static_cast(r_)); + rev_send_displs_.resize(static_cast(r_)); + rev_recv_counts_.resize(static_cast(r_)); + rev_recv_displs_.resize(static_cast(r_)); + mask_words_ = pad_to_line_(static_cast((r_ + 63) / 64)); + send_mask_.assign(static_cast(s_) * mask_words_, 0); + run_stride_ = pad_to_line_(static_cast(r_)); + run_scratch_.assign(static_cast(s_) * run_stride_, 0); + if (config::get().comm_profile) { + prof_ = std::make_unique(s_, mpi_rank_); + prof_->barrier_groups = barrier_.group_count(); + } } HybridComm(const HybridComm &) = delete; auto operator=(const HybridComm &) -> HybridComm & = delete; + // Profiling is opt-in, so the common case destroys nothing and prints nothing. + ~HybridComm() { + if (prof_ != nullptr) { + prof_->dump(); + } + } + auto size() const -> int { return r_ * s_; } auto global_rank(int local_partition) const -> int { return mpi_rank_ * s_ + local_partition; } @@ -137,6 +166,32 @@ class HybridComm { }); } + // Payload-only reverse of the immediately preceding alltoallv_resolve: same legs, opposite + // direction, one element back per element received. See alltoallv_reverse_impl_ for the contract. + auto alltoallv_reverse(int local_partition, + const void *send, + const int *send_counts /*[P]*/, + const int *send_displs /*[P]*/, + void *recv, + const int *recv_counts /*[P]*/, + const int *recv_displs /*[P]*/, + size_t elem, + MPI_Datatype dt, + int forward_stride) -> void { + guard_partition0_(local_partition, "alltoallv_reverse", [&] { + alltoallv_reverse_impl_(local_partition, + send, + send_counts, + send_displs, + recv, + recv_counts, + recv_displs, + elem, + dt, + forward_stride); + }); + } + template auto allreduce_sum(int local_partition, T local_val) -> T { return guard_partition0_(local_partition, "allreduce_sum", [&] { @@ -150,34 +205,32 @@ class HybridComm { }); } - // recv_counts[g] = amount global partition g sends to this partition. 2 barriers + one S*S-int MPI_Alltoall. + // recv_counts[g] = amount global partition g sends to this partition. 2 barriers + one S*S-int + // MPI_Alltoall; the count matrix is filled row-per-partition before the first barrier. auto alltoall_counts_impl_(int local_partition, const int *send_counts /*[P]*/, int *recv_counts /*[P]*/) -> void { - const size_t u = static_cast(local_partition); - slots_[u].counts = send_counts; - sync(); + count_verb(local_partition); if (local_partition == 0) { - // Pack the S*S count matrix per dest rank, dest-partition-major (t) then source-partition-minor (su). - // Every element is written here and MPI_Alltoall fills counts_recv_ fully, so neither is pre-zeroed. - for (int b = 0; b < r_; ++b) { - for (int t = 0; t < s_; ++t) { - for (int su = 0; su < s_; ++su) { - const size_t idx = ((static_cast(b) * static_cast(s_)) + static_cast(t)) - * static_cast(s_) - + static_cast(su); - counts_send_[idx] = slots_[static_cast(su)].counts[b * s_ + t]; - } - } - } + ++tables_gen_; + } + // Each partition writes its OWN source row of the count matrix before the barrier that publishes + // it; the rows are disjoint, so this costs no extra barrier. Every element is written here and + // MPI_Alltoall fills counts_recv_ fully, so neither buffer is pre-zeroed. + { + ScopedNs timer{par_table_ns(local_partition)}; + fill_count_row_(local_partition, send_counts); + } + sync(local_partition); + if (local_partition == 0) { + ScopedNs timer{mpi_ns(local_partition)}; MPI_Alltoall(counts_send_.data(), s_ * s_, MPI_INT, counts_recv_.data(), s_ * s_, MPI_INT, parent_); } - sync(); - // Partition t extracts its row: recv from (rank a, partition su) is contiguous per source rank a. + sync(local_partition); + // Partition t extracts its column: what (rank a, partition su) sends to it. const int t = local_partition; + ScopedNs timer{par_table_ns(local_partition)}; for (int a = 0; a < r_; ++a) { for (int su = 0; su < s_; ++su) { - const size_t idx = (static_cast(a) * static_cast(s_) * static_cast(s_)) - + (static_cast(t) * static_cast(s_)) + static_cast(su); - recv_counts[a * s_ + su] = counts_recv_[idx]; + recv_counts[a * s_ + su] = counts_recv_[cnt_idx_(a, su, t)]; } } // No trailing barrier: past the last sync only counts_recv_ is read, and partition 0 cannot rewrite @@ -197,25 +250,36 @@ class HybridComm { size_t elem, MPI_Datatype dt) -> void { const size_t u = static_cast(local_partition); + count_verb(local_partition); + if (local_partition == 0) { + ++tables_gen_; + } Slot &me = slots_[u]; me.ptr = send; me.send_counts = send_counts; me.send_displs = send_displs; me.recv_counts = recv_counts; - sync(); // B1 - - // B2: partition 0 sizes/reallocates staging; must finish before any partition packs into stage_send_. - if (local_partition == 0) { - size_staging_(elem); + // No count matrix on this path, so the veto row phase A reads is filled on its own. + { + ScopedNs timer{par_table_ns(local_partition)}; + fill_send_mask_(local_partition, send_counts); } - sync(); // B2 + sync(local_partition); // B1 - // B3: each partition packs its own cross-rank blocks into stage_send_ (disjoint writes). - pack_send_(local_partition, elem); - sync(); // B3 + // Sizing is S-way parallel and carries its own two barriers (see size_staging_parallel_); on + // return every base is visible and staging is grown, so packing into stage_send_ is safe. + size_staging_parallel_(local_partition, elem); + + // Each partition packs its own cross-rank blocks into stage_send_ (disjoint writes). + { + ScopedNs timer{move_ns(local_partition)}; + pack_send_(local_partition, elem); + } + sync(local_partition); // B3 - // B4: partition 0 runs the single MPI_Alltoallv while peers park at the barrier. + // Partition 0 runs the single MPI_Alltoallv while peers park at the barrier. if (local_partition == 0) { + ScopedNs timer{mpi_ns(local_partition)}; MPI_Alltoallv(stage_send_.data(), mpi_send_counts_.data(), mpi_send_displs_.data(), @@ -226,10 +290,12 @@ class HybridComm { dt, parent_); } - sync(); // B4 + sync(local_partition); // B4 // Scatter each global source's contiguous run from stage_recv_ to recv_displs[g] (all legs, incl. - // self-rank, go through staging). Block starts come from scatter_off_: no peer slot is read past B4. + // self-rank, go through staging). Block starts come from the offset tables, so no peer slot is + // read after the last barrier. + ScopedNs timer{move_ns(local_partition)}; char *dst = static_cast(recv); const int t = local_partition; for (int a = 0; a < r_; ++a) { @@ -238,12 +304,12 @@ class HybridComm { const int cnt = recv_counts[g]; if (cnt != 0) { std::memcpy(dst + static_cast(recv_displs[g]) * elem, - stage_recv_.data() + scatter_off_[block_idx_(a, t, su)] * elem, + stage_recv_.data() + recv_block_off_(a, t, su) * elem, static_cast(cnt) * elem); } } } - // No trailing barrier (see alltoall_counts_impl_): past B4 only stage_recv_/scatter_off_/own buffers. + // No trailing barrier (see alltoall_counts_impl_): past B4 only stage_recv_/offset tables/own buffers. } // Fused count-resolve + payload alltoallv: folds the standalone count exchange into this verb's @@ -260,49 +326,57 @@ class HybridComm { size_t elem, MPI_Datatype dt) -> void { const size_t u = static_cast(local_partition); + count_verb(local_partition); + if (local_partition == 0) { + ++tables_gen_; + } Slot &me = slots_[u]; me.ptr = send; me.send_counts = send_counts; me.send_displs = send_displs; - // recv_counts is an output here — deliberately not published; the count Alltoall resolves it. - sync(); // B1 + // recv_counts is an output here — deliberately not published; the count Alltoall resolves it. The + // count row comes from the local argument, so it rides the same barrier as the slot publish. + { + ScopedNs timer{par_table_ns(local_partition)}; + fill_count_row_(local_partition, send_counts); + } + sync(local_partition); // B1: slots and the whole send-count matrix published if (local_partition == 0) { - for (int b = 0; b < r_; ++b) { - for (int t = 0; t < s_; ++t) { - for (int su = 0; su < s_; ++su) { - counts_send_[block_idx_(b, t, su)] = slots_[static_cast(su)].send_counts[b * s_ + t]; - } - } - } + ScopedNs timer{mpi_ns(local_partition)}; MPI_Alltoall(counts_send_.data(), s_ * s_, MPI_INT, counts_recv_.data(), s_ * s_, MPI_INT, parent_); - // Size staging from counts_recv_: recv of partition t from (rank, su) sits at rank*S*S + t*S + su. - size_staging_impl_(elem, [this](int t, int rank, int su) { - return counts_recv_[(static_cast(rank) * static_cast(s_) * static_cast(s_)) - + (static_cast(t) * static_cast(s_)) + static_cast(su)]; - }); } - sync(); // B2 + sync(local_partition); // B2: counts_recv_ visible to every partition + + // Size staging from counts_recv_: what partition t gets from (rank, su) is at cnt_idx_(rank, su, t). + // Carries its own two barriers and its own timers — do not wrap it in one. + size_staging_parallel_(local_partition, elem, [this](int t, int rank, int su) { + return counts_recv_[cnt_idx_(rank, su, t)]; + }); const int t = local_partition; - long long total = 0; - for (int a = 0; a < r_; ++a) { - for (int su = 0; su < s_; ++su) { - const int g = a * s_ + su; - const int c = - counts_recv_[(static_cast(a) * static_cast(s_) * static_cast(s_)) - + (static_cast(t) * static_cast(s_)) + static_cast(su)]; - recv_counts[g] = c; - recv_displs[g] = checked_int_(total); - total += c; + { + ScopedNs timer{par_table_ns(local_partition)}; + long long total = 0; + for (int a = 0; a < r_; ++a) { + for (int su = 0; su < s_; ++su) { + const int g = a * s_ + su; + const int c = counts_recv_[cnt_idx_(a, su, t)]; + recv_counts[g] = c; + recv_displs[g] = checked_int_(total); + total += c; + } } + recv.resize(static_cast(checked_int_(total))); } - recv.resize(static_cast(checked_int_(total))); - - pack_send_(local_partition, elem); - sync(); // B3 + { + ScopedNs timer{move_ns(local_partition)}; + pack_send_(local_partition, elem); + } + sync(local_partition); // B3 if (local_partition == 0) { + ScopedNs timer{mpi_ns(local_partition)}; MPI_Alltoallv(stage_send_.data(), mpi_send_counts_.data(), mpi_send_displs_.data(), @@ -313,8 +387,9 @@ class HybridComm { dt, parent_); } - sync(); // B4 + sync(local_partition); // B4 + ScopedNs scatter_timer{move_ns(local_partition)}; char *dst = reinterpret_cast(recv.data()); for (int a = 0; a < r_; ++a) { for (int su = 0; su < s_; ++su) { @@ -322,16 +397,129 @@ class HybridComm { const int cnt = recv_counts[g]; if (cnt != 0) { std::memcpy(dst + static_cast(recv_displs[g]) * elem, - stage_recv_.data() + scatter_off_[block_idx_(a, t, su)] * elem, + stage_recv_.data() + recv_block_off_(a, t, su) * elem, static_cast(cnt) * elem); } } } + if (local_partition == 0) { + reverse_ready_gen_ = tables_gen_; // this round's tables may be reversed exactly once + } // No trailing barrier: same discipline as alltoallv_impl_. } + // Reverses the immediately preceding alltoallv_resolve: every leg carries one element back per RECORD + // it delivered, so this round's geometry is that round's with the send/recv roles swapped and every + // count and offset divided by `forward_stride`, the forward leg's elements per record (a query is >= 2 + // words, its answer one value). Every forward count is a multiple of the stride, so the division is + // exact and no count exchange or sizing pass is needed -- that is the 3*R*S^2 table entries and one + // barrier saved. Reusing the offsets undivided would still route correctly but would stage and + // transmit `forward_stride` times the bytes. + // + // The reuse is legal because both ends see the same nesting: rank X ordered its query block to Y as + // (dest partition of Y major, source partition of X minor), and Y's recv view of it has that nesting + // and those counts, so Y answering in place produces the layout X expects back. + // + // CONTRACT: must be the next table-touching verb after an alltoallv_resolve on this comm, with + // send_counts/recv_counts the transpose of that round's. Checked on partition 0 via tables_gen_. + auto alltoallv_reverse_impl_(int local_partition, + const void *send, + const int *send_counts /*[P]*/, + const int *send_displs /*[P]*/, + void *recv, + const int *recv_counts /*[P]*/, + const int *recv_displs /*[P]*/, + size_t elem, + MPI_Datatype dt, + int forward_stride) -> void { + count_verb(local_partition); + if (local_partition == 0 && tables_gen_ != reverse_ready_gen_) { + throw std::runtime_error("HybridComm::alltoallv_reverse must directly follow the " + "alltoallv_resolve whose layout it reverses; another collective " + "has overwritten the offset tables since."); + } + sync(local_partition); // B1: the contract check is global before anyone reuses a table + + if (local_partition == 0) { + ScopedNs timer{p0_table_ns(local_partition)}; + // Per-rank counts/displs are the forward round's, roles swapped and scaled down by the + // stride. Kept in their own scratch so the forward tables stay intact for the scatter below. + long long send_total = 0; + long long recv_total = 0; + for (int b = 0; b < r_; ++b) { + const size_t i = static_cast(b); + rev_send_counts_[i] = mpi_recv_counts_[i] / forward_stride; + rev_recv_counts_[i] = mpi_send_counts_[i] / forward_stride; + rev_send_displs_[i] = checked_int_(send_total); + rev_recv_displs_[i] = checked_int_(recv_total); + send_total += rev_send_counts_[i]; + recv_total += rev_recv_counts_[i]; + } + grow_(stage_send_, static_cast(send_total) * elem); + grow_(stage_recv_, static_cast(recv_total) * elem); + reverse_ready_gen_ = ~0ULL; // one reverse per resolve + } + sync(local_partition); // B2: staging sized + + // Pack into the query round's RECV geometry: partition t answers (rank a, partition su) at the + // very offset that query block occupied. + { + ScopedNs timer{move_ns(local_partition)}; + const int t = local_partition; + const char *src = static_cast(send); + for (int a = 0; a < r_; ++a) { + for (int su = 0; su < s_; ++su) { + const int g = a * s_ + su; + const int cnt = send_counts[g]; + if (cnt != 0) { + const size_t off = recv_block_off_(a, t, su) / static_cast(forward_stride); + std::memcpy(stage_send_.data() + off * elem, + src + static_cast(send_displs[g]) * elem, + static_cast(cnt) * elem); + } + } + } + } + sync(local_partition); // B3 + + if (local_partition == 0) { + ScopedNs timer{mpi_ns(local_partition)}; + MPI_Alltoallv(stage_send_.data(), + rev_send_counts_.data(), // send side = the query round's recv side / stride + rev_send_displs_.data(), + dt, + stage_recv_.data(), + rev_recv_counts_.data(), // recv side = the query round's send side / stride + rev_recv_displs_.data(), + dt, + parent_); + } + sync(local_partition); // B4 + + // Scatter from the query round's SEND geometry: partition u collects the answers to the queries + // it sent, block (rank b, dest partition t) sitting where it packed that query. + ScopedNs timer{move_ns(local_partition)}; + const int u = local_partition; + char *dst = static_cast(recv); + for (int b = 0; b < r_; ++b) { + const size_t base = static_cast(b) * static_cast(s_); + for (int t = 0; t < s_; ++t) { + const int g = b * s_ + t; + const int cnt = recv_counts[g]; + if (cnt != 0) { + const size_t off = (tpre_send_[base + static_cast(t)] + pack_off_[block_idx_(b, t, u)]) + / static_cast(forward_stride); + std::memcpy(dst + static_cast(recv_displs[g]) * elem, + stage_recv_.data() + off * elem, + static_cast(cnt) * elem); + } + } + } + } + template auto allreduce_sum_impl_(int local_partition, T local_val) -> T { + count_verb(local_partition); Slot &me = slots_[static_cast(local_partition)]; if constexpr (std::is_floating_point_v) { me.f64 = static_cast(local_val); @@ -339,8 +527,9 @@ class HybridComm { else { me.u64 = static_cast(local_val); } - sync(); + sync(local_partition); if (local_partition == 0) { + ScopedNs timer{mpi_ns(local_partition)}; if constexpr (std::is_floating_point_v) { double local = 0.0; for (int s = 0; s < s_; ++s) { @@ -356,7 +545,7 @@ class HybridComm { MPI_Allreduce(&local, &red_u64_, 1, MPI_UINT64_T, MPI_SUM, parent_); } } - sync(); + sync(local_partition); T out{}; if constexpr (std::is_floating_point_v) { out = static_cast(red_f64_); @@ -371,29 +560,34 @@ class HybridComm { // In-place element-wise allreduce-sum across the flat P-world, slice-partitioned across partitions in // ascending order (bit-identical to a sequential sum). auto allreduce_sum_inplace_impl_(int local_partition, double *values, size_t len) -> void { + count_verb(local_partition); slots_[static_cast(local_partition)].vec = values; - sync(); // all inputs published + sync(local_partition); // all inputs published if (local_partition == 0) { grow_(red_vec_, len); } - sync(); // red_vec_ sized + sync(local_partition); // red_vec_ sized constexpr size_t kLine = 64 / sizeof(double); const size_t lines = (len + kLine - 1) / kLine; const size_t per = (lines + static_cast(s_) - 1) / static_cast(s_); const size_t lo = std::min(len, static_cast(local_partition) * per * kLine); const size_t hi = std::min(len, lo + per * kLine); - for (size_t k = lo; k < hi; ++k) { - double acc = 0.0; - for (int s = 0; s < s_; ++s) { - acc += slots_[static_cast(s)].vec[k]; + { + ScopedNs timer{move_ns(local_partition)}; + for (size_t k = lo; k < hi; ++k) { + double acc = 0.0; + for (int s = 0; s < s_; ++s) { + acc += slots_[static_cast(s)].vec[k]; + } + red_vec_[k] = acc; // disjoint line-rounded slices: no two partitions store to one line } - red_vec_[k] = acc; // disjoint line-rounded slices: no two partitions store to one line } - sync(); // local reduction complete + sync(local_partition); // local reduction complete if (local_partition == 0) { + ScopedNs timer{mpi_ns(local_partition)}; MPI_Allreduce(MPI_IN_PLACE, red_vec_.data(), static_cast(len), MPI_DOUBLE, MPI_SUM, parent_); } - sync(); // global result in red_vec_ + sync(local_partition); // global result in red_vec_ std::memcpy(values, red_vec_.data(), len * sizeof(double)); // No trailing barrier: red_vec_ is rewritten only inside a future verb's barriered phases. } @@ -404,7 +598,6 @@ class HybridComm { private: struct alignas(64) Slot { const void *ptr = nullptr; - const int *counts = nullptr; const int *send_counts = nullptr; const int *send_displs = nullptr; const int *recv_counts = nullptr; @@ -413,12 +606,22 @@ class HybridComm { uint64_t u64 = 0; }; - // Flat index of the (rank, dest partition, source partition) block in the R*S*S offset/count tables. + // Flat index of the (rank, dest partition, source partition) block in the R*S*S offset tables. auto block_idx_(int b, int t, int u) const -> size_t { return (static_cast(b) * static_cast(s_) + static_cast(t)) * static_cast(s_) + static_cast(u); } + // Flat index into the exchanged count matrix, SOURCE-partition-major within each rank's block, so + // partition u owns the contiguous run [cnt_idx_(b,u,0), cnt_idx_(b,u,s_)) and fills it itself: the + // O(R*S^2) transpose partition 0 used to run alone becomes R*S contiguous writes per partition with + // no false sharing. Dest-partition-major would put s_ partitions on every cache line instead. + // MPI_Alltoall is indifferent: `b` stays outermost, and both sides index through this one helper. + auto cnt_idx_(int b, int u, int t) const -> size_t { + return (static_cast(b) * static_cast(s_) + static_cast(u)) * static_cast(s_) + + static_cast(t); + } + template static auto grow_(V &v, size_t need) -> void { if (v.size() < need) { @@ -426,63 +629,154 @@ class HybridComm { } } - // Partition 0 only; recv counts come from partition t's published recv_counts. - auto size_staging_(size_t elem) -> void { - size_staging_impl_(elem, [this](int t, int rank, int su) { - return slots_[static_cast(t)].recv_counts[rank * s_ + su]; - }); + // Round a row of 8-byte entries up to whole cache lines, so one partition's row never shares a line + // with a peer's. + static auto pad_to_line_(size_t entries) -> size_t { + constexpr size_t kPerLine = 64 / 8; + return ((entries + kPerLine - 1) / kPerLine) * kPerLine; } - // recv_count(t, rank, su) yields the count partition t on this rank receives from (rank, source partition su). - template - auto size_staging_impl_(size_t elem, RecvCountFn recv_count) -> void { + // Partition u's veto row: bit b set iff u sends at least one element to rank b. `mask_words_` is the + // padded row stride; only the first ceil(R/64) words carry bits. + auto mask_row_(int u) -> uint64_t * { return send_mask_.data() + static_cast(u) * mask_words_; } + static auto mask_test_(const uint64_t *row, int b) -> bool { + return (row[static_cast(b) >> 6] & (1ULL << (static_cast(b) & 63U))) != 0; + } + auto mask_empty_(const uint64_t *row) const -> bool { + const size_t live = static_cast((r_ + 63) / 64); + for (size_t w = 0; w < live; ++w) { + if (row[w] != 0) { + return false; + } + } + return true; + } + + // Phase A's per-rank running offsets, one padded row per partition (r_ can be in the hundreds, so + // this is a member and not a stack array). + auto run_row_(int t) -> size_t * { return run_scratch_.data() + static_cast(t) * run_stride_; } + + // Partition u summarizes its own send counts into its veto row, from its own argument and before the + // publishing barrier, so it costs no barrier and reads no peer. + auto fill_send_mask_(int local_partition, const int *send_counts /*[P]*/) -> void { + uint64_t *row = mask_row_(local_partition); + std::fill(row, row + mask_words_, uint64_t{0}); for (int b = 0; b < r_; ++b) { - long long send_sum = 0; - long long recv_sum = 0; + const int *counts = send_counts + b * s_; for (int t = 0; t < s_; ++t) { - for (int su = 0; su < s_; ++su) { - send_sum += slots_[static_cast(su)].send_counts[b * s_ + t]; - recv_sum += recv_count(t, b, su); + if (counts[t] != 0) { + row[static_cast(b) >> 6] |= 1ULL << (static_cast(b) & 63U); + break; } } - mpi_send_counts_[static_cast(b)] = checked_int_(send_sum); - mpi_recv_counts_[static_cast(b)] = checked_int_(recv_sum); } - long long send_running = 0; - long long recv_running = 0; - for (int b = 0; b < r_; ++b) { - mpi_send_displs_[static_cast(b)] = checked_int_(send_running); - mpi_recv_displs_[static_cast(b)] = checked_int_(recv_running); - send_running += mpi_send_counts_[static_cast(b)]; - recv_running += mpi_recv_counts_[static_cast(b)]; - } - const size_t total_send = static_cast(checked_int_(send_running)); - const size_t total_recv = static_cast(checked_int_(recv_running)); - // Grow-only, no zero-fill: pack_send_'s blocks tile [0, total_send) exactly and MPI_Alltoallv - // fills every live byte of stage_recv_, so stale bytes past a prior high-water mark are never read. - grow_(stage_send_, total_send * elem); - grow_(stage_recv_, total_recv * elem); - // Precompute each (rank b, dest partition t, source partition u) block start in elements, dest-major/ - // source-minor to match staging: pack/scatter become O(1) lookups that read no peer slot past B4 - // (re-summing peer count matrices instead would be O(R*S^3)). + } + + // Partition u writes its own source row of the exchanged count matrix. Disjoint per u and + // contiguous in t, so it needs no barrier of its own and no coherence ping-pong. + auto fill_count_row_(int local_partition, const int *send_counts /*[P]*/) -> void { + const int u = local_partition; for (int b = 0; b < r_; ++b) { - size_t cur = static_cast(mpi_send_displs_[static_cast(b)]); + const int *row = send_counts + b * s_; + int *dst = counts_send_.data() + cnt_idx_(b, u, 0); for (int t = 0; t < s_; ++t) { - for (int u = 0; u < s_; ++u) { - pack_off_[block_idx_(b, t, u)] = cur; - cur += static_cast(slots_[static_cast(u)].send_counts[b * s_ + t]); - } + dst[t] = row[t]; } } - for (int a = 0; a < r_; ++a) { - size_t cur = static_cast(mpi_recv_displs_[static_cast(a)]); - for (int t = 0; t < s_; ++t) { + fill_send_mask_(local_partition, send_counts); + } + + // Non-resolve alltoallv path: recv counts come from partition t's own published recv_counts, so + // in phase A below every partition reads only its own slot. + auto size_staging_parallel_(int local_partition, size_t elem) -> void { + size_staging_parallel_(local_partition, elem, [this](int t, int rank, int su) { + return slots_[static_cast(t)].recv_counts[rank * s_ + su]; + }); + } + + // Replaces the O(R*S^2) serial sizing pass with a parallel prefix. recv_count(t, rank, su) is the + // count partition t on this rank receives from (rank, source partition su). + // + // The offset of block (b, t, u) in the staged message decomposes as + // mpi_send_displs_[b] + sum_{t' < t} col_send_[b][t'] + sum_{u' < u} c[u'][b][t] + // \------------- tpre_send_[b][t], one O(R*S) scan -----/ \--- pack_off_[b][t][u] ---/ + // The right-hand term is an independent scan per (b,t) pair, so partition t owns every pair with its + // own t: R*S work each, S-way parallel, covering the same R*S^2 entries. Only the middle term needs + // global knowledge, and at O(R*S) it is small enough to leave on partition 0. + // + // The two barriers this costs (phase A complete before partition 0 reduces it, bases visible before + // anyone packs) buy S-way parallelism over what measured 84% of wall time at S=112. + template + auto size_staging_parallel_(int local_partition, size_t elem, RecvCountFn recv_count) -> void { + const int t = local_partition; + { + ScopedNs timer{par_table_ns(local_partition)}; + // Send half, SOURCE-partition-outer so each peer's veto row is loaded once for all R ranks + // instead of probing its count array R times on R separate cache lines -- the dominant cost + // of this phase once the serial fill was gone. Skipped blocks leave a stale pack_off_ entry, + // which is safe: an offset is read only where the matching count is nonzero. + size_t *run = run_row_(t); + std::fill(run, run + r_, size_t{0}); + for (int u = 0; u < s_; ++u) { + const uint64_t *mask = mask_row_(u); + if (mask_empty_(mask)) { + continue; + } + const int *counts = slots_[static_cast(u)].send_counts; + for (int b = 0; b < r_; ++b) { + if (!mask_test_(mask, b)) { + continue; // u sends nothing to rank b, so it contributes 0 to every (b,t) scan + } + pack_off_[block_idx_(b, t, u)] = run[b]; + run[b] += static_cast(counts[b * s_ + t]); + } + } + for (int b = 0; b < r_; ++b) { + col_send_[static_cast(b) * static_cast(s_) + static_cast(t)] = run[b]; + size_t got = 0; for (int su = 0; su < s_; ++su) { - scatter_off_[block_idx_(a, t, su)] = cur; - cur += static_cast(recv_count(t, a, su)); + scatter_off_[block_idx_(b, t, su)] = got; + got += static_cast(recv_count(t, b, su)); } + col_recv_[static_cast(b) * static_cast(s_) + static_cast(t)] = got; } } + sync(local_partition); // phase A complete on every partition + + if (local_partition == 0) { + ScopedNs timer{p0_table_ns(local_partition)}; + long long send_running = 0; + long long recv_running = 0; + for (int b = 0; b < r_; ++b) { + long long send_sum = 0; + long long recv_sum = 0; + const size_t base = static_cast(b) * static_cast(s_); + for (int k = 0; k < s_; ++k) { + send_sum += static_cast(col_send_[base + static_cast(k)]); + recv_sum += static_cast(col_recv_[base + static_cast(k)]); + } + mpi_send_counts_[static_cast(b)] = checked_int_(send_sum); + mpi_recv_counts_[static_cast(b)] = checked_int_(recv_sum); + mpi_send_displs_[static_cast(b)] = checked_int_(send_running); + mpi_recv_displs_[static_cast(b)] = checked_int_(recv_running); + // tpre_*_ folds the per-rank base in, so pack/scatter add exactly two numbers. + size_t cur_s = static_cast(send_running); + size_t cur_r = static_cast(recv_running); + for (int k = 0; k < s_; ++k) { + tpre_send_[base + static_cast(k)] = cur_s; + cur_s += col_send_[base + static_cast(k)]; + tpre_recv_[base + static_cast(k)] = cur_r; + cur_r += col_recv_[base + static_cast(k)]; + } + send_running += mpi_send_counts_[static_cast(b)]; + recv_running += mpi_recv_counts_[static_cast(b)]; + } + // Grow-only, no zero-fill: pack_send_'s blocks tile [0, total_send) exactly and MPI_Alltoallv + // fills every live byte of stage_recv_, so stale bytes past a prior high-water mark are never read. + grow_(stage_send_, static_cast(checked_int_(send_running)) * elem); + grow_(stage_recv_, static_cast(checked_int_(recv_running)) * elem); + } + sync(local_partition); // bases and staging visible to every packer } auto pack_send_(int local_partition, size_t elem) -> void { @@ -491,10 +785,13 @@ class HybridComm { const int *my_send_counts = slots_[static_cast(u)].send_counts; const int *my_send_displs = slots_[static_cast(u)].send_displs; for (int b = 0; b < r_; ++b) { + const size_t base = static_cast(b) * static_cast(s_); for (int t = 0; t < s_; ++t) { const int cnt = my_send_counts[b * s_ + t]; if (cnt != 0) { - std::memcpy(stage_send_.data() + pack_off_[block_idx_(b, t, u)] * elem, + // Absolute start = per-(b,t) base (partition 0) + within-(b,t) scan (partition t). + const size_t off = tpre_send_[base + static_cast(t)] + pack_off_[block_idx_(b, t, u)]; + std::memcpy(stage_send_.data() + off * elem, src + static_cast(my_send_displs[b * s_ + t]) * elem, static_cast(cnt) * elem); } @@ -502,6 +799,12 @@ class HybridComm { } } + // Absolute start of the block partition t receives from (rank a, source partition su). + auto recv_block_off_(int a, int t, int su) const -> size_t { + return tpre_recv_[static_cast(a) * static_cast(s_) + static_cast(t)] + + scatter_off_[block_idx_(a, t, su)]; + } + static auto checked_int_(long long v) -> int { if (v < 0 || v > static_cast(2147483647)) { throw std::runtime_error("HybridComm: aggregated per-rank count overflows int (message too large)"); @@ -521,7 +824,31 @@ class HybridComm { std::abort(); // MPI_Abort is not marked [[noreturn]]; unreachable in practice } - auto sync() -> void { barrier_.sync(); } + // Barrier wait is attributed to the partition that waited, which is the whole point: when the + // master monopolises a serial phase, its own wait stays near zero while every peer's grows. + auto sync(int local_partition) -> void { + if (prof_ == nullptr) { + barrier_.sync(local_partition); + return; + } + CommProfile::Slot &sl = prof_->slot(local_partition); + ++sl.n_barriers; + ScopedNs t{&sl.barrier_ns}; + barrier_.sync(local_partition); + } + + // nullptr target ⇒ ScopedNs is inert, so instrumented regions need no #ifdef or duplicate code. + auto p0_table_ns(int u) -> uint64_t * { return prof_ == nullptr ? nullptr : &prof_->slot(u).table_p0_ns; } + auto par_table_ns(int u) -> uint64_t * { return prof_ == nullptr ? nullptr : &prof_->slot(u).table_par_ns; } + auto move_ns(int u) -> uint64_t * { return prof_ == nullptr ? nullptr : &prof_->slot(u).table_move_ns; } + auto mpi_ns(int u) -> uint64_t * { return prof_ == nullptr ? nullptr : &prof_->slot(u).mpi_ns; } + auto count_verb(int u) -> void { + if (prof_ != nullptr) { + ++prof_->slot(u).n_verbs; + } + } + + std::unique_ptr prof_; MPI_Comm parent_; int s_; @@ -538,12 +865,32 @@ class HybridComm { std::vector mpi_send_displs_; std::vector mpi_recv_counts_; std::vector mpi_recv_displs_; - // [R*S*S] block starts (elements) in the staging buffers. + // [R*S*S] within-(rank, dest partition) exclusive scans over the SOURCE partition, filled by the + // owning dest partition; absolute starts are these plus tpre_*_ (see size_staging_parallel_). std::vector pack_off_; std::vector scatter_off_; + // [R*S] per-(rank, dest partition) column totals and their prefix, the only globally-reduced part. + std::vector col_send_; + std::vector col_recv_; + std::vector tpre_send_; + std::vector tpre_recv_; + // [S * mask_words_] one cache-line-padded veto row per source partition, and [S * run_stride_] + // per-partition scratch for phase A's per-rank running offsets. Both written only by their owner. + std::vector send_mask_; + std::vector run_scratch_; + size_t mask_words_ = 8; + size_t run_stride_ = 8; // Aggregated MPI payload staging, HWM-sized. std::vector stage_send_; std::vector stage_recv_; + // [R] reverse-round per-rank counts/displs (the forward round's, swapped and divided by the stride). + // Only partition 0 touches these and tables_gen_/reverse_ready_gen_, inside a barriered window. + std::vector rev_send_counts_; + std::vector rev_send_displs_; + std::vector rev_recv_counts_; + std::vector rev_recv_displs_; + uint64_t tables_gen_ = 0; + uint64_t reverse_ready_gen_ = ~0ULL; double red_f64_ = 0.0; uint64_t red_u64_ = 0; std::vector red_vec_; diff --git a/src/monoprop/detail/mpi/MPICompat.h b/src/monoprop/detail/mpi/MPICompat.h index 786d7342..20344bc5 100644 --- a/src/monoprop/detail/mpi/MPICompat.h +++ b/src/monoprop/detail/mpi/MPICompat.h @@ -236,11 +236,18 @@ struct PendingAlltoallv { // skip_self: do not send the self slot (the caller handles self inline) — self send/recv = 0. // known_recv_counts: recv counts already known (e.g. the transpose of the query counts), so skip the // count exchange. The self slot is also zeroed when skip_self is set. +// reverse_of_previous: this exchange is the answer leg of the immediately preceding begin_alltoallv on +// the same comm, and forward_stride is that leg's elements per record (a query is several words, its +// answer one value). Only the hybrid transport acts on it, reusing that round's offset tables instead of +// rebuilding them (see HybridComm::alltoallv_reverse); it requires known_recv_counts, and no other +// collective may intervene on that comm. template inline auto begin_alltoallv(const std::vector> &send_data, Comm comm, bool skip_self = false, - const std::vector *known_recv_counts = nullptr) -> PendingAlltoallv { + const std::vector *known_recv_counts = nullptr, + bool reverse_of_previous = false, + int forward_stride = 1) -> PendingAlltoallv { const int num_ranks = size(comm); if (static_cast(send_data.size()) != num_ranks) { throw CollectiveArgumentError( @@ -342,15 +349,29 @@ inline auto begin_alltoallv(const std::vector> &send_data, } #ifdef monoprop_ENABLE_MPI else if (comm.kind == Comm::Kind::Hybrid) { - comm.hyb->alltoallv(comm.shm_rank, - h.send_buffer.data(), - h.send_counts.data(), - h.send_displs.data(), - h.recv_buffer.data(), - h.recv_counts.data(), - h.recv_displs.data(), - sizeof(T), - datatype::get()); + if (reverse_of_previous) { + comm.hyb->alltoallv_reverse(comm.shm_rank, + h.send_buffer.data(), + h.send_counts.data(), + h.send_displs.data(), + h.recv_buffer.data(), + h.recv_counts.data(), + h.recv_displs.data(), + sizeof(T), + datatype::get(), + forward_stride); + } + else { + comm.hyb->alltoallv(comm.shm_rank, + h.send_buffer.data(), + h.send_counts.data(), + h.send_displs.data(), + h.recv_buffer.data(), + h.recv_counts.data(), + h.recv_displs.data(), + sizeof(T), + datatype::get()); + } } #endif else { diff --git a/src/monoprop/detail/mpi/PartitionBarrier.h b/src/monoprop/detail/mpi/PartitionBarrier.h index e8cac2ec..02032adc 100644 --- a/src/monoprop/detail/mpi/PartitionBarrier.h +++ b/src/monoprop/detail/mpi/PartitionBarrier.h @@ -14,9 +14,11 @@ #pragma once +#include #include #include #include +#include #include "monoprop/detail/mpi/CpuRelax.h" @@ -31,34 +33,77 @@ class ShmCommPoisoned : public std::runtime_error { // Sense-reversing generation barrier for a fixed number of in-process partition threads. Each barrier // word gets a private cache line: if `gen_` shared `arrived_`'s line, spinners' reloads would miss to // L3 on every peer arrival — O(S) coherence bounces (measured top hotspot at S=112). +// +// Given a group id per participant (its L3 domain, from CpuTopology) the barrier becomes two-level: fan +// in within a domain, then across domains, then release within the domain. Both the arrival fetch_add +// and the release store then cost O(S/G) coherence transactions instead of O(S), and stay inside one L3 +// slice. No group ids (pinning off, or /sys unreadable) or a single domain degrades to the flat barrier. class PartitionBarrier { public: - explicit PartitionBarrier(int participants) : participants_(participants) {} + explicit PartitionBarrier(int participants, const std::vector &group_of = {}) : participants_(participants) { + if (static_cast(group_of.size()) != participants || participants <= 0) { + return; // flat + } + // Compact the domain ids to 0..G-1 in first-seen order, and count each group. + std::vector domains; + group_of_.resize(static_cast(participants)); + for (int p = 0; p < participants; ++p) { + const auto it = std::find(domains.begin(), domains.end(), group_of[static_cast(p)]); + if (it == domains.end()) { + group_of_[static_cast(p)] = static_cast(domains.size()); + domains.push_back(group_of[static_cast(p)]); + } + else { + group_of_[static_cast(p)] = static_cast(it - domains.begin()); + } + } + if (domains.size() < 2) { + group_of_.clear(); + return; // flat + } + groups_ = static_cast(domains.size()); + group_arrived_ = std::vector(domains.size()); + group_gen_ = std::vector(domains.size()); + group_size_.assign(domains.size(), 0); + for (int p = 0; p < participants; ++p) { + ++group_size_[static_cast(group_of_[static_cast(p)])]; + } + } PartitionBarrier(const PartitionBarrier &) = delete; auto operator=(const PartitionBarrier &) -> PartitionBarrier & = delete; - auto sync() -> void { - const unsigned g = gen_.load(std::memory_order_acquire); - if (arrived_.fetch_add(1, std::memory_order_acq_rel) + 1 == participants_) { - arrived_.store(0, std::memory_order_relaxed); - gen_.store(g + 1, std::memory_order_release); + // `participant` selects the caller's group; ignored on the flat path. + auto sync(int participant) -> void { + if (groups_ < 2) { + const unsigned g = gen_.load(std::memory_order_acquire); + if (arrived_.fetch_add(1, std::memory_order_acq_rel) + 1 == participants_) { + arrived_.store(0, std::memory_order_relaxed); + gen_.store(g + 1, std::memory_order_release); + } + else { + spin_until_(gen_, g); + } } else { - // Bounded on-core spin first (pinned partitions ⇒ the release store lands in the pause window, - // no syscall); only long waits (imbalance, oversubscription) fall to yield. - int spins = 0; - while (gen_.load(std::memory_order_acquire) == g) { - if (poisoned_.load(std::memory_order_acquire)) { - throw ShmCommPoisoned(); - } - if (spins < detail::kSpinPauseIters) { - ++spins; - detail::cpu_relax(); + const size_t gi = static_cast(group_of_[static_cast(participant)]); + const unsigned seen = group_gen_[gi].v.load(std::memory_order_acquire); + if (group_arrived_[gi].v.fetch_add(1, std::memory_order_acq_rel) + 1 == group_size_[gi]) { + group_arrived_[gi].v.store(0, std::memory_order_relaxed); + // The domain's last arriver represents it upstream, and releases the domain only after + // the root barrier, so passing the domain word still implies everyone arrived. + const unsigned root_seen = gen_.load(std::memory_order_acquire); + if (arrived_.fetch_add(1, std::memory_order_acq_rel) + 1 == groups_) { + arrived_.store(0, std::memory_order_relaxed); + gen_.store(root_seen + 1, std::memory_order_release); } else { - std::this_thread::yield(); + spin_until_(gen_, root_seen); } + group_gen_[gi].v.store(seen + 1, std::memory_order_release); + } + else { + spin_until_(group_gen_[gi].v, seen); } } if (poisoned_.load(std::memory_order_acquire)) { @@ -66,20 +111,60 @@ class PartitionBarrier { } } + // L3 domains the participants were grouped into; < 2 means the flat barrier is in use. Reported by + // CommProfile so a run states which barrier it ran instead of leaving it inferred from the timing. + auto group_count() const -> int { return groups_; } + // Signal that this participant is unwinding (e.g. an engine exception), releasing peers spinning in a // barrier. Idempotent. auto poison() -> void { poisoned_.store(true, std::memory_order_release); } // Must be called only when every participant is quiescent (between rounds), so a poison-aborted round - // leaves no dirty state. `gen_` deliberately stays monotonic: each participant re-reads it at its next - // barrier. + // leaves no dirty state. The generations deliberately stay monotonic: each participant re-reads its + // own at the next barrier. auto reset() -> void { poisoned_.store(false, std::memory_order_relaxed); arrived_.store(0, std::memory_order_relaxed); + for (auto &c : group_arrived_) { + c.v.store(0, std::memory_order_relaxed); + } } private: + // Spin until `word` leaves `seen`. Bounded on-core spin first (pinned partitions ⇒ the release store + // lands in the pause window, no syscall); only long waits (imbalance, oversubscription) fall to + // yield. A poisoned peer releases us with an exception rather than a hang. + auto spin_until_(const std::atomic &word, unsigned seen) const -> void { + int spins = 0; + while (word.load(std::memory_order_acquire) == seen) { + if (poisoned_.load(std::memory_order_acquire)) { + throw ShmCommPoisoned(); + } + if (spins < detail::kSpinPauseIters) { + ++spins; + detail::cpu_relax(); + } + else { + std::this_thread::yield(); + } + } + } + + // Per-domain words, each on its own cache line for the same reason the flat pair is split. + struct alignas(64) Count { + std::atomic v{0}; + }; + struct alignas(64) Gen { + std::atomic v{0}; + }; + int participants_; + int groups_ = 0; // < 2 ⇒ flat: arrived_/gen_ count participants, not domains + std::vector group_of_; + std::vector group_size_; + std::vector group_arrived_; + std::vector group_gen_; + // Flat barrier, or (two-level) the root barrier the domains' last arrivers meet at. alignas(64) std::atomic arrived_{0}; alignas(64) std::atomic gen_{0}; alignas(64) std::atomic poisoned_{false}; diff --git a/src/monoprop/detail/mpi/ShmComm.h b/src/monoprop/detail/mpi/ShmComm.h index a9e021b3..657a95ea 100644 --- a/src/monoprop/detail/mpi/ShmComm.h +++ b/src/monoprop/detail/mpi/ShmComm.h @@ -24,7 +24,9 @@ #include #include +#include "monoprop/detail/EnvConfig.h" #include "monoprop/detail/mpi/CheckedCount.h" +#include "monoprop/detail/mpi/CommProfile.h" #include "monoprop/detail/mpi/PartitionBarrier.h" // In-process shared-memory SPMD transport: S partition-master threads each call the same collective @@ -37,21 +39,41 @@ namespace monoprop::mpi { class ShmComm { public: - explicit ShmComm(int n) : n_(n), slots_(static_cast(n)), barrier_(n) {} + // partition_l3_domains (optional, one entry per partition) makes the barrier two-level; see + // PartitionBarrier. + explicit ShmComm(int n, const std::vector &partition_l3_domains = {}) + : n_(n), + slots_(static_cast(n)), + barrier_(n, partition_l3_domains) { + if (config::get().comm_profile) { + prof_ = std::make_unique(n, /*mpi_rank=*/-1); // -1 marks the single-rank transport + prof_->barrier_groups = barrier_.group_count(); + } + } ShmComm(const ShmComm &) = delete; auto operator=(const ShmComm &) -> ShmComm & = delete; + ~ShmComm() { + if (prof_ != nullptr) { + prof_->dump(); + } + } + auto size() const -> int { return n_; } // recv_counts[s] = what rank s sends to me (the transpose of the send-count matrix). auto alltoall_counts(int rank, const int *send_counts, int *recv_counts) -> void { + count_verb(rank); slots_[static_cast(rank)].counts = send_counts; - sync(); - for (int s = 0; s < n_; ++s) { - recv_counts[s] = slots_[static_cast(s)].counts[rank]; + sync(rank); + { + ScopedNs timer{par_table_ns(rank)}; + for (int s = 0; s < n_; ++s) { + recv_counts[s] = slots_[static_cast(s)].counts[rank]; + } } - sync(); + sync(rank); } // Variable all-to-all over caller-owned flat buffers (counts/displs in elements, `elem` = element @@ -64,23 +86,27 @@ class ShmComm { const int *recv_counts, const int *recv_displs, size_t elem) -> void { + count_verb(rank); Slot &me = slots_[static_cast(rank)]; me.ptr = send; me.displs = send_displs; - sync(); - auto *dst = static_cast(recv); - for (int s = 0; s < n_; ++s) { - const Slot &src = slots_[static_cast(s)]; - const auto *sp = static_cast(src.ptr); - const auto count = static_cast(recv_counts[s]); - if (count == 0) { - continue; + sync(rank); + { + ScopedNs timer{move_ns(rank)}; + auto *dst = static_cast(recv); + for (int s = 0; s < n_; ++s) { + const Slot &src = slots_[static_cast(s)]; + const auto *sp = static_cast(src.ptr); + const auto count = static_cast(recv_counts[s]); + if (count == 0) { + continue; + } + std::memcpy(dst + static_cast(recv_displs[s]) * elem, + sp + static_cast(src.displs[rank]) * elem, + count * elem); } - std::memcpy(dst + static_cast(recv_displs[s]) * elem, - sp + static_cast(src.displs[rank]) * elem, - count * elem); } - sync(); + sync(rank); } // Fused count-resolve + payload all-to-all in one round (2 syncs vs 4). Same contiguous @@ -97,27 +123,34 @@ class ShmComm { me.ptr = send; me.displs = send_displs; me.counts = send_counts; - sync(); // B1: send buffers published - long long total = 0; - for (int s = 0; s < n_; ++s) { - const int c = slots_[static_cast(s)].counts[rank]; // what s sends to me - recv_counts[s] = c; - recv_displs[s] = checked_mpi_count(total, "Recv displacement"); - total += c; + count_verb(rank); + sync(rank); // B1: send buffers published + { + ScopedNs timer{par_table_ns(rank)}; + long long total = 0; + for (int s = 0; s < n_; ++s) { + const int c = slots_[static_cast(s)].counts[rank]; // what s sends to me + recv_counts[s] = c; + recv_displs[s] = checked_mpi_count(total, "Recv displacement"); + total += c; + } + recv.resize(static_cast(checked_mpi_count(total, "Total recv count"))); } - recv.resize(static_cast(checked_mpi_count(total, "Total recv count"))); - auto *dst = reinterpret_cast(recv.data()); - for (int s = 0; s < n_; ++s) { - const Slot &src = slots_[static_cast(s)]; - const auto count = static_cast(recv_counts[s]); - if (count == 0) { - continue; + { + ScopedNs timer{move_ns(rank)}; + auto *dst = reinterpret_cast(recv.data()); + for (int s = 0; s < n_; ++s) { + const Slot &src = slots_[static_cast(s)]; + const auto count = static_cast(recv_counts[s]); + if (count == 0) { + continue; + } + std::memcpy(dst + static_cast(recv_displs[s]) * sizeof(T), + reinterpret_cast(src.ptr) + static_cast(src.displs[rank]) * sizeof(T), + count * sizeof(T)); } - std::memcpy(dst + static_cast(recv_displs[s]) * sizeof(T), - reinterpret_cast(src.ptr) + static_cast(src.displs[rank]) * sizeof(T), - count * sizeof(T)); } - sync(); // B2: peers finished reading our send buffer before the caller may reuse it + sync(rank); // B2: peers finished reading our send buffer before the caller may reuse it } template @@ -129,7 +162,8 @@ class ShmComm { else { me.u64 = static_cast(local_val); } - sync(); + count_verb(rank); + sync(rank); T acc{}; for (int s = 0; s < n_; ++s) { const Slot &src = slots_[static_cast(s)]; @@ -140,30 +174,34 @@ class ShmComm { acc += static_cast(src.u64); } } - sync(); + sync(rank); return acc; } // Safe in place: each element is read then overwritten by its single slice owner, and slices are // cache-line-rounded. auto allreduce_sum_inplace(int rank, double *values, size_t len) -> void { + count_verb(rank); slots_[static_cast(rank)].vec = values; - sync(); - constexpr size_t kLine = 64 / sizeof(double); - const size_t lines = (len + kLine - 1) / kLine; - const size_t per = (lines + static_cast(n_) - 1) / static_cast(n_); - const size_t lo = std::min(len, static_cast(rank) * per * kLine); - const size_t hi = std::min(len, lo + per * kLine); - for (size_t k = lo; k < hi; ++k) { - double acc = 0.0; - for (int s = 0; s < n_; ++s) { // ascending rank order - acc += slots_[static_cast(s)].vec[k]; - } - for (int s = 0; s < n_; ++s) { // publish the same bits into every rank's buffer - slots_[static_cast(s)].vec[k] = acc; + sync(rank); + { + ScopedNs timer{move_ns(rank)}; + constexpr size_t kLine = 64 / sizeof(double); + const size_t lines = (len + kLine - 1) / kLine; + const size_t per = (lines + static_cast(n_) - 1) / static_cast(n_); + const size_t lo = std::min(len, static_cast(rank) * per * kLine); + const size_t hi = std::min(len, lo + per * kLine); + for (size_t k = lo; k < hi; ++k) { + double acc = 0.0; + for (int s = 0; s < n_; ++s) { // ascending rank order + acc += slots_[static_cast(s)].vec[k]; + } + for (int s = 0; s < n_; ++s) { // publish the same bits into every rank's buffer + slots_[static_cast(s)].vec[k] = acc; + } } } - sync(); // peers write into our buffer (and read from it) until here + sync(rank); // peers write into our buffer (and read from it) until here } // See PartitionBarrier::poison / ::reset for when each is legal to call. @@ -183,8 +221,28 @@ class ShmComm { uint64_t u64 = 0; }; - auto sync() -> void { barrier_.sync(); } + // Same per-partition attribution as HybridComm::sync. Every non-barrier phase here is already + // parallel, so a run's whole floor lands in barrier_ns and reads off as cost-per-sync. + auto sync(int rank) -> void { + if (prof_ == nullptr) { + barrier_.sync(rank); + return; + } + CommProfile::Slot &sl = prof_->slot(rank); + ++sl.n_barriers; + ScopedNs t{&sl.barrier_ns}; + barrier_.sync(rank); + } + + auto par_table_ns(int rank) -> uint64_t * { return prof_ == nullptr ? nullptr : &prof_->slot(rank).table_par_ns; } + auto move_ns(int rank) -> uint64_t * { return prof_ == nullptr ? nullptr : &prof_->slot(rank).table_move_ns; } + auto count_verb(int rank) -> void { + if (prof_ != nullptr) { + ++prof_->slot(rank).n_verbs; + } + } + std::unique_ptr prof_; int n_; std::vector slots_; PartitionBarrier barrier_; diff --git a/src/monoprop/detail/partition/CpuTopology.h b/src/monoprop/detail/partition/CpuTopology.h index 8eb146dd..3f973b21 100644 --- a/src/monoprop/detail/partition/CpuTopology.h +++ b/src/monoprop/detail/partition/CpuTopology.h @@ -229,6 +229,32 @@ inline auto partition_cpusets(size_t n, size_t group_index = 0, size_t group_cou return sets; } +// The L3 domain each partition cpuset lands in, in partition_cpusets order -- what a two-level +// PartitionBarrier groups by. Derived from the sets, not from the placement logic, so the two cannot +// drift apart. Empty (⇒ flat barrier) if the sets are empty or one names no core the scan knows. +inline auto cpuset_l3_domains(const std::vector &sets) -> std::vector { + if (sets.empty()) { + return {}; + } + const auto cores = enumerate_physical_cores(); + std::vector domains; + domains.reserve(sets.size()); + for (const CpuSet &set : sets) { + int found = -1; + for (const auto &core : cores) { + if (CPU_ISSET(core.cpu, &set)) { + found = core.l3_domain; + break; + } + } + if (found < 0) { + return {}; + } + domains.push_back(found); + } + return domains; +} + // A failing pthread call is ignored: only performance depends on it. inline auto pin_this_thread(const CpuSet &set) -> void { pthread_setaffinity_np(pthread_self(), sizeof(CpuSet), &set); @@ -256,6 +282,9 @@ inline auto partition_cpusets(size_t /*n*/, size_t /*group_index*/ = 0, size_t / -> std::vector { return {}; } +inline auto cpuset_l3_domains(const std::vector & /*sets*/) -> std::vector { + return {}; +} inline auto pin_this_thread(const CpuSet & /*set*/) -> void {} #endif // __linux__ diff --git a/src/monoprop/detail/partition/PartitionGroup.h b/src/monoprop/detail/partition/PartitionGroup.h index fc3f64fd..d2661402 100644 --- a/src/monoprop/detail/partition/PartitionGroup.h +++ b/src/monoprop/detail/partition/PartitionGroup.h @@ -57,9 +57,11 @@ class PartitionGroup { parent_(parent), partitions_(static_cast(n_partitions)), errs_(static_cast(n_partitions)) { - make_transport_(); + // Placement is decided before the transport, because the transport's barrier is grouped by the + // L3 domain each partition will be pinned to. discover_node_peers_(); cpusets_ = topo_partition_cpusets(n_, node_rank_, node_size_); + make_transport_(); start_masters_(); // The masters are already running, so a ctor throw must not escape: ~PartitionGroup would never run, // and destroying joinable threads during unwinding calls std::terminate. @@ -81,8 +83,8 @@ class PartitionGroup { node_size_(src.node_size_), partitions_(static_cast(src.n_)), errs_(static_cast(src.n_)) { - make_transport_(); cpusets_ = topo_partition_cpusets(n_, node_rank_, node_size_); + make_transport_(); // after cpusets_, as in the primary ctor start_masters_(); try { // see the primary ctor: a throw past live masters would std::terminate run_on_all([&](int r) { @@ -168,13 +170,16 @@ class PartitionGroup { } auto make_transport_() -> void { + // Empty unless the partitions are pinned and /sys was readable ⇒ flat barrier (see + // PartitionBarrier); cpusets_ must already be set. + const std::vector domains = monoprop::detail::partition::cpuset_l3_domains(cpusets_); #ifdef monoprop_ENABLE_MPI if (parent_.kind == mpi::Comm::Kind::Mpi && mpi::size(parent_) > 1) { - hyb_ = std::make_unique(parent_.mpi, n_); + hyb_ = std::make_unique(parent_.mpi, n_, domains); return; } #endif - shm_ = std::make_unique(n_); + shm_ = std::make_unique(n_, domains); } auto comm_for_(int r) -> mpi::Comm { #ifdef monoprop_ENABLE_MPI diff --git a/tests/cpp/hybrid_comm_tests.cpp b/tests/cpp/hybrid_comm_tests.cpp index 7c0e80f4..24fa3a39 100644 --- a/tests/cpp/hybrid_comm_tests.cpp +++ b/tests/cpp/hybrid_comm_tests.cpp @@ -328,4 +328,132 @@ BOOST_AUTO_TEST_CASE(hybrid_comm_poison_releases_waiters) { } } +// The cases above give every destination the SAME count per source, which cannot tell the +// (rank, dest partition, source partition) index order the offset tables are built from from its +// transpose. So drive counts that depend on source AND destination, and tag every element with both ends +// of its leg: a swapped index or a wrong base then misroutes payload instead of yielding a +// coincidentally-correct total. S=5 (> the 3 used above) leaves R*S^2 room to go wrong. +BOOST_AUTO_TEST_CASE(hybrid_comm_alltoallv_resolve_asymmetric_counts) { + if (world_size() < 2) { + return; + } + const int R = world_size(); + const int S = 5; + const int P = R * S; + const int rounds = 12; + // Deliberately not symmetric under swapping src/dst, and hits 0 for some legs. + const auto leg_len = [](int src, int dst, int round) { return (src * 7 + dst * 3 + round) % 5; }; + const auto tag = [](int src, int dst, int j) { return ((src * 1000 + dst) * 100) + j; }; + std::atomic failures{0}; + auto errs = run_hybrid(S, [&](HybridComm &hyb, int u) { + Comm c = Comm::make_hybrid(&hyb, u); + const int g = monoprop::mpi::rank(c); + std::vector recv; + std::vector rc(static_cast(P)), rd(static_cast(P)); + for (int round = 0; round < rounds; ++round) { + std::vector send; + std::vector sc(static_cast(P)), sd(static_cast(P)); + int off = 0; + for (int d = 0; d < P; ++d) { + const int len = leg_len(g, d, round); + sc[static_cast(d)] = len; + sd[static_cast(d)] = off; + for (int j = 0; j < len; ++j) { + send.push_back(tag(g, d, j)); + } + off += len; + } + hyb.alltoallv_resolve(u, + send.data(), + sc.data(), + sd.data(), + recv, + rc.data(), + rd.data(), + sizeof(int), + monoprop::mpi::datatype::get()); + int expected_total = 0; + for (int src = 0; src < P; ++src) { + const int len = leg_len(src, g, round); + if (rc[static_cast(src)] != len || rd[static_cast(src)] != expected_total) { + failures.fetch_add(1); + } + for (int j = 0; j < len; ++j) { + if (recv[static_cast(expected_total + j)] != tag(src, g, j)) { + failures.fetch_add(1); + } + } + expected_total += len; + } + if (static_cast(recv.size()) != expected_total) { + failures.fetch_add(1); + } + } + }); + for (const auto &e : errs) { + BOOST_CHECK(e == nullptr); + } + BOOST_CHECK_EQUAL(failures.load(), 0); +} + +// Same asymmetric legs through the count-exchange + flat-alltoallv pair, which uses the other +// sizing entry point (recv counts published per partition rather than resolved from the wire). +BOOST_AUTO_TEST_CASE(hybrid_comm_alltoall_counts_then_alltoallv_asymmetric) { + if (world_size() < 2) { + return; + } + const int R = world_size(); + const int S = 4; + const int P = R * S; + const auto leg_len = [](int src, int dst) { return (src * 5 + dst * 11) % 4; }; + const auto tag = [](int src, int dst, int j) { return ((src * 1000 + dst) * 100) + j; }; + std::atomic failures{0}; + auto errs = run_hybrid(S, [&](HybridComm &hyb, int u) { + const int g = hyb.global_rank(u); + std::vector sc(static_cast(P)), sd(static_cast(P)); + std::vector rc(static_cast(P)), rd(static_cast(P)); + std::vector send; + int off = 0; + for (int d = 0; d < P; ++d) { + const int len = leg_len(g, d); + sc[static_cast(d)] = len; + sd[static_cast(d)] = off; + for (int j = 0; j < len; ++j) { + send.push_back(tag(g, d, j)); + } + off += len; + } + hyb.alltoall_counts(u, sc.data(), rc.data()); + int total = 0; + for (int src = 0; src < P; ++src) { + if (rc[static_cast(src)] != leg_len(src, g)) { + failures.fetch_add(1); + } + rd[static_cast(src)] = total; + total += rc[static_cast(src)]; + } + std::vector recv(static_cast(total)); + hyb.alltoallv(u, + send.data(), + sc.data(), + sd.data(), + recv.data(), + rc.data(), + rd.data(), + sizeof(int), + monoprop::mpi::datatype::get()); + for (int src = 0; src < P; ++src) { + for (int j = 0; j < rc[static_cast(src)]; ++j) { + if (recv[static_cast(rd[static_cast(src)] + j)] != tag(src, g, j)) { + failures.fetch_add(1); + } + } + } + }); + for (const auto &e : errs) { + BOOST_CHECK(e == nullptr); + } + BOOST_CHECK_EQUAL(failures.load(), 0); +} + #endif // monoprop_ENABLE_MPI diff --git a/tests/cpp/partition_barrier_tests.cpp b/tests/cpp/partition_barrier_tests.cpp new file mode 100644 index 00000000..dcc31591 --- /dev/null +++ b/tests/cpp/partition_barrier_tests.cpp @@ -0,0 +1,161 @@ +// Copyright 2026 Algorithmiq +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include + +#include +#include +#include +#include +#include + +#include "monoprop/detail/mpi/PartitionBarrier.h" + +using monoprop::mpi::PartitionBarrier; +using monoprop::mpi::ShmCommPoisoned; + +namespace { + +// Every participant stamps its own slot with the round number; after the barrier every slot must read as +// stamped, so anyone released early sees a stale one. Tested directly because the two-level path needs +// pinning and >=2 L3 domains to engage through a real ShmComm, which no test host is guaranteed to have. +auto stamp_rounds(int n, const std::vector &groups, int rounds) -> std::vector { + PartitionBarrier barrier(n, groups); + std::vector slots(static_cast(n), -1); + std::vector errs(static_cast(n)); + std::atomic mismatches{0}; + std::vector threads; + threads.reserve(static_cast(n)); + for (int p = 0; p < n; ++p) { + threads.emplace_back([&, p] { + try { + for (int round = 0; round < rounds; ++round) { + slots[static_cast(p)] = round; + barrier.sync(p); + for (int q = 0; q < n; ++q) { + if (slots[static_cast(q)] != round) { + ++mismatches; + } + } + barrier.sync(p); // peers must finish reading before the next stamp overwrites + } + } + catch (...) { + errs[static_cast(p)] = std::current_exception(); + } + }); + } + for (auto &t : threads) { + t.join(); + } + BOOST_CHECK_EQUAL(mismatches.load(), 0); + return errs; +} + +auto no_errors(const std::vector &errs) -> void { + for (const auto &e : errs) { + BOOST_CHECK(e == nullptr); + } +} + +} // namespace + +// No group ids ⇒ the flat barrier, the path every unpinned run takes. +BOOST_AUTO_TEST_CASE(partition_barrier_flat_releases_together) { + for (const int n : {1, 2, 3, 8}) { + no_errors(stamp_rounds(n, {}, 20)); + } +} + +// Two-level: same semantics, and the group sizes are deliberately uneven and not powers of two, so a +// barrier that mixed up "last in group" with "last overall" would deadlock or release early. +BOOST_AUTO_TEST_CASE(partition_barrier_grouped_releases_together) { + no_errors(stamp_rounds(8, {0, 0, 0, 0, 1, 1, 1, 1}, 20)); + no_errors(stamp_rounds(7, {0, 0, 0, 1, 1, 2, 2}, 20)); + no_errors(stamp_rounds(6, {3, 9, 3, 9, 3, 9}, 20)); // interleaved, non-contiguous domain ids + no_errors(stamp_rounds(5, {0, 1, 1, 1, 1}, 20)); // a domain of one +} + +// One domain is no reason to pay for a root barrier: it must behave exactly like the flat path. +BOOST_AUTO_TEST_CASE(partition_barrier_single_group_is_flat) { + no_errors(stamp_rounds(4, {2, 2, 2, 2}, 20)); +} + +// A group id list that does not cover every participant is a programming error upstream, not a reason +// to hang: it falls back to flat rather than indexing out of range. +BOOST_AUTO_TEST_CASE(partition_barrier_short_group_list_falls_back) { + no_errors(stamp_rounds(4, {0, 1}, 20)); +} + +// poison() must release waiters in BOTH levels: a peer that throws mid-round leaves the others parked +// on their domain's word, not the root's. +BOOST_AUTO_TEST_CASE(partition_barrier_poison_releases_grouped_waiters) { + constexpr int kN = 6; + const std::vector groups{0, 0, 0, 1, 1, 1}; + PartitionBarrier barrier(kN, groups); + std::atomic poisoned_count{0}; + std::vector threads; + threads.reserve(kN - 1); + for (int p = 0; p < kN - 1; ++p) { // participant kN-1 never arrives + threads.emplace_back([&, p] { + try { + barrier.sync(p); + } + catch (const ShmCommPoisoned &) { + ++poisoned_count; + } + }); + } + // Let the arrivers reach the barrier before poisoning, so this exercises the release of parked + // waiters rather than the post-barrier poison check. + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + barrier.poison(); + for (auto &t : threads) { + t.join(); + } + // The last arriver of the complete domain parks at the root; the rest park on their domain word. + BOOST_CHECK_EQUAL(poisoned_count.load(), kN - 1); +} + +// reset() after an aborted round must leave no partial arrival count behind, in any group. +BOOST_AUTO_TEST_CASE(partition_barrier_reset_clears_partial_arrivals) { + constexpr int kN = 4; + const std::vector groups{0, 0, 1, 1}; + PartitionBarrier barrier(kN, groups); + std::thread lone([&] { + try { + barrier.sync(0); // arrives alone, then is released by the poison below + } + catch (const ShmCommPoisoned &) { + } + }); + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + barrier.poison(); + lone.join(); + barrier.reset(); + + std::atomic through{0}; + std::vector threads; + threads.reserve(kN); + for (int p = 0; p < kN; ++p) { + threads.emplace_back([&, p] { + barrier.sync(p); + ++through; + }); + } + for (auto &t : threads) { + t.join(); + } + BOOST_CHECK_EQUAL(through.load(), kN); +} From 69a793272a0556a5e145944a8522db0e0ead31aa Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Wed, 29 Jul 2026 09:36:47 +0200 Subject: [PATCH 03/17] =?UTF-8?q?docs(parallelism):=20=F0=9F=93=9D=20docum?= =?UTF-8?q?ent=20monoprop=5FCOMM=5FPROFILE=20and=20the=20two-level=20barri?= =?UTF-8?q?er?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both are new behaviour from the HybridComm floor work: a runtime knob belongs in the environment-variable table, and the barrier's L3-domain grouping is a performance property a reader tuning partition placement needs to know about. Assisted-by: ClaudeCode:claude-opus-5 --- docs/content/docs/features/parallelism.mdx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/content/docs/features/parallelism.mdx b/docs/content/docs/features/parallelism.mdx index 8ebacb7b..943626b8 100644 --- a/docs/content/docs/features/parallelism.mdx +++ b/docs/content/docs/features/parallelism.mdx @@ -22,6 +22,9 @@ gives each partition a pinned worker thread that runs it serially. Each partitio keeps its own small, cache-resident term index; a gate is applied to all partitions at once, synchronised by a lightweight barrier, and anticommuting terms whose partner lives in another partition are resolved through a per-gate exchange. +When the partitions are pinned and span more than one L3 domain, that barrier fans in +within each domain and then across domains, so both the arrival counter and the +release store stay inside one L3 slice instead of crossing the socket interconnect. ## Runtime environment variables @@ -30,6 +33,7 @@ whose partner lives in another partition are resolved through a per-gate exchang | `monoprop_NUM_THREADS` | one partition per physical core | Caps the number of partitions. Set it to run fewer partitions than cores. | | `monoprop_PARTITIONS` | `auto` | `auto` = one partition per core (capped by `monoprop_NUM_THREADS`); an integer `N` = exactly `N` partitions; `off` = one partition holding the whole operator. | | `monoprop_PARTITION_PINNING` | `on` | `0`/`false`/`no` disables pinning each partition to a core. Has an effect only on Linux. | +| `monoprop_COMM_PROFILE` | `off` | `1`/`true`/`yes` makes each transport print one `COMMPROF` line per rank to stderr when it is destroyed, accounting for where its collectives' wall time went. Diagnostic only; off costs one branch per collective phase. | ```bash # Run 8 partitions instead of one-per-core: From 22631cd7c3db6d2692bed1d9fa55fd455f8591fe Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Tue, 11 Aug 2026 13:19:32 +0100 Subject: [PATCH 04/17] =?UTF-8?q?test(mpi):=20=F0=9F=94=A7=20move=20the=20?= =?UTF-8?q?branch's=20HybridComm=20cases=20onto=20main's=20args=20bundles?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hybrid_comm_tests.cpp auto-merged cleanly, so it kept calling the pre-bundle per-argument alltoallv / alltoallv_resolve signatures that the merge replaced. These were the only two build errors; the library itself compiled unchanged. Both sites now use the braced-init form already used at hybrid_comm_tests.cpp:236 and shm_comm_tests.cpp:262, which came from main's side of the merge -- one convention in the file rather than two. Co-Authored-By: Claude Opus 5 --- cpp/tests/hybrid_comm_tests.cpp | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/cpp/tests/hybrid_comm_tests.cpp b/cpp/tests/hybrid_comm_tests.cpp index 084929ef..60c042ed 100644 --- a/cpp/tests/hybrid_comm_tests.cpp +++ b/cpp/tests/hybrid_comm_tests.cpp @@ -363,13 +363,12 @@ BOOST_AUTO_TEST_CASE(hybrid_comm_alltoallv_resolve_asymmetric_counts) { off += len; } hyb.alltoallv_resolve(u, - send.data(), - sc.data(), - sd.data(), - recv, - rc.data(), - rd.data(), - sizeof(int), + {.send = send.data(), + .send_counts = sc.data(), + .send_displs = sd.data(), + .recv = recv, + .recv_counts = rc.data(), + .recv_displs = rd.data()}, monoprop::mpi::datatype::get()); int expected_total = 0; for (int src = 0; src < P; ++src) { @@ -433,13 +432,13 @@ BOOST_AUTO_TEST_CASE(hybrid_comm_alltoall_counts_then_alltoallv_asymmetric) { } std::vector recv(static_cast(total)); hyb.alltoallv(u, - send.data(), - sc.data(), - sd.data(), - recv.data(), - rc.data(), - rd.data(), - sizeof(int), + monoprop::mpi::FlatAlltoallvArgs{.send = send.data(), + .send_counts = sc.data(), + .send_displs = sd.data(), + .recv = recv.data(), + .recv_counts = rc.data(), + .recv_displs = rd.data()} + .bytes(), monoprop::mpi::datatype::get()); for (int src = 0; src < P; ++src) { for (int j = 0; j < rc[static_cast(src)]; ++j) { From c1e034cf48674a0a2134d214dce573c72289929b Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Tue, 11 Aug 2026 16:47:37 +0100 Subject: [PATCH 05/17] =?UTF-8?q?style(mpi):=20=F0=9F=8E=A8=20realign=20th?= =?UTF-8?q?e=20HybridComm=20thread-level=20throw=20for=20clang-format?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The merge took main's `MpiThreadLevelUnsupported` exception type but kept this branch's continuation indentation, which was aligned to the shorter `std::runtime_error(` it replaced. clang-format 21.1.0 flags both continuation lines, and that is the whole of the lint job's failure. Whitespace only: the message text and every surrounding statement are untouched. Verified with `clang-format@21.1.0 --dry-run -Werror` over all 11 changed C++ files (clean), plus gersemi on the changed CMakeLists. Co-Authored-By: Claude Opus 5 --- cpp/monoprop/detail/mpi/HybridComm.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/monoprop/detail/mpi/HybridComm.h b/cpp/monoprop/detail/mpi/HybridComm.h index 41685767..8462f62f 100644 --- a/cpp/monoprop/detail/mpi/HybridComm.h +++ b/cpp/monoprop/detail/mpi/HybridComm.h @@ -65,8 +65,8 @@ class HybridComm { MPI_Query_thread(&provided); if (provided < MPI_THREAD_SERIALIZED) { throw MpiThreadLevelUnsupported("HybridComm requires MPI_THREAD_SERIALIZED (partition-0 masters call " - "MPI while peers are parked); provided level is lower. Ensure " - "mpi::init / mpi4py requests SERIALIZED or MULTIPLE."); + "MPI while peers are parked); provided level is lower. Ensure " + "mpi::init / mpi4py requests SERIALIZED or MULTIPLE."); } // Size all (R,S)-fixed scratch once so per-call paths never allocate; staging grows on demand. const size_t rss = static_cast(r_) * static_cast(s_) * static_cast(s_); From d2083d1e59e9f73afe67485f0edfb725a939e071 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Wed, 12 Aug 2026 13:37:27 +0100 Subject: [PATCH 06/17] =?UTF-8?q?fix(partition):=20=F0=9F=90=9B=20make=20p?= =?UTF-8?q?artition=20placement=20and=20the=20barrier=20spin=20portable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three independent mechanisms made intra-node performance depend on the machine and the launcher rather than on the work. All three degraded silently: every equivalence and determinism test passed throughout. Pinning disabled itself under Slurm. enumerate_physical_cores() already filters by the process affinity mask, and partition_cpusets() then divided by the number of co-located ranks a second time, so group_count * n exceeded the rank's share, the guard read that as "host too small", and the rank ran unpinned -- taking the two-level barrier's domains with it, since cpuset_domains() derives them from the cpusets. Measured on Deucalion, both builds interleaved in one allocation on one node: layout 8x16 went barrier_groups=0 -> 4 and ~437 -> 15.5 us/sync, layout 2x64 0 -> 16 and ~840 -> 11.5 us/sync, while the 1x128 layout that never hit the bug kept its 32 domains and its timing. End-to-end on two nodes, 1.58x-3.84x and 2.68x-4.19x across build_graph/energy/gradient/inplace/pare. Mask width cannot tell a per-rank slice from a shared one -- eight ranks holding 16 cores each and eight sharing one 16-core mask both leave a rank seeing 16 of 128 -- and the two need opposite placement, so collapsing on width alone would point every co-located rank at the same cores. PartitionGroup therefore allgathers the raw masks over the node-local communicator it already opens, and classify_node_mask answers PerRank only for pairwise-disjoint masks; identical, overlapping and unreadable all answer Shared, which is both the old behaviour and the safe direction. The barrier's spin budget was a fixed iteration count whose wall time is architecture-dependent: cpu_relax() is one PAUSE on x86 and one YIELD on aarch64, which differ by more than an order of magnitude, so 2048 iterations spent a wholly different budget on each and on aarch64 skipped the on-core spin almost entirely. It is now a time budget, calibrated by measurement on this Zen 2 part and overridable per run with monoprop_SPIN_BUDGET_US. Parking past the yield phase was tried and rejected (2021 vs 762 us/sync oversubscribed): one sleeper's timer overshoot delays everyone behind the barrier. Locality-group discovery keyed on cache/index3 alone, so a part without an L3 made every core its own domain -- a flat barrier carrying S extra cache lines, while barrier_groups still reported S and read as if the grouping had engaged. It now takes the deepest cache level shared with another core, falling back to the NUMA node. Deucalion's A64FX nodes turn out to expose no cpu cache sysfs at all, so there the NUMA fallback is the only signal and yields 4 domains of 12 cores. On x86 the deepest shared level is still the CCX, so the measured placement is unchanged. "Shared" deliberately means shared with another core rather than with an SMT sibling: a per-core L1 or L2 lists both hardware threads, so a size>=2 test would have reinstated the same defect on any SMT part exposing no cross-core cache. Also: PartitionBarrier treats domains == participants as degenerate and takes the flat path, so barrier_groups can no longer report a level that is not running; parse_id no longer clamps to CPU_SETSIZE, which had collapsed every cpulist to empty on hosts with more CPUs than cpu_set_t can address; and placement now refuses outright rather than CPU_SET past CPU_SETSIZE, which is a silent no-op that would pin a partition to no core at all. Assisted-by: ClaudeCode:claude-opus-5 --- cpp/monoprop/detail/EnvConfig.h | 4 + cpp/monoprop/detail/mpi/CpuRelax.h | 36 ++- cpp/monoprop/detail/mpi/PartitionBarrier.h | 52 +++- cpp/monoprop/detail/partition/CpuTopology.h | 225 +++++++++++++++--- .../detail/partition/PartitionGroup.h | 35 ++- cpp/tests/cpu_topology_tests.cpp | 223 +++++++++++++++++ cpp/tests/env_config_tests.cpp | 12 + cpp/tests/partition_barrier_tests.cpp | 14 ++ docs/content/docs/features/parallelism.mdx | 18 +- 9 files changed, 560 insertions(+), 59 deletions(-) diff --git a/cpp/monoprop/detail/EnvConfig.h b/cpp/monoprop/detail/EnvConfig.h index 67f4fe40..f686619f 100644 --- a/cpp/monoprop/detail/EnvConfig.h +++ b/cpp/monoprop/detail/EnvConfig.h @@ -25,6 +25,8 @@ // monoprop_PARTITION_PINNING bool, default ON; 0/false disables per-core pinning → partition_pinning // monoprop_PARTITIONS int N | "auto" | "off"; parsed where it is used (resolve_partition_count_) // monoprop_COMM_PROFILE bool, default OFF; per-partition collective accounting to stderr → comm_profile +// monoprop_SPIN_BUDGET_US positive int us, default kDefaultSpinBudgetUs; barrier on-core spin before +// yielding. Exists to be swept against a real workload → spin_budget_us namespace monoprop::config { @@ -59,6 +61,7 @@ struct Settings { std::optional num_threads; bool partition_pinning = true; bool comm_profile = false; + std::optional spin_budget_us; // nullopt ⇒ the barrier's own default }; // Parse the environment once; the Settings are cached and shared across TUs. @@ -68,6 +71,7 @@ inline auto get() -> const Settings & { s.num_threads = detail::parse_positive_int(std::getenv("monoprop_NUM_THREADS")); s.partition_pinning = detail::parse_flag(std::getenv("monoprop_PARTITION_PINNING"), true); s.comm_profile = detail::parse_flag(std::getenv("monoprop_COMM_PROFILE"), false); + s.spin_budget_us = detail::parse_positive_int(std::getenv("monoprop_SPIN_BUDGET_US")); return s; }(); return settings; diff --git a/cpp/monoprop/detail/mpi/CpuRelax.h b/cpp/monoprop/detail/mpi/CpuRelax.h index 42ec2fd0..ce6dd8a6 100644 --- a/cpp/monoprop/detail/mpi/CpuRelax.h +++ b/cpp/monoprop/detail/mpi/CpuRelax.h @@ -35,8 +35,38 @@ inline auto cpu_relax() noexcept -> void { #endif } -// cpu_relax() iterations a barrier spinner burns before donating its timeslice. PAUSE is ~140 cycles on -// Sapphire Rapids ⇒ 2048 iters ≈ 0.1 ms, past a balanced exchange's arrival gaps; longer waits yield. -inline constexpr int kSpinPauseIters = 2048; +// How long a barrier spinner stays on-core before it starts yielding, in TIME rather than in cpu_relax() +// iterations. An iteration count does not carry across architectures: cpu_relax() is one PAUSE on x86 and +// one YIELD on aarch64, and those differ by more than an order of magnitude, so a count calibrated on one +// spends a wholly different budget on the other. On aarch64 the previous 2048-iteration budget came to +// roughly a microsecond -- the on-core spin was effectively skipped and every wait fell to sched_yield. +// +// 30 us is that old budget's equivalent on this Zen 2 part, measured rather than derived: a barrier stress +// at S=32 over 4 and 8 CPUs reproduces the old wall time at 30 us (0.130 s vs 0.153 s, 0.136 s vs 0.228 s) +// and drifts off it by 60 us. PAUSE here is therefore ~30 cycles, not the ~140 of Sapphire Rapids the old +// constant's comment was reasoning about. +// +// Which means one honest caveat: a single time budget cannot match the old spend on every x86 part, +// because the old spend itself varied with PAUSE latency -- that variance was the bug. On a part where +// PAUSE really is ~140 cycles the old constant bought ~100 us, so 30 us is a genuine shortening there, not +// a like-for-like port. It is calibrated where it was measured and where the benchmarks run; elsewhere +// monoprop_SPIN_BUDGET_US is the adjustment, and a part with a long PAUSE is the first place to reach for +// it. +// +// Retuning is a separate question, deliberately left to the cluster A/B: the same sweep has shorter +// budgets winning by 5-10x once partitions outnumber cores (10 us gives 0.028 s and 0.022 s), because a +// spinner holds the core its late peer needs to arrive on. Under the pinned one-partition-per-core layout +// the design targets, the trade runs the other way and staying on-core is the point -- +// monoprop_SPIN_BUDGET_US exists so that can be swept on real workloads without a rebuild. +// +// Parking (sleep_for) past a second, longer budget was tried and rejected: at S=32 on 4 CPUs one +// sleeper's timer overshoot pushed its peers past their own budgets, and the cascade cost 2021 us/sync +// against 762 for plain yielding. A yielder is runnable the moment a core frees; a sleeper cannot answer +// before its timer expires. +inline constexpr int kDefaultSpinBudgetUs = 30; + +// cpu_relax() calls between deadline checks. steady_clock::now() costs ~20 ns, which would dominate the +// pause it is meant to pace, so the check is amortized over a batch. +inline constexpr int kRelaxPerClockCheck = 64; } // namespace monoprop::mpi::detail diff --git a/cpp/monoprop/detail/mpi/PartitionBarrier.h b/cpp/monoprop/detail/mpi/PartitionBarrier.h index 58a1e326..05a893b0 100644 --- a/cpp/monoprop/detail/mpi/PartitionBarrier.h +++ b/cpp/monoprop/detail/mpi/PartitionBarrier.h @@ -16,10 +16,12 @@ #include #include +#include #include #include #include +#include "monoprop/detail/EnvConfig.h" #include "monoprop/detail/mpi/CpuRelax.h" namespace monoprop::mpi { @@ -34,10 +36,11 @@ class ShmCommPoisoned : public std::runtime_error { // word gets a private cache line: if `gen_` shared `arrived_`'s line, spinners' reloads would miss to // L3 on every peer arrival — O(S) coherence bounces (measured top hotspot at S=112). // -// Given a group id per participant (its L3 domain, from CpuTopology) the barrier becomes two-level: fan -// in within a domain, then across domains, then release within the domain. Both the arrival fetch_add -// and the release store then cost O(S/G) coherence transactions instead of O(S), and stay inside one L3 -// slice. No group ids (pinning off, or /sys unreadable) or a single domain degrades to the flat barrier. +// Given a group id per participant (its locality domain, from CpuTopology: the deepest shared cache, or +// the NUMA node) the barrier becomes two-level: fan in within a domain, then across domains, then release +// within the domain. Both the arrival fetch_add and the release store then cost O(S/G) coherence +// transactions instead of O(S), and stay inside one cache slice. No group ids (pinning off, or /sys +// unreadable), a single domain, or all-singleton domains degrade to the flat barrier. // // The acquire/release/relaxed orderings below are load-bearing, not an oversight: the release store to // `gen_` is what publishes the preceding relaxed reset of `arrived_`. Promoting them to seq_cst would @@ -62,7 +65,12 @@ class PartitionBarrier { group_of_[static_cast(p)] = static_cast(it - domains.begin()); } } - if (domains.size() < 2) { + // One domain has nothing to fan in across. So does a set of all-singleton domains, which is what + // an unreadable topology produces: every participant would be its own representative and they + // would all still meet at the root, i.e. the flat barrier plus a cache line and a release store + // per participant. Reporting both as flat also stops group_count() from claiming a level that is + // not actually running. + if (domains.size() < 2 || domains.size() == static_cast(participants)) { group_of_.clear(); return; // flat } @@ -116,7 +124,7 @@ class PartitionBarrier { } } - // L3 domains the participants were grouped into; < 2 means the flat barrier is in use. Reported by + // Locality domains the participants were grouped into; < 2 means the flat barrier is in use. Reported by // CommProfile so a run states which barrier it ran instead of leaving it inferred from the timing. auto group_count() const -> int { return groups_; } @@ -136,17 +144,33 @@ class PartitionBarrier { } private: - // Spin until `word` leaves `seen`. Bounded on-core spin first (pinned partitions ⇒ the release store - // lands in the pause window, no syscall); only long waits (imbalance, oversubscription) fall to - // yield. A poisoned peer releases us with an exception rather than a hang. + // Wait until `word` leaves `seen`, backing off through the two phases CpuRelax.h describes: on-core + // relax for a bounded time, then yield indefinitely. Both phases re-read `poisoned_` on every + // iteration, so an unwinding peer releases us with an exception rather than a hang and poison() needs + // no wakeup channel of its own. There is deliberately no third parking phase: sleeping past the yield + // budget measured 2021 vs 762 us/sync when oversubscribed, because one sleeper's timer overshoot + // delays every participant behind the barrier (see CpuRelax.h). auto spin_until_(const std::atomic &word, unsigned seen) const -> void { - int spins = 0; + // The deadline is computed lazily, after the first load fails. The overwhelmingly common case is + // that the release store has already landed, and a clock read per arrival is pure overhead on the + // path this barrier exists to keep cheap. + std::chrono::steady_clock::time_point spin_deadline{}; + bool have_deadline = false; + bool spinning = true; + int until_check = detail::kRelaxPerClockCheck; while (word.load(std::memory_order_acquire) == seen) { if (poisoned_.load(std::memory_order_acquire)) { throw ShmCommPoisoned(); } - if (spins < detail::kSpinPauseIters) { - ++spins; + if (!have_deadline) { + spin_deadline = std::chrono::steady_clock::now() + spin_budget_; + have_deadline = true; + } + if (spinning) { + if (--until_check <= 0) { + until_check = detail::kRelaxPerClockCheck; + spinning = std::chrono::steady_clock::now() < spin_deadline; + } detail::cpu_relax(); } else { @@ -163,6 +187,10 @@ class PartitionBarrier { std::atomic v{0}; }; + // Read once per barrier, not once per spin: monoprop_SPIN_BUDGET_US is a sweep knob, and the spin loop + // is the measured hotspot. + std::chrono::microseconds spin_budget_{config::get().spin_budget_us.value_or(detail::kDefaultSpinBudgetUs)}; + int participants_; int groups_ = 0; // < 2 ⇒ flat: arrived_/gen_ count participants, not domains std::vector group_of_; diff --git a/cpp/monoprop/detail/partition/CpuTopology.h b/cpp/monoprop/detail/partition/CpuTopology.h index 3f973b21..58212fcb 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.h +++ b/cpp/monoprop/detail/partition/CpuTopology.h @@ -15,6 +15,7 @@ #pragma once #include +#include #include #include "monoprop/detail/EnvConfig.h" @@ -25,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -33,7 +35,7 @@ #endif // CPU-topology helpers for partition placement (the one platform-specific file). Policy: one partition per -// physical core, spread across L3/ccx domains. The Linux fast path parses /sys and pins each master, +// physical core, spread across locality domains. The Linux fast path parses /sys and pins each master, // intersected with the process's allowed-CPU mask; elsewhere partitions run unpinned (still correct, no // locality win). @@ -41,7 +43,16 @@ namespace monoprop::detail::partition { struct PhysicalCore { int cpu = 0; // representative hardware thread (an allowed SMT sibling of the core) - int l3_domain = 0; + int domain = 0; +}; + +// How the launcher divided this host's CPUs among the ranks sharing it. Mask size alone cannot tell the +// two apart -- four ranks holding 32 cores each and four ranks sharing one 32-core mask both report 32 -- +// and they need opposite placement, so the caller that owns a node-local communicator measures this by +// exchanging masks rather than inferring it. See partition_cpusets and classify_node_mask. +enum class NodeMask { + Shared, ///< Every co-located rank sees the same mask (`mpirun --bind-to none`, or a lone process). + PerRank, ///< Each co-located rank was given its own disjoint slice (`srun --cpu-bind=cores`). }; #if defined(__linux__) @@ -50,7 +61,32 @@ using CpuSet = cpu_set_t; namespace topo_detail { -// Parse a Linux cpulist ("0-3,16-19") into the set of CPU ids it names. +// Widest range a single cpulist token may expand to. Larger than any real machine, small enough that a +// corrupt token cannot turn into an allocation. +inline constexpr int kMaxCpuListSpan = 1 << 20; + +// One non-negative integer, or nullopt when `text` is not exactly that. strtol rather than stoi because +// every caller here reads /sys, which a container's masked view or a truncated read can leave malformed: +// this path's contract is to degrade to "unpinned", never to throw out of a constructor. +inline auto parse_id(const std::string &text) -> std::optional { + if (text.empty()) { + return std::nullopt; + } + const char *begin = text.c_str(); + char *end = nullptr; + const long value = std::strtol(begin, &end, 10); + // Deliberately not bounded by CPU_SETSIZE: this parses cache levels and NUMA node ids as well as CPU + // ids, and a host with more than CPU_SETSIZE CPUs would otherwise have every cpulist it reports -- + // including /sys/.../cpu/online -- collapse to empty, silently disabling both pinning and the + // two-level barrier. Ids too large to place are dropped where CPU_SET is called instead. + if (end == begin || *end != '\0' || value < 0 || value > std::numeric_limits::max()) { + return std::nullopt; + } + return static_cast(value); +} + +// Parse a Linux cpulist ("0-3,16-19") into the set of CPU ids it names. Malformed tokens are skipped +// rather than thrown on; see parse_id. inline auto parse_cpulist(const std::string &text) -> std::vector { std::vector out; std::stringstream ss(text); @@ -58,16 +94,21 @@ inline auto parse_cpulist(const std::string &text) -> std::vector { while (std::getline(ss, tok, ',')) { const auto dash = tok.find('-'); if (dash == std::string::npos) { - if (!tok.empty()) { - out.push_back(std::stoi(tok)); + if (const auto one = parse_id(tok)) { + out.push_back(*one); } + continue; } - else { - const int lo = std::stoi(tok.substr(0, dash)); - const int hi = std::stoi(tok.substr(dash + 1)); - for (int c = lo; c <= hi; ++c) { - out.push_back(c); - } + const auto lo = parse_id(tok.substr(0, dash)); + const auto hi = parse_id(tok.substr(dash + 1)); + // A reversed range names nothing, and an absurdly wide one is garbage rather than a machine: with + // parse_id no longer clamped to CPU_SETSIZE, expanding it unchecked would allocate on the strength + // of a malformed /sys read. + if (!lo || !hi || *hi < *lo || *hi - *lo > kMaxCpuListSpan) { + continue; + } + for (int c = *lo; c <= *hi; ++c) { + out.push_back(c); } } return out; @@ -98,19 +139,70 @@ inline auto allowed_cpus() -> std::set { return allowed; } +// The CPUs sharing the deepest cache level that groups `cpu_base`'s core with another core, else the CPUs +// of its NUMA node, else empty. `siblings` is the core's SMT sibling list (empty ⇒ just `cpu`). +// +// Deliberately not keyed on index3/L3, and the NUMA fallback is not a nicety. Measured on Deucalion's +// A64FX nodes: /sys/devices/system/cpu/cpuN/cache does not exist at all -- no level is reported, not just +// no L3 -- so the cache walk finds nothing and the NUMA node is the only signal left. Keying on L3 made +// every core its own domain there, which turns the two-level barrier into a flat one carrying S extra +// cache lines while barrier_groups still reports S, reading as if the optimization had engaged. On x86 the +// deepest shared level *is* L3, so this keeps the CCX grouping the current placement was measured against. +inline auto shared_domain_cpus(const std::string &cpu_base, int cpu, const std::vector &siblings) + -> std::vector { + const std::vector own = siblings.empty() ? std::vector{cpu} : siblings; + std::vector deepest; + int deepest_level = -1; + // Cache indices are contiguous, so the first unreadable one ends the list. + for (int idx = 0;; ++idx) { + const std::string dir = cpu_base + "/cache/index" + std::to_string(idx); + const auto level = parse_id(read_line(dir + "/level")); + if (!level) { + break; + } + const auto members = parse_cpulist(read_line(dir + "/shared_cpu_list")); + // "Shared" has to mean shared with another *core*, not with our own SMT sibling: a per-core L1 or + // L2 still lists every hardware thread of the core, so a size>=2 test would accept it, make each + // core its own domain, and never reach the NUMA fallback. That would reintroduce the exact defect + // this function exists to fix on any SMT part that exposes no shared cache. + const bool groups_another_core = std::any_of(members.begin(), members.end(), [&](int m) { + return std::find(own.begin(), own.end(), m) == own.end(); + }); + if (!groups_another_core) { + continue; + } + if (*level > deepest_level) { + deepest_level = *level; + deepest = members; + } + } + if (!deepest.empty()) { + return deepest; + } + // No shared cache reported. The NUMA node is the next-coarsest thing a barrier can stay inside. + for (int node : parse_cpulist(read_line("/sys/devices/system/node/possible"))) { + const auto cpus = parse_cpulist(read_line("/sys/devices/system/node/node" + std::to_string(node) + "/cpulist")); + if (std::find(cpus.begin(), cpus.end(), cpu) != cpus.end()) { + return cpus; + } + } + return {}; +} + } // namespace topo_detail -// Enumerate physical cores (one per smt sibling group) the process is allowed to use, tagged with their -// L3 domain. A core is included iff a sibling is in the allowed mask, with the smallest allowed sibling -// as representative, so a partial allocation never pins outside the mask. Empty if /sys cannot be read. +// Enumerate physical cores (one per smt sibling group) the process is allowed to use, tagged with the +// locality domain they share (deepest shared cache, else NUMA node; see shared_domain_cpus). A core is +// included iff a sibling is in the allowed mask, with the smallest allowed sibling as representative, so +// a partial allocation never pins outside the mask. Empty if /sys cannot be read. inline auto enumerate_physical_cores() -> std::vector { const std::set allowed = topo_detail::allowed_cpus(); const bool filter = !allowed.empty(); // no mask readable ⇒ accept every CPU const auto is_allowed = [&](int cpu) { return !filter || allowed.contains(cpu); }; std::vector cores; - std::set seen_cores; // sibling-group key (min sibling) already recorded - std::vector> l3_members; // cpu-set per distinct L3 domain, in discovery order + std::set seen_cores; // sibling-group key (min sibling) already recorded + std::vector> domain_members; // cpu-set per distinct domain, in discovery order // Scan a bounded id range rather than stopping at the first gap: online CPU ids are not contiguous // (offlined or hot-plugged CPUs leave holes), and breaking on the first unreadable id truncates the @@ -145,43 +237,95 @@ inline auto enumerate_physical_cores() -> std::vector { continue; } - const auto l3 = topo_detail::parse_cpulist(topo_detail::read_line(base + "/cache/index3/shared_cpu_list")); + const auto shared = topo_detail::shared_domain_cpus(base, cpu, siblings); int domain = -1; - for (size_t d = 0; d < l3_members.size(); ++d) { - if (std::find(l3_members[d].begin(), l3_members[d].end(), group_key) != l3_members[d].end()) { + for (size_t d = 0; d < domain_members.size(); ++d) { + if (std::find(domain_members[d].begin(), domain_members[d].end(), group_key) != domain_members[d].end()) { domain = static_cast(d); break; } } if (domain < 0) { - domain = static_cast(l3_members.size()); - l3_members.push_back(l3.empty() ? std::vector{group_key} : l3); + domain = static_cast(domain_members.size()); + domain_members.push_back(shared.empty() ? std::vector{group_key} : shared); } cores.push_back(PhysicalCore{rep, domain}); } return cores; } +// This process's current affinity mask, for a caller that needs to compare it against its peers'. An +// unreadable mask comes back empty, which classify_node_mask reads as "cannot tell" ⇒ Shared. +inline auto this_thread_cpuset() -> CpuSet { + CpuSet set; + CPU_ZERO(&set); + if (sched_getaffinity(0, sizeof(set), &set) != 0) { + CPU_ZERO(&set); + } + return set; +} + +// Classify how the launcher divided the host, given every co-located rank's mask. The caller gathers them +// because it owns the node-local communicator and this header stays free of MPI. +// +// Pairwise disjoint and non-empty ⇒ PerRank. Anything else -- identical masks, partial overlap, or a mask +// that could not be read -- ⇒ Shared, which is the conservative answer; see partition_cpusets. +inline auto classify_node_mask(const std::vector &masks) -> NodeMask { + if (masks.size() < 2) { + return NodeMask::Shared; // nobody to be disjoint from + } + for (size_t a = 0; a < masks.size(); ++a) { + if (CPU_COUNT(&masks[a]) == 0) { + return NodeMask::Shared; + } + for (size_t b = a + 1; b < masks.size(); ++b) { + for (int c = 0; c < CPU_SETSIZE; ++c) { + if (CPU_ISSET(c, &masks[a]) && CPU_ISSET(c, &masks[b])) { + return NodeMask::Shared; // shares a CPU ⇒ not a per-rank split + } + } + } + } + return NodeMask::PerRank; +} + // Build `n` partition cpusets, one physical core each. `group_index`/`group_count` place one MPI rank's -// partitions among the ranks sharing this host, spread across L3 domains and disjoint from the other ranks' +// partitions among the ranks sharing this host, spread across locality domains and disjoint from the other ranks' // — two ranks must never share a core (one rank's busy-polling collectives would starve the other's // barrier spins). Empty (⇒ unpinned) if the host lacks group_count*n cores. -inline auto partition_cpusets(size_t n, size_t group_index = 0, size_t group_count = 1) -> std::vector { +inline auto partition_cpusets(size_t n, + size_t group_index = 0, + size_t group_count = 1, + NodeMask mask = NodeMask::Shared) -> std::vector { if (!config::get().partition_pinning) { return {}; } + // enumerate_physical_cores() reports only cores inside this process's mask, so under a PerRank mask the + // launcher has already handed each co-located rank a disjoint slice, and dividing by group_count a + // second time splits an already-split machine: group_count * n exceeds the share, the guard below reads + // that as "host too small", and every rank silently runs unpinned -- taking the two-level barrier's + // domains with it, because cpuset_domains() derives them from these sets. Partition the whole slice and + // leave the cross-rank split to whoever imposed it. + // + // The default is Shared because that is the pre-existing behaviour and the safe error direction: + // collapsing a genuinely shared mask would point every co-located rank at the *same* cores, breaking + // the disjointness invariant above, whereas dividing a PerRank mask only costs pinning. + if (mask == NodeMask::PerRank) { + group_index = 0; + group_count = 1; + } const auto cores = enumerate_physical_cores(); if (cores.empty() || group_count * n > cores.size()) { return {}; } int max_domain = 0; for (const auto &c : cores) { - max_domain = std::max(max_domain, c.l3_domain); + max_domain = std::max(max_domain, c.domain); } // Ordering: interleaved for a lone process, contiguous blocks for co-located ranks. std::vector> by_domain(static_cast(max_domain) + 1); for (const auto &c : cores) { - by_domain[static_cast(c.l3_domain)].push_back(c.cpu); + by_domain[static_cast(c.domain)].push_back(c.cpu); } // Interleave `buckets` depth-first: bucket0[0], bucket1[0], …, bucket0[1], bucket1[1], … const auto interleave = [](const std::vector> &buckets) { @@ -211,7 +355,7 @@ inline auto partition_cpusets(size_t n, size_t group_index = 0, size_t group_cou order = interleave(mine); } else { - // More co-located ranks than L3 domains: flat domain-major order, one contiguous slice each. + // More co-located ranks than domains: flat domain-major order, one contiguous slice each. for (const auto &bucket : by_domain) { order.insert(order.end(), bucket.begin(), bucket.end()); } @@ -223,16 +367,23 @@ inline auto partition_cpusets(size_t n, size_t group_index = 0, size_t group_cou std::vector sets(n); for (size_t i = 0; i < n; ++i) { + const int cpu = order[offset + i]; + // cpu_set_t addresses only CPU_SETSIZE CPUs, and CPU_SET past that is a silent no-op that would + // leave an empty set pinning nothing. Refuse the whole placement instead: unpinned is a documented + // outcome, whereas "pinned to no core" is not. + if (cpu < 0 || cpu >= CPU_SETSIZE) { + return {}; + } CPU_ZERO(&sets[i]); - CPU_SET(order[offset + i], &sets[i]); + CPU_SET(cpu, &sets[i]); } return sets; } -// The L3 domain each partition cpuset lands in, in partition_cpusets order -- what a two-level +// The locality domain each partition cpuset lands in, in partition_cpusets order -- what a two-level // PartitionBarrier groups by. Derived from the sets, not from the placement logic, so the two cannot // drift apart. Empty (⇒ flat barrier) if the sets are empty or one names no core the scan knows. -inline auto cpuset_l3_domains(const std::vector &sets) -> std::vector { +inline auto cpuset_domains(const std::vector &sets) -> std::vector { if (sets.empty()) { return {}; } @@ -243,7 +394,7 @@ inline auto cpuset_l3_domains(const std::vector &sets) -> std::vector std::vector { return {}; } -inline auto partition_cpusets(size_t /*n*/, size_t /*group_index*/ = 0, size_t /*group_count*/ = 1) - -> std::vector { +inline auto partition_cpusets(size_t /*n*/, + size_t /*group_index*/ = 0, + size_t /*group_count*/ = 1, + NodeMask /*mask*/ = NodeMask::Shared) -> std::vector { return {}; } -inline auto cpuset_l3_domains(const std::vector & /*sets*/) -> std::vector { +inline auto this_thread_cpuset() -> CpuSet { + return {}; +} +inline auto classify_node_mask(const std::vector & /*masks*/) -> NodeMask { + return NodeMask::Shared; +} +inline auto cpuset_domains(const std::vector & /*sets*/) -> std::vector { return {}; } inline auto pin_this_thread(const CpuSet & /*set*/) -> void {} diff --git a/cpp/monoprop/detail/partition/PartitionGroup.h b/cpp/monoprop/detail/partition/PartitionGroup.h index d2661402..069ee3c3 100644 --- a/cpp/monoprop/detail/partition/PartitionGroup.h +++ b/cpp/monoprop/detail/partition/PartitionGroup.h @@ -58,7 +58,7 @@ class PartitionGroup { partitions_(static_cast(n_partitions)), errs_(static_cast(n_partitions)) { // Placement is decided before the transport, because the transport's barrier is grouped by the - // L3 domain each partition will be pinned to. + // locality domain each partition will be pinned to. discover_node_peers_(); cpusets_ = topo_partition_cpusets(n_, node_rank_, node_size_); make_transport_(); @@ -81,6 +81,9 @@ class PartitionGroup { parent_(src.parent_), node_rank_(src.node_rank_), node_size_(src.node_size_), + // Copied, not re-measured: discover_node_peers_() is collective over the parent and this ctor is + // not. Losing it would silently place the clone differently from the group it was copied from. + node_mask_(src.node_mask_), partitions_(static_cast(src.n_)), errs_(static_cast(src.n_)) { cpusets_ = topo_partition_cpusets(n_, node_rank_, node_size_); @@ -148,15 +151,17 @@ class PartitionGroup { } // Free-function wrapper so the header compiles on non-Linux (where partition_cpusets returns {}). - static auto topo_partition_cpusets(int n, int group_index, int group_count) + auto topo_partition_cpusets(int n, int group_index, int group_count) const -> std::vector { return monoprop::detail::partition::partition_cpusets(static_cast(n), static_cast(group_index), - static_cast(group_count)); + static_cast(group_count), + node_mask_); } // Under an MPI parent, find how many ranks share this host and which we are, so each co-located rank - // pins to a disjoint core block (see partition_cpusets). Collective over `parent`; clones copy the result. + // pins to a disjoint core block (see partition_cpusets), and whether the launcher already sliced the + // host per rank. Collective over `parent`; clones copy the result. auto discover_node_peers_() -> void { #ifdef monoprop_ENABLE_MPI if (parent_.kind == mpi::Comm::Kind::Mpi && mpi::size(parent_) > 1) { @@ -164,6 +169,17 @@ class PartitionGroup { MPI_Comm_split_type(parent_.mpi, MPI_COMM_TYPE_SHARED, 0, MPI_INFO_NULL, &node); MPI_Comm_rank(node, &node_rank_); MPI_Comm_size(node, &node_size_); + // Exchange the raw affinity masks while the node communicator is open. Measured rather than + // inferred because mask *size* cannot distinguish "8 ranks holding 16 cores each" from "8 ranks + // sharing one 16-core mask": both leave a rank seeing 16 of the host's 128 CPUs, and the two + // need opposite placement. Guessing it wrong in the collapsing direction points every + // co-located rank at the same cores, which is worse than not pinning at all. + if (node_size_ > 1) { + const auto mine = monoprop::detail::partition::this_thread_cpuset(); + std::vector all(static_cast(node_size_)); + MPI_Allgather(&mine, sizeof(mine), MPI_BYTE, all.data(), sizeof(mine), MPI_BYTE, node); + node_mask_ = monoprop::detail::partition::classify_node_mask(all); + } MPI_Comm_free(&node); } #endif @@ -172,7 +188,7 @@ class PartitionGroup { auto make_transport_() -> void { // Empty unless the partitions are pinned and /sys was readable ⇒ flat barrier (see // PartitionBarrier); cpusets_ must already be set. - const std::vector domains = monoprop::detail::partition::cpuset_l3_domains(cpusets_); + const std::vector domains = monoprop::detail::partition::cpuset_domains(cpusets_); #ifdef monoprop_ENABLE_MPI if (parent_.kind == mpi::Comm::Kind::Mpi && mpi::size(parent_) > 1) { hyb_ = std::make_unique(parent_.mpi, n_, domains); @@ -255,9 +271,12 @@ class PartitionGroup { } int n_; - mpi::Comm parent_; // enclosing communicator (size R) — decides the transport - int node_rank_ = 0; // this rank's index among the ranks sharing the host - int node_size_ = 1; // how many parent ranks share the host (1 unless MPI R>1) + mpi::Comm parent_; // enclosing communicator (size R) — decides the transport + int node_rank_ = 0; // this rank's index among the ranks sharing the host + int node_size_ = 1; // how many parent ranks share the host (1 unless MPI R>1) + // How the launcher divided the host among those ranks, measured in discover_node_peers_(). Shared until + // measured, which is both the single-rank truth and the safe default (see partition_cpusets). + monoprop::detail::partition::NodeMask node_mask_ = monoprop::detail::partition::NodeMask::Shared; std::unique_ptr shm_; // set iff R == 1 #ifdef monoprop_ENABLE_MPI std::unique_ptr hyb_; // set iff R > 1 diff --git a/cpp/tests/cpu_topology_tests.cpp b/cpp/tests/cpu_topology_tests.cpp index 55816317..2b3b2e9f 100644 --- a/cpp/tests/cpu_topology_tests.cpp +++ b/cpp/tests/cpu_topology_tests.cpp @@ -18,8 +18,11 @@ #include #include +#include +#include #include #include +#include #include #include "monoprop/detail/partition/CpuTopology.h" @@ -74,6 +77,8 @@ BOOST_AUTO_TEST_CASE(cpu_topology_place_co_located_ranks) { #if defined(__linux__) +#include + using partition::topo_detail::parse_cpulist; using partition::topo_detail::read_line; @@ -137,4 +142,222 @@ BOOST_AUTO_TEST_CASE(cpu_topology_allowed_cpus_nonempty_on_ci) { BOOST_CHECK(!allowed.empty()); } +// Restores the caller's affinity mask however the scope exits, so a failing check cannot leave the rest +// of the suite pinned to two cores. +class ScopedAffinity { +public: + ScopedAffinity() { pinned_ = sched_getaffinity(0, sizeof(saved_), &saved_) == 0; } + ScopedAffinity(const ScopedAffinity &) = delete; + auto operator=(const ScopedAffinity &) -> ScopedAffinity & = delete; + ~ScopedAffinity() { + if (pinned_) { + sched_setaffinity(0, sizeof(saved_), &saved_); + } + } + +private: + cpu_set_t saved_{}; + bool pinned_ = false; +}; + +// A rank whose mask is its own slice of the node must still get a placement. Slurm hands each co-located +// rank a disjoint mask and then tells the rank group_count = ranks-per-node; enumerate_physical_cores() +// already filtered by the mask, so re-dividing pushed group_count * n past the share and answered +// "unpinned", which also flattened the two-level barrier (measured on this cluster as barrier_groups=0 and +// 437 vs 15.5 us/sync at 8 ranks x 16 partitions). +BOOST_AUTO_TEST_CASE(cpu_topology_per_rank_mask_still_places) { + const auto full = partition::enumerate_physical_cores(); + if (full.size() < 2) { + return; // too small to simulate a slice of a larger host + } + + const ScopedAffinity restore_on_exit; + + // Stand in for `srun --cpus-per-task=2`: keep two physical cores of a host that has more. + cpu_set_t confined; + CPU_ZERO(&confined); + CPU_SET(full[0].cpu, &confined); + CPU_SET(full[1].cpu, &confined); + if (sched_setaffinity(0, sizeof(confined), &confined) != 0) { + return; // not permitted here (seccomp, restrictive cgroup); nothing to assert + } + + // n = the whole share, and a group_count as if seven sibling ranks shared the node. + const auto sets = + partition::partition_cpusets(/*n=*/2, /*group_index=*/3, /*group_count=*/8, partition::NodeMask::PerRank); + BOOST_CHECK_EQUAL(sets.size(), 2u); + for (const cpu_set_t &set : sets) { + // Never pin outside the mask the launcher gave us. + BOOST_CHECK(CPU_ISSET(full[0].cpu, &set) || CPU_ISSET(full[1].cpu, &set)); + BOOST_CHECK_EQUAL(CPU_COUNT(&set), 1); + } + // One domain entry per set, and the two sets must not be the same core. + BOOST_CHECK_EQUAL(partition::cpuset_domains(sets).size(), 2u); + BOOST_CHECK(!CPU_EQUAL(&sets[0], &sets[1])); +} + +// The invariant the PerRank collapse above most endangers: under a genuinely Shared mask, two co-located +// ranks must still deal themselves DIFFERENT cores. Collapsing unconditionally on "the mask is narrower +// than the machine" pointed every rank at the same cores, because mask width cannot distinguish a per-rank +// slice from a shared one -- which is why the caller measures it instead (see classify_node_mask). +BOOST_AUTO_TEST_CASE(cpu_topology_shared_mask_keeps_co_located_ranks_disjoint) { + const auto full = partition::enumerate_physical_cores(); + if (full.size() < 4) { + return; // need two cores per rank for two ranks + } + + const ScopedAffinity restore_on_exit; + + // A shared step mask narrower than the host: four cores that both ranks can see. + cpu_set_t shared; + CPU_ZERO(&shared); + for (size_t i = 0; i < 4; ++i) { + CPU_SET(full[i].cpu, &shared); + } + if (sched_setaffinity(0, sizeof(shared), &shared) != 0) { + return; + } + + const auto rank0 = + partition::partition_cpusets(/*n=*/2, /*group_index=*/0, /*group_count=*/2, partition::NodeMask::Shared); + const auto rank1 = + partition::partition_cpusets(/*n=*/2, /*group_index=*/1, /*group_count=*/2, partition::NodeMask::Shared); + BOOST_CHECK_EQUAL(rank0.size(), 2u); + BOOST_CHECK_EQUAL(rank1.size(), 2u); + for (const cpu_set_t &a : rank0) { + for (const cpu_set_t &b : rank1) { + // Two ranks sharing a core would have each one's busy-polling collectives starve the other's + // barrier spins -- the failure this whole placement path exists to avoid. + BOOST_CHECK(!CPU_EQUAL(&a, &b)); + } + } +} + +// Two ranks holding disjoint masks are a per-rank split; identical masks are a shared one. Nothing else +// distinguishes them, which is the whole reason this is measured rather than inferred. +BOOST_AUTO_TEST_CASE(cpu_topology_classify_node_mask_disjoint_vs_identical) { + cpu_set_t a; + cpu_set_t b; + CPU_ZERO(&a); + CPU_ZERO(&b); + CPU_SET(0, &a); + CPU_SET(1, &a); + CPU_SET(2, &b); + CPU_SET(3, &b); + BOOST_CHECK(partition::classify_node_mask({a, b}) == partition::NodeMask::PerRank); + BOOST_CHECK(partition::classify_node_mask({a, a}) == partition::NodeMask::Shared); + + // Partial overlap is pathological; Shared is the conservative answer because dividing never + // double-books a core within a rank, whereas collapsing points every rank at the same cores. + cpu_set_t c = a; + CPU_SET(2, &c); + BOOST_CHECK(partition::classify_node_mask({a, c}) == partition::NodeMask::Shared); + + // An unreadable mask arrives empty and must not be read as "disjoint from everything". + cpu_set_t empty; + CPU_ZERO(&empty); + BOOST_CHECK(partition::classify_node_mask({a, empty}) == partition::NodeMask::Shared); + // A lone rank has nobody to be disjoint from. + BOOST_CHECK(partition::classify_node_mask({a}) == partition::NodeMask::Shared); +} + +// Malformed and out-of-range /sys content. This path's contract is to degrade to "unpinned", never to +// throw out of a constructor, and never to answer with a cpulist it half-understood. +BOOST_AUTO_TEST_CASE(cpu_topology_parse_cpulist_rejects_malformed_ranges) { + BOOST_CHECK(parse_cpulist("3-0").empty()); // reversed range names nothing + BOOST_CHECK(parse_cpulist("0-").empty()); // missing endpoint drops the token + BOOST_CHECK(parse_cpulist("-3").empty()); // ditto + BOOST_CHECK(parse_cpulist("abc").empty()); // non-numeric + BOOST_CHECK(parse_cpulist("1-2x").empty()); // trailing junk rejects the whole token + BOOST_TEST(parse_cpulist("7,3-0,9") == (std::vector{7, 9}), boost::test_tools::per_element()); + + // Ids at and past CPU_SETSIZE must still PARSE: parse_id also reads cache levels and NUMA node ids, + // and a host with more CPUs than cpu_set_t can address would otherwise have every cpulist it reports + // -- including cpu/online -- collapse to empty, silently disabling pinning and the two-level barrier. + // Placement drops them where CPU_SET is called instead. + BOOST_TEST(parse_cpulist(std::to_string(CPU_SETSIZE)) == (std::vector{CPU_SETSIZE}), + boost::test_tools::per_element()); + BOOST_CHECK_EQUAL(parse_cpulist("0-2047").size(), 2048u); + + // An absurdly wide range is garbage rather than a machine, and must not become an allocation. + BOOST_CHECK(parse_cpulist("0-999999999").empty()); +} + +// Fixture-driven cover for shared_domain_cpus, which is otherwise exercised only through this host's own +// /sys -- an x86 part that does have an L3, so neither the shared-L2 shape nor the SMT trap below is +// reachable from real hardware here. `cpu_base` is a parameter precisely so this is testable. +namespace { + +// Write a synthetic /sys-shaped cache tree. `levels` is (level, shared_cpu_list) in index order. +auto write_cache_fixture(const std::filesystem::path &base, const std::vector> &levels) + -> void { + for (size_t i = 0; i < levels.size(); ++i) { + const auto dir = base / "cache" / ("index" + std::to_string(i)); + std::filesystem::create_directories(dir); + std::ofstream(dir / "level") << levels[i].first << "\n"; + std::ofstream(dir / "type") << "Unified\n"; + std::ofstream(dir / "shared_cpu_list") << levels[i].second << "\n"; + } +} + +// Removes the fixture tree however the scope exits. +class ScopedTree { +public: + explicit ScopedTree(std::string name) + : path_(std::filesystem::temp_directory_path() / ("monoprop-topo-" + std::move(name))) { + std::filesystem::remove_all(path_); + } + ScopedTree(const ScopedTree &) = delete; + auto operator=(const ScopedTree &) -> ScopedTree & = delete; + ~ScopedTree() { std::filesystem::remove_all(path_); } + auto path() const -> const std::filesystem::path & { return path_; } + +private: + std::filesystem::path path_; +}; + +} // namespace + +BOOST_AUTO_TEST_CASE(cpu_topology_shared_domain_prefers_deepest_cross_core_level) { + const ScopedTree tree("deepest"); + // L1 and L2 private to the core (they still list both SMT threads), L3 spanning four cores. + write_cache_fixture(tree.path(), {{1, "0,64"}, {2, "0,64"}, {3, "0-3,64-67"}}); + const auto got = partition::topo_detail::shared_domain_cpus(tree.path().string(), 0, {0, 64}); + BOOST_TEST(got == (std::vector{0, 1, 2, 3, 64, 65, 66, 67}), boost::test_tools::per_element()); +} + +// The SMT trap: a per-core L1/L2 lists every hardware thread of the core, so a "shared by >= 2 CPUs" test +// accepts it, makes each core its own domain, and never reaches the NUMA fallback. On an SMT part exposing +// no cross-core cache that silently reinstates the defect this function exists to fix. +BOOST_AUTO_TEST_CASE(cpu_topology_shared_domain_ignores_smt_siblings) { + const ScopedTree tree("smt"); + write_cache_fixture(tree.path(), {{1, "0,64"}, {2, "0,64"}}); + const std::vector siblings{0, 64}; + const auto got = partition::topo_detail::shared_domain_cpus(tree.path().string(), 0, siblings); + // Must not have selected the sibling-only level. Whatever it returns comes from the NUMA fallback, so + // it either found nothing or found a set wider than this one core. + BOOST_CHECK(got != siblings); +} + +// Levels are selected by reported cache level, not by index order, so a /sys that lists them out of order +// still groups by the deepest one. +BOOST_AUTO_TEST_CASE(cpu_topology_shared_domain_selects_by_level_not_index) { + const ScopedTree tree("order"); + write_cache_fixture(tree.path(), {{3, "0-3"}, {1, "0"}, {2, "0-1"}}); + const auto got = partition::topo_detail::shared_domain_cpus(tree.path().string(), 0, {0}); + BOOST_TEST(got == (std::vector{0, 1, 2, 3}), boost::test_tools::per_element()); +} + +// A64FX-shaped: /sys/devices/system/cpu/cpuN/cache does not exist at all on Deucalion's ARM nodes -- no +// level is reported, not just no L3 -- so the NUMA node is the only signal left. Verified on cna0001. +BOOST_AUTO_TEST_CASE(cpu_topology_shared_domain_falls_back_when_no_cache_tree) { + const ScopedTree tree("nocache"); + std::filesystem::create_directories(tree.path()); // exists, but with no cache/ inside + const auto got = partition::topo_detail::shared_domain_cpus(tree.path().string(), 0, {0}); + // Falls through to this host's real NUMA topology: either unreadable (empty) or a node containing cpu0. + if (!got.empty()) { + BOOST_CHECK(std::find(got.begin(), got.end(), 0) != got.end()); + } +} + #endif // __linux__ diff --git a/cpp/tests/env_config_tests.cpp b/cpp/tests/env_config_tests.cpp index 77ff0955..0d440379 100644 --- a/cpp/tests/env_config_tests.cpp +++ b/cpp/tests/env_config_tests.cpp @@ -17,6 +17,7 @@ #include #include "monoprop/detail/EnvConfig.h" +#include "monoprop/detail/mpi/CpuRelax.h" using monoprop::config::detail::parse_flag; using monoprop::config::detail::parse_positive_int; @@ -59,6 +60,17 @@ BOOST_AUTO_TEST_CASE(env_config_parse_positive_int_range) { BOOST_CHECK(parse_positive_int("1000000") == std::optional(1'000'000)); // inclusive upper bound } +// monoprop_SPIN_BUDGET_US shares parse_positive_int, so what it adds is the fallback: an unset or +// malformed value must leave the barrier on its own default rather than on a zero-length spin. +BOOST_AUTO_TEST_CASE(env_config_spin_budget_falls_back_to_barrier_default) { + BOOST_CHECK(parse_positive_int(nullptr).value_or(monoprop::mpi::detail::kDefaultSpinBudgetUs) + == monoprop::mpi::detail::kDefaultSpinBudgetUs); + BOOST_CHECK(parse_positive_int("0").value_or(monoprop::mpi::detail::kDefaultSpinBudgetUs) + == monoprop::mpi::detail::kDefaultSpinBudgetUs); + BOOST_CHECK(parse_positive_int("25").value_or(monoprop::mpi::detail::kDefaultSpinBudgetUs) == 25); + BOOST_CHECK_GT(monoprop::mpi::detail::kDefaultSpinBudgetUs, 0); +} + BOOST_AUTO_TEST_CASE(env_config_settings_cached_singleton) { const auto &a = monoprop::config::get(); const auto &b = monoprop::config::get(); diff --git a/cpp/tests/partition_barrier_tests.cpp b/cpp/tests/partition_barrier_tests.cpp index dcc31591..b556381c 100644 --- a/cpp/tests/partition_barrier_tests.cpp +++ b/cpp/tests/partition_barrier_tests.cpp @@ -98,6 +98,20 @@ BOOST_AUTO_TEST_CASE(partition_barrier_short_group_list_falls_back) { no_errors(stamp_rounds(4, {0, 1}, 20)); } +// All-singleton domains are what an unreadable cache/NUMA topology produces. The two-level path would +// then have every participant represent itself at the root -- the flat barrier plus a cache line and an +// extra release store each -- so it must report and behave as flat, and group_count() must not claim a +// level that is not running. +BOOST_AUTO_TEST_CASE(partition_barrier_all_singleton_groups_are_flat) { + const PartitionBarrier singletons(4, {0, 1, 2, 3}); + BOOST_CHECK_LT(singletons.group_count(), 2); + no_errors(stamp_rounds(4, {0, 1, 2, 3}, 20)); + + // A grouping with real fan-in still engages, so the guard has not disabled the optimization wholesale. + const PartitionBarrier grouped(4, {0, 0, 1, 1}); + BOOST_CHECK_EQUAL(grouped.group_count(), 2); +} + // poison() must release waiters in BOTH levels: a peer that throws mid-round leaves the others parked // on their domain's word, not the root's. BOOST_AUTO_TEST_CASE(partition_barrier_poison_releases_grouped_waiters) { diff --git a/docs/content/docs/features/parallelism.mdx b/docs/content/docs/features/parallelism.mdx index 943626b8..22ea16c5 100644 --- a/docs/content/docs/features/parallelism.mdx +++ b/docs/content/docs/features/parallelism.mdx @@ -22,9 +22,20 @@ gives each partition a pinned worker thread that runs it serially. Each partitio keeps its own small, cache-resident term index; a gate is applied to all partitions at once, synchronised by a lightweight barrier, and anticommuting terms whose partner lives in another partition are resolved through a per-gate exchange. -When the partitions are pinned and span more than one L3 domain, that barrier fans in -within each domain and then across domains, so both the arrival counter and the -release store stay inside one L3 slice instead of crossing the socket interconnect. +When the partitions are pinned and span more than one locality domain, that barrier +fans in within each domain and then across domains, so both the arrival counter and +the release store stay inside one cache slice instead of crossing the socket +interconnect. A domain is the deepest cache level that more than one core shares — +an L3 slice or CCX on x86 — falling back to the NUMA node on parts that report no +shared cache at all. + +Placement respects whatever CPU mask the process was launched under. When several +ranks share a node, they compare masks with each other to decide how the node was +divided: given a mask each (`srun --cpu-bind=cores --cpus-per-task=N`), every rank +partitions the cores it was actually given, and given one mask between them +(`mpirun --bind-to none`), they divide it so that no two ranks land on the same +core. The two cases look identical from a single rank — each sees fewer CPUs than +the machine has — which is why they are measured rather than guessed. ## Runtime environment variables @@ -34,6 +45,7 @@ release store stay inside one L3 slice instead of crossing the socket interconne | `monoprop_PARTITIONS` | `auto` | `auto` = one partition per core (capped by `monoprop_NUM_THREADS`); an integer `N` = exactly `N` partitions; `off` = one partition holding the whole operator. | | `monoprop_PARTITION_PINNING` | `on` | `0`/`false`/`no` disables pinning each partition to a core. Has an effect only on Linux. | | `monoprop_COMM_PROFILE` | `off` | `1`/`true`/`yes` makes each transport print one `COMMPROF` line per rank to stderr when it is destroyed, accounting for where its collectives' wall time went. Diagnostic only; off costs one branch per collective phase. | +| `monoprop_SPIN_BUDGET_US` | `30` | Microseconds a partition spends spinning on-core at a barrier before it starts yielding. Longer suits a pinned one-partition-per-core layout; shorter suits an oversubscribed one, where a spinner holds the core its late peer needs. A tuning knob — the default is calibrated, not arbitrary. | ```bash # Run 8 partitions instead of one-per-core: From 352aea9f86857cc193645951b6d113bcc2d9f584 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Wed, 12 Aug 2026 14:04:02 +0100 Subject: [PATCH 07/17] =?UTF-8?q?fix(mpi):=20=F0=9F=90=9B=20make=20Pending?= =?UTF-8?q?Alltoallv=20move-only=20and=20self-completing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The handle owns the send/recv buffers that a posted MPI_Ialltoallv writes directly into, but it was a plain aggregate: copyable, and destructible without ever completing the request. Both are latent memory corruption on the Kind::Mpi async path. A copy handed two owners the same MPI_Request, so the second wait_into would wait on a request the first had already completed and set to MPI_REQUEST_NULL in its own copy -- and both copies' buffers were live targets of one transfer. Nothing copies the handle today (both call sites are `auto h = begin_alltoallv(...)`), which is why this never surfaced; deleting the copy keeps it that way by construction rather than by habit. Destroying a handle without calling wait_into is what an exception or an early return between post and unpack does, and it freed send_buffer/recv_buffer while MPI was still writing into them. The destructor now waits, wait() is idempotent and factored out of wait_into, and the type is [[nodiscard]] so dropping the handle at a call site is a diagnostic instead of a use-after-free. Assisted-by: ClaudeCode:claude-opus-5 --- cpp/monoprop/detail/mpi/MPICompat.h | 38 +++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/cpp/monoprop/detail/mpi/MPICompat.h b/cpp/monoprop/detail/mpi/MPICompat.h index efce1bd6..af09d76a 100644 --- a/cpp/monoprop/detail/mpi/MPICompat.h +++ b/cpp/monoprop/detail/mpi/MPICompat.h @@ -203,7 +203,8 @@ inline auto alltoall_counts(const int *send_counts, int *recv_counts, int n, Com // recv_counts is valid on return from begin_alltoallv; wait_into completes the payload transfer (a // no-op on the synchronous Shm / single-process paths) and unpacks by source. template -struct PendingAlltoallv { +struct [[nodiscard("complete the transfer with wait_into(); dropping the handle frees buffers MPI still " + "owns")]] PendingAlltoallv { int num_ranks = 0; std::vector send_counts; std::vector send_displs; @@ -215,13 +216,46 @@ struct PendingAlltoallv { MPI_Request request = MPI_REQUEST_NULL; // set only on the Kind::Mpi async path #endif - auto wait_into(std::vector> &recv_data) -> void { + // Move-only and self-completing, for the same reason mpi::Ticket is: the posted MPI_Ialltoallv writes + // directly into this handle's own send_buffer/recv_buffer, so a copy would hand two owners the same + // request (and wait on it twice), and a handle destroyed without wait_into -- what an exception or an + // early return between post and unpack does -- would free those buffers while MPI is still writing + // into them. + PendingAlltoallv() = default; + PendingAlltoallv(const PendingAlltoallv &) = delete; + auto operator=(const PendingAlltoallv &) -> PendingAlltoallv & = delete; + PendingAlltoallv(PendingAlltoallv &&other) noexcept { *this = std::move(other); } + auto operator=(PendingAlltoallv &&other) noexcept -> PendingAlltoallv & { + if (this != &other) { + wait(); // never drop a request this handle already owns + num_ranks = other.num_ranks; + send_counts = std::move(other.send_counts); + send_displs = std::move(other.send_displs); + recv_counts = std::move(other.recv_counts); + recv_displs = std::move(other.recv_displs); + send_buffer = std::move(other.send_buffer); + recv_buffer = std::move(other.recv_buffer); +#ifdef monoprop_ENABLE_MPI + request = other.request; + other.request = MPI_REQUEST_NULL; +#endif + } + return *this; + } + ~PendingAlltoallv() { wait(); } + + // Idempotent; a no-op on the synchronous Shm / single-process paths and in non-MPI builds. + auto wait() -> void { #ifdef monoprop_ENABLE_MPI if (request != MPI_REQUEST_NULL) { MPI_Wait(&request, MPI_STATUS_IGNORE); request = MPI_REQUEST_NULL; } #endif + } + + auto wait_into(std::vector> &recv_data) -> void { + wait(); recv_data.resize(static_cast(num_ranks)); for (int i = 0; i < num_ranks; ++i) { const auto lo = recv_buffer.begin() + recv_displs[static_cast(i)]; From 1e79e1b759f97fa4cc296432508e4c4bba2255bb Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Wed, 12 Aug 2026 14:04:17 +0100 Subject: [PATCH 08/17] =?UTF-8?q?test(mpi):=20=F0=9F=A7=AA=20assert=20the?= =?UTF-8?q?=20spin=20budget=20reaches=20the=20barrier,=20not=20that=20a=20?= =?UTF-8?q?parser=20parses?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit env_config_spin_budget_falls_back_to_barrier_default re-implemented `parse_positive_int(monoprop_SPIN_BUDGET_US).value_or(kDefaultSpinBudgetUs)` and asserted the result equalled itself. That passes whether or not PartitionBarrier is wired to the setting at all, which is the only thing the case exists to check. config::get() caches on first call, so the env path is genuinely unreachable from an in-process test. Make the budget injectable instead: a third constructor parameter, defaulting to the configured value and then the compiled-in one, plus a spin_budget() accessor. The case now observes what the barrier resolved, and an explicit override is shown to win -- which is also what lets the default be swept and justified by measurement rather than asserted. Assisted-by: ClaudeCode:claude-opus-5 --- cpp/monoprop/detail/mpi/PartitionBarrier.h | 22 +++++++++++++---- cpp/tests/env_config_tests.cpp | 28 +++++++++++++++------- 2 files changed, 38 insertions(+), 12 deletions(-) diff --git a/cpp/monoprop/detail/mpi/PartitionBarrier.h b/cpp/monoprop/detail/mpi/PartitionBarrier.h index 05a893b0..1decfaed 100644 --- a/cpp/monoprop/detail/mpi/PartitionBarrier.h +++ b/cpp/monoprop/detail/mpi/PartitionBarrier.h @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -48,7 +49,16 @@ class ShmCommPoisoned : public std::runtime_error { // in sonar-project.properties. Do not "simplify" them to the default ordering. class PartitionBarrier { public: - explicit PartitionBarrier(int participants, const std::vector &group_of = {}) : participants_(participants) { + // `spin_budget` overrides how long a waiter stays on-core before yielding; unset takes + // monoprop_SPIN_BUDGET_US, else kDefaultSpinBudgetUs. Injectable because config::get() caches on + // first call, so a test cannot reach the env path in-process, and because sweeping the budget is + // how its default is justified. + explicit PartitionBarrier(int participants, + const std::vector &group_of = {}, + std::optional spin_budget = std::nullopt) + : spin_budget_(spin_budget.value_or( + std::chrono::microseconds{config::get().spin_budget_us.value_or(detail::kDefaultSpinBudgetUs)})), + participants_(participants) { if (static_cast(group_of.size()) != participants || participants <= 0) { return; // flat } @@ -128,6 +138,10 @@ class PartitionBarrier { // CommProfile so a run states which barrier it ran instead of leaving it inferred from the timing. auto group_count() const -> int { return groups_; } + // The on-core spin budget actually in force, so a test can observe the resolution rather than + // re-computing it, and so a sweep can report what it swept. + auto spin_budget() const -> std::chrono::microseconds { return spin_budget_; } + // Signal that this participant is unwinding (e.g. an engine exception), releasing peers spinning in a // barrier. Idempotent. auto poison() -> void { poisoned_.store(true, std::memory_order_release); } @@ -187,9 +201,9 @@ class PartitionBarrier { std::atomic v{0}; }; - // Read once per barrier, not once per spin: monoprop_SPIN_BUDGET_US is a sweep knob, and the spin loop - // is the measured hotspot. - std::chrono::microseconds spin_budget_{config::get().spin_budget_us.value_or(detail::kDefaultSpinBudgetUs)}; + // Resolved once per barrier, not once per spin: monoprop_SPIN_BUDGET_US is a sweep knob, and the spin + // loop is the measured hotspot. + std::chrono::microseconds spin_budget_; int participants_; int groups_ = 0; // < 2 ⇒ flat: arrived_/gen_ count participants, not domains diff --git a/cpp/tests/env_config_tests.cpp b/cpp/tests/env_config_tests.cpp index 0d440379..595a8d4c 100644 --- a/cpp/tests/env_config_tests.cpp +++ b/cpp/tests/env_config_tests.cpp @@ -14,10 +14,12 @@ #include +#include #include #include "monoprop/detail/EnvConfig.h" #include "monoprop/detail/mpi/CpuRelax.h" +#include "monoprop/detail/mpi/PartitionBarrier.h" using monoprop::config::detail::parse_flag; using monoprop::config::detail::parse_positive_int; @@ -60,15 +62,25 @@ BOOST_AUTO_TEST_CASE(env_config_parse_positive_int_range) { BOOST_CHECK(parse_positive_int("1000000") == std::optional(1'000'000)); // inclusive upper bound } -// monoprop_SPIN_BUDGET_US shares parse_positive_int, so what it adds is the fallback: an unset or -// malformed value must leave the barrier on its own default rather than on a zero-length spin. -BOOST_AUTO_TEST_CASE(env_config_spin_budget_falls_back_to_barrier_default) { - BOOST_CHECK(parse_positive_int(nullptr).value_or(monoprop::mpi::detail::kDefaultSpinBudgetUs) - == monoprop::mpi::detail::kDefaultSpinBudgetUs); - BOOST_CHECK(parse_positive_int("0").value_or(monoprop::mpi::detail::kDefaultSpinBudgetUs) - == monoprop::mpi::detail::kDefaultSpinBudgetUs); - BOOST_CHECK(parse_positive_int("25").value_or(monoprop::mpi::detail::kDefaultSpinBudgetUs) == 25); +// Observe the budget the barrier actually resolved, rather than recomputing +// `parse_positive_int(...).value_or(kDefault)` here: an assertion of that shape passes even if the +// barrier is never wired to the setting at all, which is the only thing worth checking. config::get() +// caches on first call, so the env path is unreachable in-process; the injectable override is not. +BOOST_AUTO_TEST_CASE(env_config_spin_budget_reaches_the_barrier) { + using namespace std::chrono_literals; BOOST_CHECK_GT(monoprop::mpi::detail::kDefaultSpinBudgetUs, 0); + + // Default construction lands on the configured value, else the compiled-in default -- never on a + // zero-length spin, which would send every waiter straight to sched_yield. + const monoprop::mpi::PartitionBarrier configured(2); + const auto expected = std::chrono::microseconds{ + monoprop::config::get().spin_budget_us.value_or(monoprop::mpi::detail::kDefaultSpinBudgetUs)}; + BOOST_CHECK(configured.spin_budget() == expected); + BOOST_CHECK_GT(configured.spin_budget().count(), 0); + + // An explicit override wins over both, which is what lets the default be swept and justified. + const monoprop::mpi::PartitionBarrier overridden(2, {}, 25us); + BOOST_CHECK(overridden.spin_budget() == 25us); } BOOST_AUTO_TEST_CASE(env_config_settings_cached_singleton) { From ff6f141fcf64d90919d00cb2865070ddec6e9e90 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Wed, 12 Aug 2026 14:50:40 +0100 Subject: [PATCH 09/17] =?UTF-8?q?feat(mpi):=20=E2=9C=A8=20add=20monoprop?= =?UTF-8?q?=5FBARRIER=5FGROUPING=20so=20the=20two-level=20barrier=20can=20?= =?UTF-8?q?be=20measured?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two-level barrier's value has never been isolated, and until now it could not be. Its domains are derived from the cpusets, so the only way to get a flat barrier from outside the process was to turn pinning off -- which also unpins. Every before/after therefore confounded "grouped vs flat" with "pinned vs unpinned", including the ones used to justify the second level in the first place. monoprop_BARRIER_GROUPING=0 forces the flat path and leaves pinning alone, which makes "grouped vs flat, both pinned, one build, one allocation" a run rather than an argument. There is a specific question waiting on it. At ~29M terms on two nodes the placement fix made layout 8x16's energy 1.56x and gradient 1.45x faster, but `pare` 1.37x slower on the median and 1.40x on the min -- median and min agreeing, so not noise. `pare` is the shortest collective-bearing operation in the suite at ~10 ms, and the second level trades one fetch_add for two sequential hops, so it can only pay where there is contention to relieve. A short collective whose partitions arrive together has none. The knob is diagnostic and tuning, defaulting to on, so behaviour is unchanged unless it is set. If the flat barrier turns out to win broadly, this is also the measurement that justifies deleting the second level rather than assuming it earns its keep. Assisted-by: ClaudeCode:claude-opus-5 --- cpp/monoprop/detail/EnvConfig.h | 7 +++++++ cpp/monoprop/detail/mpi/PartitionBarrier.h | 12 ++++++++++-- docs/content/docs/features/parallelism.mdx | 1 + 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/cpp/monoprop/detail/EnvConfig.h b/cpp/monoprop/detail/EnvConfig.h index f686619f..11766581 100644 --- a/cpp/monoprop/detail/EnvConfig.h +++ b/cpp/monoprop/detail/EnvConfig.h @@ -27,6 +27,11 @@ // monoprop_COMM_PROFILE bool, default OFF; per-partition collective accounting to stderr → comm_profile // monoprop_SPIN_BUDGET_US positive int us, default kDefaultSpinBudgetUs; barrier on-core spin before // yielding. Exists to be swept against a real workload → spin_budget_us +// monoprop_BARRIER_GROUPING bool, default ON; 0/false forces the flat barrier while LEAVING PINNING ON +// → barrier_grouping. Without this the two-level barrier cannot be measured: +// its domains come from the cpusets, so turning pinning off to get a flat +// barrier also unpins, and every before/after confounds the two. Exists so +// "grouped vs flat, both pinned" is a run rather than an argument. namespace monoprop::config { @@ -62,6 +67,7 @@ struct Settings { bool partition_pinning = true; bool comm_profile = false; std::optional spin_budget_us; // nullopt ⇒ the barrier's own default + bool barrier_grouping = true; }; // Parse the environment once; the Settings are cached and shared across TUs. @@ -72,6 +78,7 @@ inline auto get() -> const Settings & { s.partition_pinning = detail::parse_flag(std::getenv("monoprop_PARTITION_PINNING"), true); s.comm_profile = detail::parse_flag(std::getenv("monoprop_COMM_PROFILE"), false); s.spin_budget_us = detail::parse_positive_int(std::getenv("monoprop_SPIN_BUDGET_US")); + s.barrier_grouping = detail::parse_flag(std::getenv("monoprop_BARRIER_GROUPING"), true); return s; }(); return settings; diff --git a/cpp/monoprop/detail/mpi/PartitionBarrier.h b/cpp/monoprop/detail/mpi/PartitionBarrier.h index 1decfaed..d8bf8ec2 100644 --- a/cpp/monoprop/detail/mpi/PartitionBarrier.h +++ b/cpp/monoprop/detail/mpi/PartitionBarrier.h @@ -41,7 +41,15 @@ class ShmCommPoisoned : public std::runtime_error { // the NUMA node) the barrier becomes two-level: fan in within a domain, then across domains, then release // within the domain. Both the arrival fetch_add and the release store then cost O(S/G) coherence // transactions instead of O(S), and stay inside one cache slice. No group ids (pinning off, or /sys -// unreadable), a single domain, or all-singleton domains degrade to the flat barrier. +// unreadable), a single domain, all-singleton domains, or monoprop_BARRIER_GROUPING=0 degrade to the +// flat barrier. That last one exists because the domains come from the cpusets: without it the only way +// to get a flat barrier is to unpin, so "grouped vs flat" and "pinned vs unpinned" could never be +// separated, and the second level's value could not be measured on a real workload. +// +// The second level is not a free win. It replaces one fetch_add with two sequential hops, so it pays +// only when there is contention to relieve: at high partition counts, or when arrivals are skewed. On a +// collective short enough that the barrier is most of it, and with partitions arriving together, the +// extra hop is pure latency. // // The acquire/release/relaxed orderings below are load-bearing, not an oversight: the release store to // `gen_` is what publishes the preceding relaxed reset of `arrived_`. Promoting them to seq_cst would @@ -59,7 +67,7 @@ class PartitionBarrier { : spin_budget_(spin_budget.value_or( std::chrono::microseconds{config::get().spin_budget_us.value_or(detail::kDefaultSpinBudgetUs)})), participants_(participants) { - if (static_cast(group_of.size()) != participants || participants <= 0) { + if (static_cast(group_of.size()) != participants || participants <= 0 || !config::get().barrier_grouping) { return; // flat } // Compact the domain ids to 0..G-1 in first-seen order, and count each group. diff --git a/docs/content/docs/features/parallelism.mdx b/docs/content/docs/features/parallelism.mdx index 22ea16c5..819ff813 100644 --- a/docs/content/docs/features/parallelism.mdx +++ b/docs/content/docs/features/parallelism.mdx @@ -46,6 +46,7 @@ the machine has — which is why they are measured rather than guessed. | `monoprop_PARTITION_PINNING` | `on` | `0`/`false`/`no` disables pinning each partition to a core. Has an effect only on Linux. | | `monoprop_COMM_PROFILE` | `off` | `1`/`true`/`yes` makes each transport print one `COMMPROF` line per rank to stderr when it is destroyed, accounting for where its collectives' wall time went. Diagnostic only; off costs one branch per collective phase. | | `monoprop_SPIN_BUDGET_US` | `30` | Microseconds a partition spends spinning on-core at a barrier before it starts yielding. Longer suits a pinned one-partition-per-core layout; shorter suits an oversubscribed one, where a spinner holds the core its late peer needs. A tuning knob — the default is calibrated, not arbitrary. | +| `monoprop_BARRIER_GROUPING` | `on` | `0`/`false`/`no` forces the flat barrier while leaving pinning on. The two-level barrier takes its domains from the cpusets, so disabling pinning to get a flat barrier also unpins — which makes "is the second level worth it?" unanswerable from the outside. This knob separates the two so it can be measured on a real workload. Diagnostic and tuning; the second level is a win at high partition counts and a small loss on collectives short enough for the extra hop to dominate. | ```bash # Run 8 partitions instead of one-per-core: From bc6a270ad61dc8c540b620ec421b20c5eeb3803c Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Thu, 13 Aug 2026 11:11:41 +0100 Subject: [PATCH 10/17] =?UTF-8?q?feat(partition):=20=E2=9C=A8=20report=20h?= =?UTF-8?q?ow=20many=20partitions=20actually=20pinned?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit barrier_groups = 0 has two legitimate causes that are indistinguishable from outside the process: nothing was pinned, or every partition landed in a single locality domain and so has nothing to fan in across. That ambiguity is what let the Slurm mask bug read as a tuning result rather than a defect. pin_this_thread now returns whether the affinity call took, PartitionGroup counts the masters that succeeded, and CommProfile prints it as `pinned=`. A sentinel of -1 distinguishes "no PartitionGroup owns this transport" -- a bare-transport unit test -- from a genuine zero. Assisted-by: ClaudeCode:claude-opus-5 --- cpp/monoprop/detail/mpi/CommProfile.h | 11 +++++-- cpp/monoprop/detail/mpi/HybridComm.h | 8 +++++ cpp/monoprop/detail/mpi/ShmComm.h | 8 +++++ cpp/monoprop/detail/partition/CpuTopology.h | 16 +++++++--- .../detail/partition/PartitionGroup.h | 32 +++++++++++++++++-- 5 files changed, 67 insertions(+), 8 deletions(-) diff --git a/cpp/monoprop/detail/mpi/CommProfile.h b/cpp/monoprop/detail/mpi/CommProfile.h index 1575a04a..cfd391d9 100644 --- a/cpp/monoprop/detail/mpi/CommProfile.h +++ b/cpp/monoprop/detail/mpi/CommProfile.h @@ -52,9 +52,15 @@ class CommProfile { : slots_(static_cast(n_partitions)), mpi_rank_(mpi_rank) {} - // L3 domains the transport's barrier grouped its partitions into (< 2 ⇒ the flat barrier ran). + // Locality domains the transport's barrier grouped its partitions into (< 2 ⇒ the flat barrier ran). int barrier_groups = 0; + // How many partitions actually got pinned, out of `partitions`. Reported because barrier_groups = 0 has + // two legitimate causes that are otherwise indistinguishable from outside the process: nothing was + // pinned, or every partition landed in one locality domain and so has nothing to fan in across. + // -1 means the transport was never told (no PartitionGroup owns it, e.g. a bare-transport unit test). + int pinned = -1; + auto slot(int partition) -> Slot & { return slots_[static_cast(partition)]; } // Aggregate over partitions, plus partition 0 broken out. Written to stderr at teardown, one @@ -86,12 +92,13 @@ class CommProfile { // Peer barrier wait is the cost of the master's serial phases seen from the other side. const uint64_t peer_barrier_ns = total.barrier_ns - p0.barrier_ns; std::print(stderr, - "COMMPROF rank={} partitions={} barrier_groups={} verbs={} barriers={} " + "COMMPROF rank={} partitions={} barrier_groups={} pinned={} verbs={} barriers={} " "table_p0_s={:.3f} table_par_s={:.3f} table_move_s={:.3f} mpi_s={:.3f} " "barrier_p0_s={:.3f} barrier_peers_s={:.3f} barrier_per_sync_us={:.2f}\n", mpi_rank_, slots_.size(), barrier_groups, + pinned, p0.n_verbs, p0.n_barriers, to_s(p0.table_p0_ns), diff --git a/cpp/monoprop/detail/mpi/HybridComm.h b/cpp/monoprop/detail/mpi/HybridComm.h index 8462f62f..1845b458 100644 --- a/cpp/monoprop/detail/mpi/HybridComm.h +++ b/cpp/monoprop/detail/mpi/HybridComm.h @@ -601,6 +601,14 @@ class HybridComm { auto poison() -> void { barrier_.poison(); } auto reset() -> void { barrier_.reset(); } + // Told once by the owning PartitionGroup after its masters are up; a no-op when profiling is off. + // See CommProfile::pinned for why the count is reported at all. + auto note_pinned(int n) -> void { + if (prof_) { + prof_->pinned = n; + } + } + private: struct alignas(64) Slot { const void *ptr = nullptr; diff --git a/cpp/monoprop/detail/mpi/ShmComm.h b/cpp/monoprop/detail/mpi/ShmComm.h index aa1ba595..c917ab1d 100644 --- a/cpp/monoprop/detail/mpi/ShmComm.h +++ b/cpp/monoprop/detail/mpi/ShmComm.h @@ -210,6 +210,14 @@ class ShmComm { auto reset() -> void { barrier_.reset(); } + // Told once by the owning PartitionGroup after its masters are up; a no-op when profiling is off. + // See CommProfile::pinned for why the count is reported at all. + auto note_pinned(int n) -> void { + if (prof_) { + prof_->pinned = n; + } + } + private: // One cache-line-isolated publish slot per rank. A rank writes only its own slot, and reads peers' // slots only between the two barriers of a collective. diff --git a/cpp/monoprop/detail/partition/CpuTopology.h b/cpp/monoprop/detail/partition/CpuTopology.h index 58212fcb..5b35dbfc 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.h +++ b/cpp/monoprop/detail/partition/CpuTopology.h @@ -406,9 +406,13 @@ inline auto cpuset_domains(const std::vector &sets) -> std::vector return domains; } -// A failing pthread call is ignored: only performance depends on it. -inline auto pin_this_thread(const CpuSet &set) -> void { - pthread_setaffinity_np(pthread_self(), sizeof(CpuSet), &set); +// Correctness never depends on pinning, so a failure is not an error -- but it must not be invisible. +// Returns whether the affinity actually took: `barrier_groups = 0` has two legitimate causes (nothing +// was pinned, or each rank was confined to a single locality domain and so has nothing to fan in +// across), and without this they are indistinguishable from outside the process. CommProfile reports +// the count so a run states what happened instead of leaving it inferred from the timing. +[[nodiscard]] inline auto pin_this_thread(const CpuSet &set) -> bool { + return pthread_setaffinity_np(pthread_self(), sizeof(CpuSet), &set) == 0; } #else // portable fallback: no topology, no pinning @@ -444,7 +448,11 @@ inline auto classify_node_mask(const std::vector & /*masks*/) -> NodeMas inline auto cpuset_domains(const std::vector & /*sets*/) -> std::vector { return {}; } -inline auto pin_this_thread(const CpuSet & /*set*/) -> void {} +// Always false here: there is no pinning on this platform, so reporting "0 pinned" is accurate rather +// than a failure. See the Linux overload for why the result is surfaced at all. +[[nodiscard]] inline auto pin_this_thread(const CpuSet & /*set*/) -> bool { + return false; +} #endif // __linux__ diff --git a/cpp/monoprop/detail/partition/PartitionGroup.h b/cpp/monoprop/detail/partition/PartitionGroup.h index 069ee3c3..19c90940 100644 --- a/cpp/monoprop/detail/partition/PartitionGroup.h +++ b/cpp/monoprop/detail/partition/PartitionGroup.h @@ -72,6 +72,7 @@ class PartitionGroup { stop_and_join_(); throw; } + publish_pinned_count_(); } // Fresh transport and threads over the same parent; each partition is deep-copied on its new master, then @@ -100,6 +101,7 @@ class PartitionGroup { stop_and_join_(); throw; } + publish_pinned_count_(); } auto operator=(const PartitionGroup &) -> PartitionGroup & = delete; @@ -185,6 +187,23 @@ class PartitionGroup { #endif } + // Called once the first run_on_all has returned, which is the earliest point every master has passed + // its pin attempt: masters pin before taking any job, so the count is final and needs no extra + // synchronisation of its own. Reporting it is what makes `barrier_groups = 0` readable -- unpinned and + // one-domain-per-rank are both legitimate causes of it and are otherwise indistinguishable. + auto publish_pinned_count_() -> void { + const int pinned = pinned_count_.load(std::memory_order_relaxed); +#ifdef monoprop_ENABLE_MPI + if (hyb_) { + hyb_->note_pinned(pinned); + return; + } +#endif + if (shm_) { + shm_->note_pinned(pinned); + } + } + auto make_transport_() -> void { // Empty unless the partitions are pinned and /sys was readable ⇒ flat barrier (see // PartitionBarrier); cpusets_ must already be set. @@ -240,8 +259,14 @@ class PartitionGroup { } auto master_loop_(int rank) -> void { - if (!cpusets_.empty()) { - pin_this_thread(cpusets_[static_cast(rank)]); + // Relaxed is sufficient, but not because ordering does not matter here -- it is because the + // ordering comes from somewhere else. Each master increments once before it takes any job, so + // every increment happens-before that master's job completion, and job completion is already + // published to the facade thread through m_/done_count_. publish_pinned_count_ reads the counter + // only after run_on_all has returned, i.e. after acquiring that same mutex. Do not "strengthen" + // this to acq_rel expecting it to carry the edge itself; the edge is the dispatch mutex's. + if (!cpusets_.empty() && pin_this_thread(cpusets_[static_cast(rank)])) { + pinned_count_.fetch_add(1, std::memory_order_relaxed); } unsigned seen = 0; for (;;) { @@ -285,6 +310,9 @@ class PartitionGroup { std::vector errs_; std::vector cpusets_; std::vector masters_; + // Incremented by each master that actually pinned. Relaxed throughout: written once per master before + // it takes work, read once after they are all up (see publish_pinned_count_). + std::atomic pinned_count_{0}; // Job dispatch: the facade thread publishes one job and waits for all masters to complete it. std::mutex m_; From 2e79b1920a0218755a4044ef8c1b4de33780dddc Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Thu, 13 Aug 2026 11:11:58 +0100 Subject: [PATCH 11/17] =?UTF-8?q?test(cpp):=20=F0=9F=A7=AA=20add=20an=20op?= =?UTF-8?q?t-in=20ThreadSanitizer=20build=20and=20audit=20the=20barrier=20?= =?UTF-8?q?orderings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PartitionBarrier's memory orderings were documented as load-bearing and sonar-project.properties suppresses cpp:S8417 for that file, but nothing checked the claim -- the repo had no sanitizer configuration at all. monoprop_ENABLE_TSAN is an option rather than a build type, so it composes with any CMAKE_BUILD_TYPE: the threading layer is only worth auditing at the optimization level it ships with. Flags go on CMAKE_CXX_FLAGS because -fsanitize=thread must reach the link line as well as every compile line. cpp/tests/tsan.supp suppresses third-party (OpenMPI/PMIx/libevent) reports only, never anything under cpp/monoprop/, and that was verified by confirming a deliberately weakened ordering still reports through it. partition_barrier_tests, shm_comm_tests and partition_equivalence_tests are clean under TSan. A clean run proves nothing on its own here, because the synchronisation is hand-rolled atomics with no mutex for TSan to hook, so it was checked against a mutation control: demoting any single one of the five orderings -- either generation store, the group-generation store, the arrival fetch_adds, or the spin load -- makes TSan report the published data as a race. Each is individually necessary and none is stronger than it needs to be. Assisted-by: ClaudeCode:claude-opus-5 --- AGENTS.md | 7 ++- CMakeLists.txt | 1 + CMakePresets.json | 27 +++++++++++ README.md | 4 ++ cmake/compiler_flags/CXXFlags.cmake | 7 +++ cmake/compiler_flags/Sanitizers.cmake | 54 ++++++++++++++++++++++ cpp/monoprop/detail/mpi/PartitionBarrier.h | 6 ++- cpp/tests/CMakeLists.txt | 15 ++++++ cpp/tests/tsan.supp | 35 ++++++++++++++ docs/content/docs/building.mdx | 31 +++++++++++++ docs/content/docs/testing.mdx | 6 +++ 11 files changed, 191 insertions(+), 2 deletions(-) create mode 100644 cmake/compiler_flags/Sanitizers.cmake create mode 100644 cpp/tests/tsan.supp diff --git a/AGENTS.md b/AGENTS.md index 2d1d5e0d..cfc638b9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -52,7 +52,12 @@ Key files: agree, or dispatch routes at a template the bindings never instantiated. - `CMakePresets.json`: the single source of truth for the supported C++ unit-test build/run entry points. The presets adopt the scikit-build-core trees generated by `uv sync`; regenerate the tree - with `uv sync`, then use the matching `skbuild-*` preset to build or run CTest. + with `uv sync`, then use the matching `skbuild-*` preset to build or run CTest. `skbuild-tsan` + adopts the `monoprop_ENABLE_TSAN=ON` (ThreadSanitizer) tree — see + `docs/content/docs/building.mdx`. That option is opt-in, composes with any build type, and is the + only machine check on the hand-written memory orderings in + `cpp/monoprop/detail/mpi/PartitionBarrier.h`; `cpp/tests/tsan.supp` suppresses third-party + reports only, never anything under `cpp/monoprop/`. ### Core abstractions (the propagation backbone) diff --git a/CMakeLists.txt b/CMakeLists.txt index 65a033ad..50b03507 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -98,6 +98,7 @@ message( message(STATUS " User-appended : ${EXTRA_CXXFLAGS}") message(STATUS " MPI parallelization : ${monoprop_ENABLE_MPI}") +message(STATUS " ThreadSanitizer : ${monoprop_ENABLE_TSAN}") message(STATUS " Wide term index : ${monoprop_WIDE_TERM_INDEX}") message(STATUS " Max simulable modes : ${monoprop_MAX_NUM_MODES}") message(STATUS " C++ unit tests : ${monoprop_ENABLE_CXX_UNIT_TESTS}") diff --git a/CMakePresets.json b/CMakePresets.json index 115f246e..3f7d3b0f 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -14,6 +14,13 @@ "description": "Adopt the Debug build tree generated by 'uv sync --config-settings=cmake.build-type=Debug' (the 'Install in Debug mode' task). Do NOT reconfigure this preset from CMake Tools; regenerate the tree with uv/scikit-build-core, then use this preset only to build and run the C++ CTest suite.", "binaryDir": "${sourceDir}/build/editable/Debug", "generator": "Ninja" + }, + { + "name": "skbuild-tsan", + "displayName": "scikit-build-core (ThreadSanitizer, adopt existing tree)", + "description": "Adopt the ThreadSanitizer tree generated by 'uv sync --all-extras --reinstall-package monoprop --no-cache --config-settings-package=\"monoprop:cmake.build-type=RelWithDebInfo\" --config-settings-package=\"monoprop:cmake.define.monoprop_ENABLE_TSAN=ON\"'. RelWithDebInfo, not Debug: the partition threading layer is only worth auditing at the optimization level it ships with, and -g is added by monoprop_ENABLE_TSAN regardless. Do NOT reconfigure this preset from CMake Tools; regenerate the tree with uv/scikit-build-core, then use this preset only to build and run the C++ CTest suite.", + "binaryDir": "${sourceDir}/build/editable/RelWithDebInfo", + "generator": "Ninja" } ], "buildPresets": [ @@ -36,6 +43,16 @@ "monoprop_unit_tests.x" ], "jobs": 4 + }, + { + "name": "skbuild-tsan", + "displayName": "scikit-build-core (ThreadSanitizer) - Build C++ unit tests", + "description": "Build the C++ unit test executable in the ThreadSanitizer tree. Requires the tree to already exist (see the skbuild-tsan configure preset).", + "configurePreset": "skbuild-tsan", + "targets": [ + "monoprop_unit_tests.x" + ], + "jobs": 4 } ], "testPresets": [ @@ -58,6 +75,16 @@ "outputOnFailure": true }, "inheritConfigureEnvironment": true + }, + { + "name": "skbuild-tsan", + "displayName": "scikit-build-core (ThreadSanitizer) - Run C++ unit tests", + "description": "Run the C++ CTest suite under ThreadSanitizer. cpp/tests/CMakeLists.txt attaches TSAN_OPTIONS (including cpp/tests/tsan.supp) to every discovered test, so no extra environment is needed. Expect a large slowdown; a race makes the owning test exit non-zero.", + "configurePreset": "skbuild-tsan", + "output": { + "outputOnFailure": true + }, + "inheritConfigureEnvironment": true } ] } diff --git a/README.md b/README.md index 0b52a0be..5ceaea8d 100644 --- a/README.md +++ b/README.md @@ -114,6 +114,10 @@ just test-mpi # Python + C++ tests under MPI just test-wide # Python + C++ unit tests with a 64-bit TermIndex ``` +To audit the partition threading layer for data races, build with +`cmake.define.monoprop_ENABLE_TSAN=ON` and run CTest against that tree; see the +[building guide](https://docs.monoprop.algorithmiq.tech/building). + See the [testing guide](https://docs.monoprop.algorithmiq.tech/testing) for the with/without-MPI details and the rank matrix. diff --git a/cmake/compiler_flags/CXXFlags.cmake b/cmake/compiler_flags/CXXFlags.cmake index d5ebdc10..e53202d2 100644 --- a/cmake/compiler_flags/CXXFlags.cmake +++ b/cmake/compiler_flags/CXXFlags.cmake @@ -24,9 +24,14 @@ # list, *e.g.* to override previous compiler flags without touching the CMake # scripts. Default is empty. # +# Opt-in sanitizer flags are appended to ``CMAKE_CXX_FLAGS`` by +# ``Sanitizers.cmake``; see that file for why they do not go into +# ``monoprop_CXX_FLAGS``. +# # Variables used:: # # monoprop_ENABLE_ARCH_FLAGS +# monoprop_ENABLE_TSAN # EXTRA_CXXFLAGS # # Variables modified:: @@ -78,3 +83,5 @@ set(monoprop_CXX_FLAGS "") include(${CMAKE_CURRENT_LIST_DIR}/GNU.CXX.cmake) include(${CMAKE_CURRENT_LIST_DIR}/Intel.CXX.cmake) include(${CMAKE_CURRENT_LIST_DIR}/Clang.CXX.cmake) +# Last, so the sanitizer flags cannot be clobbered by a per-compiler `set()`. +include(${CMAKE_CURRENT_LIST_DIR}/Sanitizers.cmake) diff --git a/cmake/compiler_flags/Sanitizers.cmake b/cmake/compiler_flags/Sanitizers.cmake new file mode 100644 index 00000000..ad0b1777 --- /dev/null +++ b/cmake/compiler_flags/Sanitizers.cmake @@ -0,0 +1,54 @@ +#.rst: +# +# Opt-in sanitizer configuration. +# +# Sanitizers are wired in here rather than as a build type because they are +# orthogonal to optimization level: the partition threading layer is only worth +# auditing at the optimization level it ships with, so +# ``monoprop_ENABLE_TSAN=ON`` composes with any ``CMAKE_BUILD_TYPE`` instead of +# replacing it. +# +# The flags are appended to ``CMAKE_CXX_FLAGS`` (not to ``monoprop_CXX_FLAGS``, +# which reaches ``target_compile_options`` only) because ``-fsanitize=thread`` +# must appear on the *link* line as well as every compile line; CMake passes +# ``CMAKE_CXX_FLAGS`` to both. The explicit linker-flag appends below cover +# link steps that a toolchain file may drive without ``CMAKE_CXX_FLAGS``. +# +# Variables used:: +# +# monoprop_ENABLE_TSAN +# +# Variables modified:: +# +# CMAKE_CXX_FLAGS +# CMAKE_EXE_LINKER_FLAGS +# CMAKE_SHARED_LINKER_FLAGS +# CMAKE_MODULE_LINKER_FLAGS + +option( + monoprop_ENABLE_TSAN + "Build with ThreadSanitizer (-fsanitize=thread); opt-in, never on by default" + OFF +) + +if(monoprop_ENABLE_TSAN) + if(NOT CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") + message( + FATAL_ERROR + "monoprop_ENABLE_TSAN requires GCC or Clang. Detected: ${CMAKE_CXX_COMPILER_ID}" + ) + endif() + + # -g is forced even in Release: without line tables a TSan report names only + # addresses, and a report you cannot attribute to a memory ordering is not an + # audit. -fno-omit-frame-pointer is already in monoprop_CXX_FLAGS, but repeat + # it here so the setting survives an EXTRA_CXXFLAGS override of that list. + set( + monoprop_TSAN_FLAGS + "-fsanitize=thread -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -g" + ) + string(APPEND CMAKE_CXX_FLAGS " ${monoprop_TSAN_FLAGS}") + string(APPEND CMAKE_EXE_LINKER_FLAGS " -fsanitize=thread") + string(APPEND CMAKE_SHARED_LINKER_FLAGS " -fsanitize=thread") + string(APPEND CMAKE_MODULE_LINKER_FLAGS " -fsanitize=thread") +endif() diff --git a/cpp/monoprop/detail/mpi/PartitionBarrier.h b/cpp/monoprop/detail/mpi/PartitionBarrier.h index d8bf8ec2..c0ef5054 100644 --- a/cpp/monoprop/detail/mpi/PartitionBarrier.h +++ b/cpp/monoprop/detail/mpi/PartitionBarrier.h @@ -54,7 +54,11 @@ class ShmCommPoisoned : public std::runtime_error { // The acquire/release/relaxed orderings below are load-bearing, not an oversight: the release store to // `gen_` is what publishes the preceding relaxed reset of `arrived_`. Promoting them to seq_cst would // put a full barrier in the spin loop of that same hotspot, so cpp:S8417 is suppressed for this file -// in sonar-project.properties. Do not "simplify" them to the default ordering. +// in sonar-project.properties. Do not "simplify" them to the default ordering, and do not weaken one +// either: the monoprop_ENABLE_TSAN build (see docs/content/docs/building.mdx) is the check that stands +// in for the suppressed rule, and demoting any single one of the five -- either `gen_` store, the +// `group_gen_` store, the arrival fetch_adds, or the spin load -- makes it report the published data as +// a race. Every ordering here is individually necessary, and none is stronger than it needs to be. class PartitionBarrier { public: // `spin_budget` overrides how long a waiter stays on-core before yielding; unset takes diff --git a/cpp/tests/CMakeLists.txt b/cpp/tests/CMakeLists.txt index 450a90a3..78a6a252 100644 --- a/cpp/tests/CMakeLists.txt +++ b/cpp/tests/CMakeLists.txt @@ -50,10 +50,25 @@ target_link_libraries( include(${CMAKE_CURRENT_LIST_DIR}/boost-test.cmake) +# Under monoprop_ENABLE_TSAN every test process needs the suppressions file, so it +# is attached as a test property rather than left to the caller's shell. +# history_size raises the shadow-memory depth so a report about a barrier word +# can still name the thread that wrote the previous generation; the spin loops +# retire far more accesses between two interesting ones than the default 2 keeps. +set(_monoprop_test_env) +if(monoprop_ENABLE_TSAN) + set( + _monoprop_test_env + ENVIRONMENT + "TSAN_OPTIONS=suppressions=${CMAKE_CURRENT_LIST_DIR}/tsan.supp:history_size=4:second_deadlock_stack=1" + ) +endif() + # Automatic discovery of unit tests. # Default CTest run includes per-case serial tests plus suite-level MPI variants # for monoprop_MPI_TEST_PROCS. discover_tests(monoprop_unit_tests.x PROPERTIES LABELS "unit" + ${_monoprop_test_env} ) diff --git a/cpp/tests/tsan.supp b/cpp/tests/tsan.supp new file mode 100644 index 00000000..e5653562 --- /dev/null +++ b/cpp/tests/tsan.supp @@ -0,0 +1,35 @@ +# ThreadSanitizer suppressions for the monoprop C++ unit tests +# (monoprop_ENABLE_TSAN=ON). Wired in as TSAN_OPTIONS=suppressions=... by +# cpp/tests/CMakeLists.txt, so a `ctest` run picks it up with no extra setup. +# +# Everything here is a *third-party* entry. There is deliberately no suppression +# for anything under cpp/monoprop/: the point of the TSan configuration is to +# machine-check PartitionBarrier's memory orderings, and a suppression there +# would silently retract that check. If a monoprop access is benign, say so in a +# comment at the access and, where the tool cannot see the ordering, annotate it +# — do not hide it here. + +# --------------------------------------------------------------------------- +# MPI (monoprop_ENABLE_MPI=ON builds only). +# +# OpenMPI/PMIx are not built with instrumentation, and their progress engine +# races on its own state by design (lock-free free lists, opal_progress +# callbacks, the async event thread). TSan cannot see their synchronization, so +# every report from inside them is noise. `called_from_lib` needs the shared +# object's soname, which is what OpenMPI 5.x installs. +# --------------------------------------------------------------------------- +called_from_lib:libmpi.so +called_from_lib:libopen-pal.so +called_from_lib:libopen-rte.so +called_from_lib:libpmix.so +called_from_lib:libevent_core.so +called_from_lib:libevent_pthreads.so +race:^ompi_ +race:^opal_ +race:^mca_ +race:^pmix +race:^PMIx +race:^orte_ +# The MPI_Init_thread/MPI_Finalize pair leaks a progress thread whose teardown +# races with the runtime's own atexit work; not monoprop's memory. +thread:^opal_progress_ diff --git a/docs/content/docs/building.mdx b/docs/content/docs/building.mdx index 98ae8395..a59ca1c5 100644 --- a/docs/content/docs/building.mdx +++ b/docs/content/docs/building.mdx @@ -109,6 +109,37 @@ uv sync --all-extras -v --config-settings=cmake.build-type=Debug ctest --test-dir build/editable/Debug ``` +### ThreadSanitizer tree + +`monoprop_ENABLE_TSAN=ON` builds with `-fsanitize=thread`. It is opt-in and +composes with any build type rather than replacing one, because the partition +threading layer — the pinned partition masters and the sense-reversing +`PartitionBarrier` they synchronise on — is only worth auditing at the +optimization level it ships with. `-g` is added regardless of build type, since a +report without line tables cannot be attributed to a memory ordering. + +```bash +uv sync --all-extras -v --reinstall-package monoprop --no-cache \ + --config-settings-package="monoprop:cmake.build-type=RelWithDebInfo" \ + --config-settings-package="monoprop:cmake.define.monoprop_ENABLE_TSAN=ON" +ctest --test-dir build/editable/RelWithDebInfo --output-on-failure -L serial +``` + +`cpp/tests/CMakeLists.txt` attaches `TSAN_OPTIONS` — including the +`cpp/tests/tsan.supp` suppressions — to every discovered test, so no extra +environment is needed. A reported race makes the owning test exit non-zero. +Expect a large slowdown, so prefer running the threading suites on their own: + +```bash +ctest --test-dir build/editable/RelWithDebInfo --output-on-failure \ + -R 'partition_barrier_|shm_comm_|partition_' +``` + +`tsan.supp` holds third-party entries only (the MPI runtime, whose progress +engine TSan cannot see the synchronisation of). Nothing under `cpp/monoprop/` is +suppressed: a suppression there would silently retract the check the +configuration exists to provide. + ### Related workflows - Use `just test-wide` for the 64-bit `monoprop_WIDE_TERM_INDEX` configuration. diff --git a/docs/content/docs/testing.mdx b/docs/content/docs/testing.mdx index 2485508c..ae29b17e 100644 --- a/docs/content/docs/testing.mdx +++ b/docs/content/docs/testing.mdx @@ -73,6 +73,12 @@ A 64-bit `TermIndex` build is the only configuration that compiles the just test-wide # rebuilds via uv sync with monoprop_WIDE_TERM_INDEX=ON, then runs CTest ``` +The partition threading layer synchronises with hand-written atomic memory +orderings, which only a ThreadSanitizer build checks. Build with +`monoprop_ENABLE_TSAN=ON` and run the threading suites against that tree — see +the [building guide](/building#threadsanitizer-tree) for the `uv sync` +invocation and what `cpp/tests/tsan.supp` does and does not suppress. + CTest registers every Boost case individually as a `serial` variant. When the build has MPI enabled and a launcher is found, it also registers the whole suite once per rank count in `monoprop_MPI_TEST_PROCS`, labelled `mpi` and `mpi-` — From e065a9e352d72702f72665b8141382714b6bf211 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Thu, 13 Aug 2026 11:41:08 +0100 Subject: [PATCH 12/17] =?UTF-8?q?test(mpi):=20=F0=9F=A7=AA=20cover=20the?= =?UTF-8?q?=20asymmetric=20emit=20gate=20across=20real=20ranks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No test combined more than one MPI rank with an asymmetric emit gate, so a defect in the cross-rank leader/follower exchange that only appears when the leader emits and the follower is dropped would have passed the whole suite. Both atols were nullopt in this file; exact_upper_atol_rescue.cpp and mpi_fresh_insert_equivalence.cpp pass upper_atol = 0, which rescues every truncated term and so restores the symmetry they appear to test. The only asymmetric-gate case with partitions > 1 runs over the in-process ShmComm and never crosses a rank boundary. Covers both sinks, which fail differently: build_graph resolves through GraphSink, which pre-sizes a response slot per incoming query, and propagate through ContractSink, which writes a half-rotation record. Energy is compared with near() because reduction order differs across rank counts; the term count is compared exactly, since that is where a dropped or double-counted rotation shows up. Two guards keep the case from passing vacuously, which matters more than the assertions themselves. The first version used the random_exact fixture and reported an ungated term count of 3 -- no threshold could drop anything, so it exercised nothing. On LihFixture (n=12, 866 terms) lower_atol drops 208-232 terms on the propagate path but *nothing* on build_graph, so a single combined flag would have left GraphSink covered in name only; the length cap drops 32 on both and is what actually exercises it. Each path therefore asserts, and reports, that its gate bit. Assisted-by: ClaudeCode:claude-opus-5 --- .../mpi_distributed_layer_equivalence.cpp | 131 ++++++++++++++++++ 1 file changed, 131 insertions(+) diff --git a/cpp/tests/mpi_distributed_layer_equivalence.cpp b/cpp/tests/mpi_distributed_layer_equivalence.cpp index 152eb2b7..11dde519 100644 --- a/cpp/tests/mpi_distributed_layer_equivalence.cpp +++ b/cpp/tests/mpi_distributed_layer_equivalence.cpp @@ -200,4 +200,135 @@ BOOST_AUTO_TEST_CASE(hybrid_mpi_partition_energy_and_size_equivalence) { BOOST_CHECK_EQUAL(n_serial, n_hybrid_global); } +// An emit gate that keeps one half of a matched rotation pair and drops the other is what makes the +// cross-rank leader/follower exchange asymmetric, and nothing in the suite exercised that with real +// ranks. Both atol arguments are nullopt in every case above; exact_upper_atol_rescue.cpp and +// mpi_fresh_insert_equivalence.cpp pass upper_atol = 0, which rescues every truncated term and so +// restores the symmetry they appear to test. The only asymmetric-gate case with more than one partition +// is partition_equivalence_tests.cpp's Pauli energy case, which runs over the in-process ShmComm and +// never crosses a rank boundary. A protocol defect that appears only when the leader emits and the +// follower does not would therefore pass the entire suite. +// +// graph_path selects which sink resolves the exchange: build_graph goes through GraphSink, which +// pre-sizes a response slot per incoming query, while propagate goes through ContractSink, which writes +// a half-rotation record instead. They fail differently, so both are covered. +// LihFixture (n=12), not the random_exact fixture the rest of this file uses. That one truncates +// nothing at any setting -- partition_equivalence_tests.cpp already records this -- and the first run of +// this case proved it from the other direction: the ungated term count was **3**, so no coefficient +// threshold could drop anything and the case passed while exercising none of the asymmetry it exists to +// cover. A gate that cannot bite is the same defect as no gate at all. +template +auto run_gated(const CaseData& data, + MPI_Comm comm, + std::optional lower_atol, + std::optional only_rotate_len_k, + bool graph_path) -> std::pair { + // only_rotate_len_k is a gate-application argument, not a construction one -- the constructor's + // ninth parameter is basis_change. Passing it here instead is a compile error, which is the useful + // kind of mistake to make. + MonomialPropagator sim(data.hamiltonian, + kCutoff, + data.initial_state, + std::nullopt, + comm, + lower_atol, + std::nullopt, + CutoffType::Length); + double e = 0.0; + if (graph_path) { + sim.build_graph(data.majoranas, + data.param_inds, + data.gen_coeffs, + std::nullopt, + std::nullopt, + only_rotate_len_k); + auto fn = sim.expectation_value_functional(); + e = fn(data.parameters); + } + else { + sim.propagate(data.majoranas, data.param_inds, data.gen_coeffs, data.parameters, only_rotate_len_k); + e = sim.expectation_value({}); + } + // size() is per-rank -- the facade holds only its local partitions -- so the comparable quantity is + // the sum over the propagator's own communicator. Over MPI_COMM_SELF that is the identity, which is + // what lets the serial reference and the distributed run share this helper. + return {e, mpi::allreduce_sum(sim.size(), comm)}; +} + +BOOST_AUTO_TEST_CASE(rank_count_matches_under_an_asymmetric_emit_gate) { + if (mpi::size(MPI_COMM_WORLD) < 2) { + BOOST_TEST_MESSAGE("Skipping asymmetric-gate case: requires at least 2 ranks."); + return; + } + constexpr size_t kLihModes = LihFixture::n_modes; + const LihFixture lih; + const CaseData& data = lih.data; + + // Ungated reference count. Without it this case could pass while every atol below was too small to + // drop anything -- that is, while exercising none of the asymmetry it exists to cover, which is the + // hole being closed. Checked once at the end, since one biting value is enough. + const auto [e_ref, n_ungated] = run_gated(data, MPI_COMM_SELF, std::nullopt, std::nullopt, true); + static_cast(e_ref); + bool gate_bit = false; + // Tracked separately from gate_bit. A single flag OR-ed across every combination is satisfied by the + // propagate cases alone, which would leave the GraphSink path -- where a mis-sized or uninitialised + // response slot would live -- covered in name only. lower_atol turns out not to reduce build_graph's + // term count at all, so the length cap is what has to bite there. + bool graph_gate_bit = false; + + for (const double atol : {1e-2, 1e-3, 1e-6}) { + for (const bool graph_path : {true, false}) { + const auto [e_serial, n_serial] = run_gated(data, MPI_COMM_SELF, atol, std::nullopt, graph_path); + const auto [e_world, n_world] = run_gated(data, MPI_COMM_WORLD, atol, std::nullopt, graph_path); + gate_bit = gate_bit || n_serial < n_ungated; + // Reported, not just asserted: BOOST_TEST_CONTEXT only prints on failure, so without this a + // passing run cannot show how much asymmetry it actually exercised -- and "the gate bit" is + // the one fact that decides whether this case is worth anything. + BOOST_TEST_MESSAGE("lower_atol=" << atol << (graph_path ? " build_graph" : " propagate") + << " ungated=" << n_ungated << " gated=" << n_serial + << " dropped=" << (n_ungated - n_serial)); + BOOST_TEST_CONTEXT("lower_atol=" << atol << (graph_path ? " build_graph" : " propagate") + << " n_serial=" << n_serial << " n_world=" << n_world) { + // Energy is tolerance-equal across rank counts, never bit-equal: the reduction order + // differs. The term count, by contrast, must agree exactly -- a rotation dropped or + // double-counted by the asymmetric path shows up here and essentially nowhere else. + BOOST_TEST(near(e_serial, e_world)); + BOOST_CHECK_EQUAL(n_serial, n_world); + } + } + } + + // A length cap gates on the generator's length rather than on a coefficient, so it drives the same + // protocol with a different survivor set. + for (const size_t cap : {size_t{2}, size_t{3}}) { + for (const bool graph_path : {true, false}) { + const auto [e_serial, n_serial] = run_gated(data, MPI_COMM_SELF, 1e-3, cap, graph_path); + const auto [e_world, n_world] = run_gated(data, MPI_COMM_WORLD, 1e-3, cap, graph_path); + if (graph_path) { + graph_gate_bit = graph_gate_bit || n_serial < n_ungated; + } + BOOST_TEST_MESSAGE("cap=" << cap << (graph_path ? " build_graph" : " propagate") + << " ungated=" << n_ungated << " gated=" << n_serial + << " dropped=" << (n_ungated - n_serial)); + BOOST_TEST_CONTEXT("only_rotate_len_k=" << cap << (graph_path ? " build_graph" : " propagate") + << " n_serial=" << n_serial << " n_world=" << n_world) { + BOOST_TEST(near(e_serial, e_world)); + BOOST_CHECK_EQUAL(n_serial, n_world); + } + } + } + + BOOST_CHECK_MESSAGE(gate_bit, + "no lower_atol reduced the term count below the ungated " << n_ungated + << ": the gate never bit, so this " + "case exercised no asymmetry"); + // Measured on LiH/n=12: lower_atol drops 208-232 of 866 on the propagate path and *nothing* on the + // build_graph path, while the length cap drops 32 on both. So this second check is what keeps + // GraphSink covered, and it fails independently of the one above -- if a fixture or default change + // ever makes the cap stop biting, the loss of coverage is reported rather than silent. + BOOST_CHECK_MESSAGE(graph_gate_bit, + "no gate reduced build_graph's term count below the ungated " + << n_ungated << ": the GraphSink path was not exercised asymmetrically"); +} + } // namespace From d0c7e6919c1dab9b2222f803047499d2d0883af1 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Thu, 13 Aug 2026 13:04:38 +0100 Subject: [PATCH 13/17] =?UTF-8?q?chore:=20=F0=9F=99=88=20ignore=20the=20lo?= =?UTF-8?q?cal=20hpc/=20job-script=20tree?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pattern containing a slash is anchored to the .gitignore's own directory, so the previous `*/hpc/*` required a leading path component and never matched the root-level hpc/ it was written for. Assisted-by: ClaudeCode:claude-opus-5 --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index e830d9ef..639bd723 100644 --- a/.gitignore +++ b/.gitignore @@ -176,3 +176,5 @@ benches/results/** # devcontainer files .devcontainer/devcontainer-lock.json +# local HPC job scripts, tools and measurement logs (see hpc/deucalion/README.md) +/hpc/ From 4b513c3ba549c64912c733b3fcb84ee849c9c596 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Thu, 13 Aug 2026 13:09:35 +0100 Subject: [PATCH 14/17] =?UTF-8?q?chore(bench):=20=F0=9F=94=A7=20stop=20pyt?= =?UTF-8?q?est=20capture=20from=20swallowing=20the=20C++=20profile=20lines?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pytest's default capture is fd-level: it replaces fd 2 for each test and discards the buffer when the test passes. monoprop_COMM_PROFILE=1 writes its COMMPROF line straight to fd 2 from a transport destructor, so a passing `just bench` reported no profile at all. Measured on one tree with one command, 2 partitions, tiny sizes: 0 COMMPROF lines without `-s`, 6 with it, both runs 10 passed. The failure mode is what makes this worth a commit rather than a note: an empty profile and an unchanged profile are the same observation, so it reads as "the change made no difference" instead of "nothing was measured". Assisted-by: ClaudeCode:claude-opus-5 --- justfile | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/justfile b/justfile index 0120d0e8..87fd2389 100644 --- a/justfile +++ b/justfile @@ -124,12 +124,19 @@ docs-install: # just bench serial # monoprop_NUM_THREADS=10 just bench serial-t10 --num-modes 64 --bench-rounds 10 +# The `-s` below is load-bearing, not cosmetic. pytest's default capture is +# fd-level: it replaces fd 2 for each test and discards the buffer when the test +# passes. monoprop_COMM_PROFILE=1 writes its COMMPROF line straight to fd 2 from a +# transport destructor, so without `-s` a passing benchmark reports no profile at +# all -- and "the two runs cost the same" is then indistinguishable from "the +# instrument never fired". + # Run the suite (timing + memory) for one LABEL; extra args go to pytest. bench LABEL *ARGS: @mkdir -p "{{ bench_results }}" label="$1"; shift; \ monoprop_BENCH_LABEL="$label" monoprop_BENCH_RESULTS="{{ bench_results }}" \ - uv run --no-sync python -m pytest benches -o filterwarnings=default \ + uv run --no-sync python -m pytest benches -o filterwarnings=default -s \ --benchmark-json="{{ bench_results }}/time-$label.json" "$@" uv run --no-sync python benches/report.py "{{ bench_results }}" @@ -146,7 +153,7 @@ bench-mpi LABEL RANKS *MPIARGS: monoprop_BENCH_LABEL="$label" monoprop_BENCH_RESULTS="{{ bench_results }}" \ uv run --no-sync mpiexec -n "$ranks" \ -x monoprop_BENCH_LABEL -x monoprop_BENCH_RESULTS "$@" \ - python -m pytest benches -o filterwarnings=default \ + python -m pytest benches -o filterwarnings=default -s \ --benchmark-json="{{ bench_results }}/time-$label.json" uv run --no-sync python benches/report.py "{{ bench_results }}" @@ -162,7 +169,7 @@ bench-build-mpi: bench-smoke: @mkdir -p "{{ bench_results }}" monoprop_BENCH_LABEL=smoke monoprop_BENCH_RESULTS="{{ bench_results }}" \ - uv run --no-sync python -m pytest benches -o filterwarnings=default \ + uv run --no-sync python -m pytest benches -o filterwarnings=default -s \ --benchmark-json="{{ bench_results }}/time-smoke.json" \ -m "not slow" --num-generators 8 --num-modes 8 --cutoff 6 --obs-terms 16 uv run --no-sync python benches/report.py "{{ bench_results }}" From 946c714285a87f5764d972ea28d525cb9877ba38 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Thu, 13 Aug 2026 13:12:20 +0100 Subject: [PATCH 15/17] =?UTF-8?q?docs:=20=F0=9F=93=9D=20record=20the=20two?= =?UTF-8?q?=20traps=20that=20silently=20void=20a=20measurement?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both fail by producing a plausible result rather than an error, which is what makes them worth writing down: - pytest's fd-level capture discards the engine's fd-2 diagnostics on a passing test, so monoprop_COMM_PROFILE reports nothing and the run reads as "no difference between the arms" rather than "nothing was measured". - uv sync does not relink the C++ test binary, and the editable tree cannot be reconfigured in place afterwards, so an edited test can be judged by a stale binary whose only symptom is a filter that matches nothing. AGENTS.md also gains the rule that generalises the first one: assert the count of what an instrument should emit, and never diagnose by comparing two zeros. Assisted-by: ClaudeCode:claude-opus-5 --- AGENTS.md | 18 +++++++++++++ README.md | 4 +++ docs/content/docs/building.mdx | 30 ++++++++++++++++++++++ docs/content/docs/features/parallelism.mdx | 2 +- docs/content/docs/testing.mdx | 8 ++++++ 5 files changed, 61 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index cfc638b9..06af310f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -111,6 +111,14 @@ mp = MajoranaPropagator(operator, initial_state, cutoff=4) - Fixture msgpack schema is documented in `tests/data/README.md` - Tests validate against exact solutions for small systems - Heavy use of `@parametrize_with_cases` decorators +- **pytest's default capture is fd-level**, so it hides C++ diagnostics: it replaces fd 2 for each + test and discards the buffer when the test passes, while the engine writes straight to fd 2 (the + `COMMPROF` line from `monoprop_COMM_PROFILE` comes out of a transport destructor). Reading those + under pytest requires `-s`; the `just bench` recipes pass it for this reason. +- **Assert the count of what an instrument should emit, at the point of collection.** "The two arms + measured the same" and "the instrument never fired" are otherwise the same observation, and that + reads as a result rather than a failure. For the same reason, never diagnose by comparing two + zeros — get a positive control that is known to emit before concluding anything from silence. ## Key Dependencies & Integration @@ -149,5 +157,15 @@ When changing behavior, APIs, build/test workflows, paths, or developer conventi - Check `build/*/compile_commands.json` for compilation flags - Use `rm -rf build` to clear environment-specific builds - Verify `monoprop_MAX_NUM_MODES` matches your use case (default: 250) +- **`uv sync` does not relink the C++ test binary.** It builds in a temporary directory and installs + only the wheel, and `bin/monoprop_unit_tests.x` is not a wheel target — so an edited test can leave + a stale binary that passes, or that reports `no test cases matching filter` for a case you just + wrote. Compare the binary's mtime against the source's before trusting either outcome. +- **The editable tree cannot be reconfigured in place**: its cache pins the build-isolation + interpreter scikit-build-core created and deleted, so `cmake --build build/editable/` fails + regenerating `build.ninja`, and the `skbuild-*` presets inherit that because they only adopt the + tree. To iterate on C++ tests, configure a standalone tree with an explicit `-Dnanobind_DIR=…` + (nanobind is a build-isolation-only dependency, absent from `.venv`) — recipe in + `docs/content/docs/building.mdx`. This is a sophisticated scientific computing project requiring careful attention to template instantiation, build system configuration, and the C++/Python boundary. diff --git a/README.md b/README.md index 5ceaea8d..0700ca76 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,10 @@ uv sync --all-extras -v ctest --test-dir build/editable/Release ``` +Note that `uv sync` builds in a temporary directory and installs only the wheel, so +it does **not** relink the test binary: after editing a C++ test, compare the +binary's mtime against the source's rather than trusting the result. + Full instructions — prerequisites, MPI options, and running the example executable — are in the [building guide](https://docs.monoprop.algorithmiq.tech/building). diff --git a/docs/content/docs/building.mdx b/docs/content/docs/building.mdx index a59ca1c5..afb67af6 100644 --- a/docs/content/docs/building.mdx +++ b/docs/content/docs/building.mdx @@ -140,6 +140,36 @@ engine TSan cannot see the synchronisation of). Nothing under `cpp/monoprop/` is suppressed: a suppression there would silently retract the check the configuration exists to provide. +### Iterating on a C++ test + +`uv sync` builds in a temporary directory and installs only the wheel, and the +test binary is not a wheel target — so editing a test and re-syncing leaves +`bin/monoprop_unit_tests.x` as it was. A stale binary does not announce itself: it +passes, or it reports `no test cases matching filter` for a case you just wrote. +Compare the binary's mtime against the source's before trusting either outcome. + +`cmake --build build/editable/` cannot repair that tree, because its cache +pins the build-isolation interpreter that scikit-build-core created and deleted, so +regenerating `build.ninja` fails. The `skbuild-*` presets adopt an existing tree +and set no cache variables of their own, so they inherit the same failure. To +iterate without a full `uv sync` per edit, configure a standalone tree instead — +nanobind is a build-isolation-only dependency and is absent from `.venv`, so it has +to be supplied by hand: + +```bash +uv pip install --python .venv/bin/python --target /tmp/nbdeps nanobind +cmake -S . -B /tmp/mp-tests -G Ninja -DCMAKE_BUILD_TYPE=Release -DSKBUILD=2 \ + -DSKBUILD_PROJECT_VERSION_FULL=0.0.0 -DSKBUILD_SABI_COMPONENT= \ + -Dnanobind_DIR=/tmp/nbdeps/nanobind/cmake \ + -DPython_EXECUTABLE="$PWD/.venv/bin/python" \ + -Dmonoprop_MAX_NUM_MODES=32 # narrow the templates; much faster to build +cmake --build /tmp/mp-tests +ctest --test-dir /tmp/mp-tests --output-on-failure -L serial +``` + +This leaves the editable install alone, so the Python side keeps working while you +rebuild the tests. Add `-Dmonoprop_ENABLE_MPI=ON` to get the `mpi`-labelled cases. + ### Related workflows - Use `just test-wide` for the 64-bit `monoprop_WIDE_TERM_INDEX` configuration. diff --git a/docs/content/docs/features/parallelism.mdx b/docs/content/docs/features/parallelism.mdx index 819ff813..06dd77d5 100644 --- a/docs/content/docs/features/parallelism.mdx +++ b/docs/content/docs/features/parallelism.mdx @@ -44,7 +44,7 @@ the machine has — which is why they are measured rather than guessed. | `monoprop_NUM_THREADS` | one partition per physical core | Caps the number of partitions. Set it to run fewer partitions than cores. | | `monoprop_PARTITIONS` | `auto` | `auto` = one partition per core (capped by `monoprop_NUM_THREADS`); an integer `N` = exactly `N` partitions; `off` = one partition holding the whole operator. | | `monoprop_PARTITION_PINNING` | `on` | `0`/`false`/`no` disables pinning each partition to a core. Has an effect only on Linux. | -| `monoprop_COMM_PROFILE` | `off` | `1`/`true`/`yes` makes each transport print one `COMMPROF` line per rank to stderr when it is destroyed, accounting for where its collectives' wall time went. Diagnostic only; off costs one branch per collective phase. | +| `monoprop_COMM_PROFILE` | `off` | `1`/`true`/`yes` makes each transport print one `COMMPROF` line per rank to stderr when it is destroyed, accounting for where its collectives' wall time went. The line is written to file descriptor 2 from the destructor, so under `pytest` it needs `-s` to survive the default fd-level capture, and the counters accumulate over the whole process lifetime — compare them against the sum of the operations that ran, not against one. Diagnostic only; off costs one branch per collective phase. | | `monoprop_SPIN_BUDGET_US` | `30` | Microseconds a partition spends spinning on-core at a barrier before it starts yielding. Longer suits a pinned one-partition-per-core layout; shorter suits an oversubscribed one, where a spinner holds the core its late peer needs. A tuning knob — the default is calibrated, not arbitrary. | | `monoprop_BARRIER_GROUPING` | `on` | `0`/`false`/`no` forces the flat barrier while leaving pinning on. The two-level barrier takes its domains from the cpusets, so disabling pinning to get a flat barrier also unpins — which makes "is the second level worth it?" unanswerable from the outside. This knob separates the two so it can be measured on a real workload. Diagnostic and tuning; the second level is a win at high partition counts and a small loss on collectives short enough for the extra hop to dominate. | diff --git a/docs/content/docs/testing.mdx b/docs/content/docs/testing.mdx index ae29b17e..1196fa35 100644 --- a/docs/content/docs/testing.mdx +++ b/docs/content/docs/testing.mdx @@ -37,6 +37,14 @@ SKBUILD_CMAKE_ARGS="-Dmonoprop_ENABLE_MPI=ON" \ mpiexec --allow-run-as-root -n 2 uv run --no-sync python -m pytest tests --with-mpi ``` +The C++ engine writes its diagnostics straight to file descriptor 2 — the +`COMMPROF` line from `monoprop_COMM_PROFILE` comes out of a transport destructor, +see [Parallelism and distribution](/features/parallelism). pytest's default +capture is fd-level: it replaces fd 2 for each test and discards the buffer when +the test passes, so under pytest those lines need `-s` to survive. Count the ones +you expect rather than reading the log for changes — an instrument that never +fired and an instrument reporting no change look exactly alike. + ### C++ unit tests The C++ tests run through CTest. The build tree is produced by `uv sync` (via From d8755f92f04247fc5774751f1ab6975077d1751a Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Thu, 13 Aug 2026 15:38:32 +0100 Subject: [PATCH 16/17] =?UTF-8?q?test(cpp):=20=E2=9A=A1=20skip=20fabric=20?= =?UTF-8?q?init=20in=20the=20single-process=20test=20variants?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CTest runs every Boost case as its own process, so an MPI build pays a full MPI_Init per case, and MPI_Init initialises every fabric device present even though a single-process test never sends a message: 8.8 s per process against 0.2 s of user CPU on a login node with 8 HCAs, i.e. 34 minutes for 224 cases. Excluding the fabric components takes each process to 1.9 s and the suite to 6.8 minutes, 224/224 passing. Scoped to the per-case `serial` variants through a new SERIAL_ENVIRONMENT argument on discover_tests, and deliberately kept off the multi-rank ones: a per-case launch has world size 1, so no transport is used and the fabric can only cost startup time, whereas OMPI_MCA_pml=^ucx makes a 2-rank run of zero_cutoff_upper_atol_zero_is_exact_World hang indefinitely where it otherwise passes in 29 ms. That hang reproduces on the pre-branch commit, so it is component selection rather than engine code. Assisted-by: ClaudeCode:claude-opus-5 --- AGENTS.md | 15 ++++++++ README.md | 6 ++++ cpp/tests/CMakeLists.txt | 68 ++++++++++++++++++++++++++++++++--- cpp/tests/boost-test.cmake | 7 +++- cpp/tests/boostAddTests.cmake | 3 ++ cspell.json | 9 +++++ docs/content/docs/testing.mdx | 20 +++++++++++ 7 files changed, 122 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 06af310f..42d06a72 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -119,6 +119,21 @@ mp = MajoranaPropagator(operator, initial_state, cutoff=4) measured the same" and "the instrument never fired" are otherwise the same observation, and that reads as a result rather than a failure. For the same reason, never diagnose by comparing two zeros — get a positive control that is known to emit before concluding anything from silence. +- **A slow CTest run on an MPI build is `MPI_Init`, not slow tests.** CTest runs each Boost case as + its own process, so each pays a full `MPI_Init`, which initialises every fabric device present + whether or not the process will send a message — 8.8 s against 0.2 s of user CPU on a login node + with 8 HCAs. The tell is wall time with no CPU behind it. `monoprop_ENABLE_MPI` builds default to + `monoprop_TEST_EXCLUDE_MPI_FABRIC=ON`, which skips fabric init for the per-case tests (8.8 s → 1.9 s; + suite 34 min → 5.4). +- **That exclusion is scoped to the `serial` variants and must stay that way.** A per-case launch is + one process — world size 1, `*_World` cases skip themselves, everything else is `MPI_COMM_SELF` — so + no transport is used and the fabric can only cost startup time. The multi-rank variants exchange + real messages: `OMPI_MCA_pml=^ucx` makes a 2-rank run of the suite hang indefinitely on a case that + otherwise passes in 29 ms, on `main` as well, so it is component selection rather than engine code. + `discover_tests`' `SERIAL_ENVIRONMENT` argument exists for exactly this split. +- Use exclusions (`^…`), never a positive component list — naming a component that must exist breaks + on the next machine, since `vader` became `sm` in Open MPI 5 and `OMPI_MCA_btl=self,vader` there + silently reduces to `self` alone. ## Key Dependencies & Integration diff --git a/README.md b/README.md index 0700ca76..b8acc5e6 100644 --- a/README.md +++ b/README.md @@ -118,6 +118,12 @@ just test-mpi # Python + C++ tests under MPI just test-wide # Python + C++ unit tests with a 64-bit TermIndex ``` +On an MPI build, CTest runs each case as its own process and so pays a full +`MPI_Init` per case — which initialises every fabric device present even though a +single-process test never sends a message. `monoprop_TEST_EXCLUDE_MPI_FABRIC` (on by +default) skips that for the per-case tests only, leaving the multi-rank variants on the +full component set; see the [testing guide](https://docs.monoprop.algorithmiq.tech/testing). + To audit the partition threading layer for data races, build with `cmake.define.monoprop_ENABLE_TSAN=ON` and run CTest against that tree; see the [building guide](https://docs.monoprop.algorithmiq.tech/building). diff --git a/cpp/tests/CMakeLists.txt b/cpp/tests/CMakeLists.txt index 78a6a252..52c4d43b 100644 --- a/cpp/tests/CMakeLists.txt +++ b/cpp/tests/CMakeLists.txt @@ -55,20 +55,78 @@ include(${CMAKE_CURRENT_LIST_DIR}/boost-test.cmake) # history_size raises the shadow-memory depth so a report about a barrier word # can still name the thread that wrote the previous generation; the spin loops # retire far more accesses between two interesting ones than the default 2 keeps. -set(_monoprop_test_env) +set(_monoprop_test_env_entries) if(monoprop_ENABLE_TSAN) + list( + APPEND _monoprop_test_env_entries + "TSAN_OPTIONS=suppressions=${CMAKE_CURRENT_LIST_DIR}/tsan.supp:history_size=4:second_deadlock_stack=1" + ) +endif() + +# CTest runs every Boost case as its own process, so each one pays a full MPI_Init -- +# and MPI_Init initialises every fabric device it finds whether or not the process will +# ever send a message. Measured on a Deucalion login node (8 Mellanox HCAs): 8.8 s per +# process, against 0.2 s of user CPU, so 224 cases spend over half an hour opening +# adapters for single-process tests. Excluding the fabric drops that to 1.9 s. +# +# A per-case launch is ONE process: its world size is 1, the `*_World` cases skip +# themselves, and every remaining communicator is MPI_COMM_SELF. So the fabric cannot +# affect the outcome, only the startup cost -- which is why this is scoped to the +# `serial` variants through SERIAL_ENVIRONMENT and must never reach the MPI ones. On +# this node `OMPI_MCA_pml=^ucx` makes a 2-rank run of +# zero_cutoff_upper_atol_zero_is_exact_World hang indefinitely where it otherwise +# passes in 29 ms; the same hang reproduces on the pre-branch commit, so it belongs to +# the component selection, not to the engine. +# +# Exclusions (`^`), never a positive component list -- naming a component that must +# exist is how this breaks on the next machine: `vader` became `sm` in Open MPI 5, and +# `OMPI_MCA_btl=self,vader` there silently reduces to `self` alone. Unknown OMPI_MCA_* +# variables are ignored by other MPIs, so this is inert under MPICH. +option( + monoprop_TEST_EXCLUDE_MPI_FABRIC + "Skip fabric init in the single-process (serial) test variants to cut MPI_Init cost" + ON +) +set(_monoprop_serial_env_entries) +if(monoprop_TEST_EXCLUDE_MPI_FABRIC AND monoprop_ENABLE_MPI) + list( + APPEND _monoprop_serial_env_entries + "OMPI_MCA_pml=^ucx" + "OMPI_MCA_btl=^openib,ofi,uct" + ) +endif() + +# ENVIRONMENT takes ONE value holding every VAR=value, so the separators must be +# ESCAPED semicolons. A plain "${list}" does not work: a CMake list *is* a +# semicolon-joined string, so it flattens straight back out and every entry after the +# first is read as the name of the next property -- verified by inspecting +# `ctest --show-only=json-v1`, where the second entry vanished from ENVIRONMENT and +# reappeared as a stray property. Keep the escape, and re-check that output if this +# block is ever edited. +set(_monoprop_test_env) +if(_monoprop_test_env_entries) + string( + REPLACE ";" + "\;" + _monoprop_test_env_joined + "${_monoprop_test_env_entries}" + ) set( _monoprop_test_env ENVIRONMENT - "TSAN_OPTIONS=suppressions=${CMAKE_CURRENT_LIST_DIR}/tsan.supp:history_size=4:second_deadlock_stack=1" + "${_monoprop_test_env_joined}" ) endif() # Automatic discovery of unit tests. # Default CTest run includes per-case serial tests plus suite-level MPI variants # for monoprop_MPI_TEST_PROCS. -discover_tests(monoprop_unit_tests.x +discover_tests( + monoprop_unit_tests.x PROPERTIES - LABELS "unit" - ${_monoprop_test_env} + LABELS + "unit" + ${_monoprop_test_env} + SERIAL_ENVIRONMENT + ${_monoprop_serial_env_entries} ) diff --git a/cpp/tests/boost-test.cmake b/cpp/tests/boost-test.cmake index 4f686b2b..1b54d3fe 100644 --- a/cpp/tests/boost-test.cmake +++ b/cpp/tests/boost-test.cmake @@ -21,12 +21,16 @@ if(NOT _monoprop_mpiexec_numproc_flag) set(_monoprop_mpiexec_numproc_flag "-n") endif() +# SERIAL_ENVIRONMENT holds VAR=value entries applied to the per-case `serial` variants ONLY, +# never to the MPI ones. The two groups differ in a way that matters for MPI: a per-case launch is +# one process, so its world size is 1 and no inter-process transport is ever used, whereas the MPI +# variants exchange real messages and need every component the fabric offers. function(discover_tests TARGET) cmake_parse_arguments( "" "" "WORKING_DIRECTORY" - "EXTRA_ARGS;PROPERTIES" + "EXTRA_ARGS;PROPERTIES;SERIAL_ENVIRONMENT" ${ARGN} ) @@ -66,6 +70,7 @@ function(discover_tests TARGET) "TEST_EXECUTABLE=$" -D "TEST_WORKING_DIR=${_WORKING_DIRECTORY}" -D "TEST_EXTRA_ARGS=${_EXTRA_ARGS}" -D "TEST_PROPERTIES=${_PROPERTIES}" -D + "TEST_SERIAL_ENVIRONMENT=${_SERIAL_ENVIRONMENT}" -D "TEST_LIST=${_TEST_LIST}" -D "CTEST_FILE=${ctest_tests_file}" -D "TEST_ENABLE_MPI_VARIANTS=${_enable_mpi_variants}" -D "TEST_MPI_NUMPROCS=${monoprop_MPI_TEST_PROCS}" -D diff --git a/cpp/tests/boostAddTests.cmake b/cpp/tests/boostAddTests.cmake index f0fd71b2..74b5ff00 100644 --- a/cpp/tests/boostAddTests.cmake +++ b/cpp/tests/boostAddTests.cmake @@ -39,6 +39,7 @@ endif() set(extra_args ${TEST_EXTRA_ARGS}) set(properties ${TEST_PROPERTIES}) +set(serial_env ${TEST_SERIAL_ENVIRONMENT}) set(script) set(tests) @@ -205,6 +206,8 @@ foreach(LINE ${LINES}) ${extra_args} LABELS serial + ENVIRONMENT + ${serial_env} ) endif() endforeach() diff --git a/cspell.json b/cspell.json index 4f86409b..b3778d13 100644 --- a/cspell.json +++ b/cspell.json @@ -17,6 +17,7 @@ "backend", "barriered", "bitstring", + "btl", "coeffs", "conj", "ctest", @@ -39,20 +40,28 @@ "Fock", "Hamiltonian", "Hartree", + "HCAs", "majorana", "Majorana", "Majoranas", "majoranic", + "Mellanox", "memray", "microbenchmarks", "mpiexec", + "ofi", + "openib", "oversubscribe", "Pauli", "Paulis", + "pml", "qubit", "qubits", "Remigio", "tracemalloc", + "uct", + "ucx", + "vader", "schrodinger", "simulable", "unnormalized", diff --git a/docs/content/docs/testing.mdx b/docs/content/docs/testing.mdx index 1196fa35..4bc8dac8 100644 --- a/docs/content/docs/testing.mdx +++ b/docs/content/docs/testing.mdx @@ -87,6 +87,26 @@ orderings, which only a ThreadSanitizer build checks. Build with the [building guide](/building#threadsanitizer-tree) for the `uv sync` invocation and what `cpp/tests/tsan.supp` does and does not suppress. +Because CTest runs each case as its own process (see below), an MPI-enabled build pays a +full `MPI_Init` per case — and `MPI_Init` initialises every fabric device it finds whether +or not the process will ever send a message. On a login node with eight host adapters that +is 8.8 s per process against 0.2 s of user CPU: it looks like slow tests, but it is wall +time with no CPU behind it. So MPI builds default to +`monoprop_TEST_EXCLUDE_MPI_FABRIC=ON`, which skips fabric init for the per-case tests +(8.8 s to 1.9 s each, and this suite from 34 minutes to 5.4). + +The exclusion applies **only** to the per-case `serial` variants, which are single-process: +their world size is 1, the `*_World` cases skip themselves, and every other communicator is +`MPI_COMM_SELF`, so no transport is used and the fabric can only cost startup time. The +multi-rank variants keep the full component set, because they exchange real messages. Set +the option to `OFF` to put the per-case tests back on the fabric too: + +```bash +uv sync --all-extras -v --reinstall-package monoprop --no-cache \ + --config-settings-package="monoprop:cmake.define.monoprop_ENABLE_MPI=ON" \ + --config-settings-package="monoprop:cmake.define.monoprop_TEST_EXCLUDE_MPI_FABRIC=OFF" +``` + CTest registers every Boost case individually as a `serial` variant. When the build has MPI enabled and a launcher is found, it also registers the whole suite once per rank count in `monoprop_MPI_TEST_PROCS`, labelled `mpi` and `mpi-` — From fa65466c19471ff48253223f95a089e5e2df3442 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Thu, 13 Aug 2026 17:20:00 +0100 Subject: [PATCH 17/17] =?UTF-8?q?style(tests):=20=F0=9F=8E=A8=20reflow=20t?= =?UTF-8?q?wo=20test=20messages=20for=20clang-format=2021?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit prek pins mirrors-clang-format at v21.1.0, which makes different line-break choices than v20 in these two BOOST_TEST_MESSAGE / BOOST_CHECK_MESSAGE chains. Whitespace only; no behaviour change. Caught by the first CI run this branch has ever had -- the conflict with main had been suppressing every pull_request workflow, so the lint failure predates the merge rather than coming from it. Assisted-by: ClaudeCode:claude-opus-5 --- cpp/tests/mpi_distributed_layer_equivalence.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cpp/tests/mpi_distributed_layer_equivalence.cpp b/cpp/tests/mpi_distributed_layer_equivalence.cpp index 11dde519..05a86e72 100644 --- a/cpp/tests/mpi_distributed_layer_equivalence.cpp +++ b/cpp/tests/mpi_distributed_layer_equivalence.cpp @@ -307,9 +307,8 @@ BOOST_AUTO_TEST_CASE(rank_count_matches_under_an_asymmetric_emit_gate) { if (graph_path) { graph_gate_bit = graph_gate_bit || n_serial < n_ungated; } - BOOST_TEST_MESSAGE("cap=" << cap << (graph_path ? " build_graph" : " propagate") - << " ungated=" << n_ungated << " gated=" << n_serial - << " dropped=" << (n_ungated - n_serial)); + BOOST_TEST_MESSAGE("cap=" << cap << (graph_path ? " build_graph" : " propagate") << " ungated=" << n_ungated + << " gated=" << n_serial << " dropped=" << (n_ungated - n_serial)); BOOST_TEST_CONTEXT("only_rotate_len_k=" << cap << (graph_path ? " build_graph" : " propagate") << " n_serial=" << n_serial << " n_world=" << n_world) { BOOST_TEST(near(e_serial, e_world)); @@ -320,8 +319,8 @@ BOOST_AUTO_TEST_CASE(rank_count_matches_under_an_asymmetric_emit_gate) { BOOST_CHECK_MESSAGE(gate_bit, "no lower_atol reduced the term count below the ungated " << n_ungated - << ": the gate never bit, so this " - "case exercised no asymmetry"); + << ": the gate never bit, so this " + "case exercised no asymmetry"); // Measured on LiH/n=12: lower_atol drops 208-232 of 866 on the propagate path and *nothing* on the // build_graph path, while the length cap drops 32 on both. So this second check is what keeps // GraphSink covered, and it fails independently of the one above -- if a fixture or default change