From bf618147aedf00bb687e89098f326ae4afbf838a Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Fri, 21 Aug 2026 11:08:32 +0100 Subject: [PATCH 1/4] feat(partition): :chart_with_upwards_trend: report the launcher's placement, and delete the pinning knob --cpu-bind=none and --cpu-bind=cores give the same thread count and the same partition count, and differ only in the mask the kernel enforces -- a difference that has already cost this project a 1.45x tax it could not see. A rank holding 16 of a host's 128 CPUs is equally "Slurm gave me my own 16" and "eight of us share these 16", and sampling /proc cannot tell them apart: it describes the process it runs in, never a peer rank on the same host. PartitionGroup already exchanges the affinity masks to choose a placement, so monoprop_COMMPLACE reports what that exchange saw -- one line per rank on stderr, masks=private vs masks=shared being the distinction the whole thing exists for. Default off, and nothing below the flag is computed when it is off. The flag gates PRINTING only. The mask exchange is deliberately not gated on it: the environment is per-rank, so a predicate over it is not rank-uniform, and one rank skipping a collective its peers entered is a hang, not a missing diagnostic. emit_place_line takes the flag rather than reading it, so a test binary launched without it set can still reach the emitting path. monoprop_PARTITION_PINNING is deleted and pinning is unconditional. Its parser matched only the first CHARACTER against {0,f,F,n,N}; `o` is not in that set, so `off`, `OFF` and `disabled` all parsed as ON. No campaign ran on the wrong arm -- no harness script ever set it. parse_env_flag compares whole words, case-insensitively, against 0|false|no|off, which is the one thing the bug was about; bare `f` and `n` change from falsey to truthy as a consequence, and a test pins that. The knob can be deleted rather than fixed because letting the launcher own placement was refuted by its own falsifier: propagate[hubbard] at layout A/N=1 measured 2.90x slower, 10 of 10. Mutation-verified: restoring first-character parsing fails the whole-word case. --- cpp/monoprop/detail/EnvConfig.h | 30 ++- cpp/monoprop/detail/partition/CMakeLists.txt | 1 + cpp/monoprop/detail/partition/CpuTopology.cpp | 77 ++++++- cpp/monoprop/detail/partition/CpuTopology.h | 29 ++- .../detail/partition/PartitionGroup.h | 59 +++++ .../detail/partition/PlacementReport.h | 84 ++++++++ cpp/tests/cpu_topology_tests.cpp | 201 +++++++++++++++--- cpp/tests/env_config_tests.cpp | 37 ++-- docs/content/docs/features/parallelism.mdx | 5 +- 9 files changed, 464 insertions(+), 59 deletions(-) create mode 100644 cpp/monoprop/detail/partition/PlacementReport.h diff --git a/cpp/monoprop/detail/EnvConfig.h b/cpp/monoprop/detail/EnvConfig.h index 5fae134a..39e31cf2 100644 --- a/cpp/monoprop/detail/EnvConfig.h +++ b/cpp/monoprop/detail/EnvConfig.h @@ -22,19 +22,35 @@ // pulled into hot-path headers. // // 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_COMMPLACE bool, default OFF; one COMMPLACE line per rank on stderr → commplace // monoprop_PARTITIONS int N | "auto" | "off"; parsed where it is used (resolve_partition_count_) +// +// monoprop_PARTITION_PINNING is deleted and pinning is unconditional. Its parser matched only the +// first CHARACTER, so `off`, `OFF` and `disabled` all parsed as ON; parse_env_flag compares whole +// words instead, which is the one thing that bug was about. namespace monoprop::config { namespace detail { -inline auto parse_flag(const char *value, bool default_value) -> bool { +// Case-insensitive whole-string compare; `lower` must already be lowercase. +inline auto iequals(const char *value, const char *lower) -> bool { + for (; *value != '\0' && *lower != '\0'; ++value, ++lower) { + const char c = (*value >= 'A' && *value <= 'Z') ? static_cast(*value - 'A' + 'a') : *value; + if (c != *lower) { + return false; + } + } + return *value == *lower; +} + +// Off when unset, empty, or a WHOLE-WORD match on 0/false/no/off; on otherwise. Whole words because +// first-character matching is what read `off` as ON. +inline auto parse_env_flag(const char *value) -> bool { if (value == nullptr || value[0] == '\0') { - return default_value; + return false; } - const char c = value[0]; - return !(c == '0' || c == 'f' || c == 'F' || c == 'n' || c == 'N'); + return !(iequals(value, "0") || iequals(value, "false") || iequals(value, "no") || iequals(value, "off")); } inline auto parse_positive_int(const char *text) -> std::optional { @@ -56,7 +72,7 @@ inline auto parse_positive_int(const char *text) -> std::optional { struct Settings { std::optional num_threads; - bool partition_pinning = true; + bool commplace = false; }; // Parse the environment once; the Settings are cached and shared across TUs. @@ -64,7 +80,7 @@ inline auto get() -> const Settings & { static const Settings 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.commplace = detail::parse_env_flag(std::getenv("monoprop_COMMPLACE")); return s; }(); return settings; diff --git a/cpp/monoprop/detail/partition/CMakeLists.txt b/cpp/monoprop/detail/partition/CMakeLists.txt index 19d4b69b..339bb2f5 100644 --- a/cpp/monoprop/detail/partition/CMakeLists.txt +++ b/cpp/monoprop/detail/partition/CMakeLists.txt @@ -6,6 +6,7 @@ target_sources( FILES "CpuTopology.h" "PartitionGroup.h" + "PlacementReport.h" ) target_sources(monoprop-objs PRIVATE CpuTopology.cpp) diff --git a/cpp/monoprop/detail/partition/CpuTopology.cpp b/cpp/monoprop/detail/partition/CpuTopology.cpp index 3eaee8f3..ca1d4903 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.cpp +++ b/cpp/monoprop/detail/partition/CpuTopology.cpp @@ -15,10 +15,12 @@ #include "monoprop/detail/partition/CpuTopology.h" #include +#include #include #include #include #include +#include #include #include @@ -276,12 +278,81 @@ auto masks_are_pairwise_disjoint(const uint64_t *masks, size_t n, size_t words) return true; } +/* ── Mask arithmetic for the placement report ──────────────────────────────── */ + +auto cpu_mask_popcount(const uint64_t *mask, size_t words) -> size_t { + if (mask == nullptr) { + return 0; + } + size_t n = 0; + for (size_t w = 0; w < words; ++w) { + n += static_cast(std::popcount(mask[w])); + } + return n; +} + +auto cpu_mask_union(uint64_t *out, const uint64_t *masks, size_t n, size_t words) -> void { + if (out == nullptr) { + return; + } + std::fill_n(out, words, uint64_t{0}); + if (masks == nullptr) { + return; + } + for (size_t r = 0; r < n; ++r) { + for (size_t w = 0; w < words; ++w) { + out[w] |= masks[(r * words) + w]; + } + } +} + +auto cpu_mask_ranges(const uint64_t *mask, size_t words) -> std::string { + if (mask == nullptr) { + return "none"; + } + std::string text; + size_t runs = 0; + size_t dropped = 0; + const size_t bits = words * 64; + for (size_t bit = 0; bit < bits;) { + if (((mask[bit / 64] >> (bit % 64)) & 1U) == 0U) { + ++bit; + continue; + } + size_t last = bit; + while (last + 1 < bits && ((mask[(last + 1) / 64] >> ((last + 1) % 64)) & 1U) != 0U) { + ++last; + } + ++runs; + // Counted before it is formatted, so ",+N" names runs that exist and were left out. + if (runs > kMaxCpuRanges) { + ++dropped; + } + else { + if (!text.empty()) { + text += ','; + } + // A single-CPU run is the bare id rather than "7-7". + text += std::to_string(bit); + if (last != bit) { + text += '-'; + text += std::to_string(last); + } + } + bit = last + 1; + } + if (text.empty()) { + return "none"; + } + if (dropped != 0) { + text += ",+" + std::to_string(dropped); + } + return text; +} + /* ── partition_cpusets ─────────────────────────────────────────────────────── */ auto partition_cpusets(size_t n, size_t group_index, size_t group_count, NodeMask mask) -> std::vector { - if (!config::get().partition_pinning) { - return {}; - } const auto cores = enumerate_physical_cores(); // A private mask IS this rank's share: the launcher already separated co-located ranks. diff --git a/cpp/monoprop/detail/partition/CpuTopology.h b/cpp/monoprop/detail/partition/CpuTopology.h index 2c2c1fca..fb5bc390 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.h +++ b/cpp/monoprop/detail/partition/CpuTopology.h @@ -20,6 +20,7 @@ * Policy: one partition per physical core, spread across L3/CCX domains, each worker thread pinned * to its representative PU. Falls back to unpinned execution when hwloc cannot load the topology or * when binding is unsupported — pinning is a performance optimisation, not a correctness requirement. + * Pinning has no runtime knob: leaving placement to the launcher measured propagate[hubbard] 2.90x slower. */ #pragma once @@ -27,10 +28,9 @@ #include #include #include +#include #include -#include "monoprop/detail/EnvConfig.h" - namespace monoprop::detail::partition { /*! @@ -83,10 +83,6 @@ auto placement_order(const std::vector &cores, size_t n, size_t gr * * @returns Vector of PhysicalCore in hwloc logical-core order, or empty when hwloc cannot load * the topology or when no core passes the affinity filter. - * - * @note This function deliberately ignores @c monoprop_PARTITION_PINNING so that the auto - * partition-count heuristic (one partition per physical core) works even when pinning is - * disabled by the user. */ auto enumerate_physical_cores() -> std::vector; @@ -104,6 +100,23 @@ auto affinity_mask_words(uint64_t *out, size_t nwords) -> bool; */ [[nodiscard]] auto masks_are_pairwise_disjoint(const uint64_t *masks, size_t n, size_t words) -> bool; +//! How many CPUs are set in the @p words -word mask at @p mask; 0 for a null mask. +[[nodiscard]] auto cpu_mask_popcount(const uint64_t *mask, size_t words) -> size_t; + +/*! @brief OR the @p n masks of @p words words laid end to end in @p masks into @p out (@p words words). + * The union says which CPUs the JOB got, as opposed to which this one rank got; @p out is cleared, not accumulated. + */ +auto cpu_mask_union(uint64_t *out, const uint64_t *masks, size_t n, size_t words) -> void; + +//! At most this many ascending runs are spelled out by cpu_mask_ranges(); the rest are counted in a ",+N" suffix. +inline constexpr size_t kMaxCpuRanges = 32; + +/*! @brief The set bits of @p mask as an ascending, comma-separated range list: "0-15,64-79". + * A single-CPU run is the bare id ("7"), an empty or null mask is "none", and truncation past + * kMaxCpuRanges is stated as a trailing ",+N" so a cut list is never read as a complete one. + */ +[[nodiscard]] auto cpu_mask_ranges(const uint64_t *mask, size_t words) -> std::string; + //! Whether the launcher has already handed this rank a private slice of the node, or the node's CPUs are shared. enum class NodeMask { Shared, PerRank }; @@ -119,8 +132,8 @@ enum class NodeMask { Shared, PerRank }; * @param group_count Total number of co-located ranks on the host. * @param mask NodeMask::PerRank only when the co-located ranks' affinity masks have been measured * pairwise DISJOINT (PartitionGroup::classify_node_masks_), so this mask is our share. - * @returns Vector of @p n CpuSet tokens, or empty when @c monoprop_PARTITION_PINNING is disabled, - * hwloc is unavailable, or fewer than @p group_count x @p n cores are visible (@p n under PerRank). + * @returns Vector of @p n CpuSet tokens, or empty when hwloc is unavailable or fewer than + * @p group_count x @p n cores are visible (@p n under PerRank). * * @note Under NodeMask::PerRank the group split is skipped: our share is already this rank's alone. */ diff --git a/cpp/monoprop/detail/partition/PartitionGroup.h b/cpp/monoprop/detail/partition/PartitionGroup.h index 31efb7ae..05952b7e 100644 --- a/cpp/monoprop/detail/partition/PartitionGroup.h +++ b/cpp/monoprop/detail/partition/PartitionGroup.h @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -26,6 +27,7 @@ #include #include +#include "monoprop/detail/EnvConfig.h" // config::get().commplace -- gates the COMMPLACE line only #include "monoprop/detail/mpi/Comm.h" #include "monoprop/detail/mpi/MPICompat.h" // mpi::size for the transport choice #include "monoprop/detail/mpi/ShmComm.h" @@ -33,6 +35,7 @@ #include "monoprop/detail/mpi/HybridComm.h" #endif #include "monoprop/detail/partition/CpuTopology.h" +#include "monoprop/detail/partition/PlacementReport.h" // Intra-process partition runtime: S master threads, each pinned to a core and running an independent // MonomialPropagator over one hash partition, with an in-process comm standing in for the network. @@ -61,6 +64,7 @@ class PartitionGroup { errs_(static_cast(n_partitions)) { make_transport_(); discover_node_peers_(); + report_placement_(); cpusets_ = topo_partition_cpusets(n_, node_rank_, node_size_, node_mask_); start_masters_(); // The masters are already running, so a ctor throw must not escape: ~PartitionGroup would never run, @@ -85,6 +89,7 @@ class PartitionGroup { partitions_(static_cast(src.n_)), errs_(static_cast(src.n_)) { make_transport_(); + // Deliberately no report_placement_(): the report describes the process, not the object. cpusets_ = topo_partition_cpusets(n_, node_rank_, node_size_, node_mask_); start_masters_(); try { // see the primary ctor: a throw past live masters would std::terminate @@ -168,8 +173,10 @@ class PartitionGroup { MPI_Comm_size(node, &node_size_); classify_node_masks_(node); MPI_Comm_free(&node); + return; } #endif + fill_own_report_(); } #ifdef monoprop_ENABLE_MPI @@ -177,6 +184,7 @@ class PartitionGroup { auto classify_node_masks_(MPI_Comm node) -> void { node_mask_ = NodeMask::Shared; if (node_size_ <= 1) { + fill_own_report_(); return; // nobody to collide with; the normal split already handles group_count == 1 } constexpr size_t kMaskWords = monoprop::detail::partition::kAffinityMaskWords; @@ -189,9 +197,59 @@ class PartitionGroup { static_cast(node_size_), kMaskWords); node_mask_ = disjoint ? NodeMask::PerRank : NodeMask::Shared; + fill_node_report_(mine.data(), all.data(), disjoint); + } + + /* COMMPLACE only, over the array MPI_Allgather already filled: no extra collective, and no + * reduction either, because every rank reads the same gathered rows and so reaches the same + * verdict. A row of zeroes is a rank whose mask did not fit the window, which is "unknown" + * rather than "shared" -- and is also why masks_are_pairwise_disjoint refuses it. */ + auto fill_node_report_(const uint64_t *mine, const uint64_t *all, bool disjoint) -> void { + if (!config::get().commplace) { + return; + } + constexpr size_t kMaskWords = monoprop::detail::partition::kAffinityMaskWords; + const auto peers = static_cast(node_size_); + report_.mpi_rank = mpi::rank(parent_); + report_.node_rank = node_rank_; + report_.node_size = node_size_; + for (size_t r = 0; r < peers; ++r) { + if (cpu_mask_popcount(all + (r * kMaskWords), kMaskWords) == 0) { + return; // leaves masks=unknown, uniformly on every rank + } + } + report_.masks = disjoint ? "private" : "shared"; + report_.cpus = cpu_mask_popcount(mine, kMaskWords); + std::vector node_union(kMaskWords, 0); + cpu_mask_union(node_union.data(), all, peers, kMaskWords); + report_.node_cpus = cpu_mask_popcount(node_union.data(), kMaskWords); + report_.node_cpu_list = cpu_mask_ranges(node_union.data(), kMaskWords); } #endif + /* COMMPLACE only, for the arm with no peers: one rank on the host, or a non-MPI parent. Nobody + * to be disjoint from, so the verdict is "alone" -- which is NOT "private", and must not be read + * as evidence that a multi-rank launcher did the right thing. */ + auto fill_own_report_() -> void { + if (!config::get().commplace) { + return; + } + constexpr size_t kMaskWords = monoprop::detail::partition::kAffinityMaskWords; + report_.mpi_rank = mpi::rank(parent_); + report_.node_rank = node_rank_; + report_.node_size = node_size_; + std::array mine{}; + if (affinity_mask_words(mine.data(), kMaskWords)) { + report_.masks = "alone"; + report_.cpus = cpu_mask_popcount(mine.data(), kMaskWords); + report_.node_cpus = report_.cpus; + report_.node_cpu_list = cpu_mask_ranges(mine.data(), kMaskWords); + } + } + + // Reports what the launcher did; decides nothing. Costs one branch when monoprop_COMMPLACE is unset. + auto report_placement_() -> void { (void)emit_place_line(stderr, config::get().commplace, report_); } + auto make_transport_() -> void { #ifdef monoprop_ENABLE_MPI if (parent_.kind == mpi::Comm::Kind::Mpi && mpi::size(parent_) > 1) { @@ -279,6 +337,7 @@ class PartitionGroup { 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) NodeMask node_mask_ = NodeMask::Shared; // set by classify_node_masks_; copied, never re-derived, by the copy ctor + PlacementReport report_; // filled only when monoprop_COMMPLACE is set; never read by any decision std::unique_ptr shm_; // set iff R == 1 #ifdef monoprop_ENABLE_MPI std::unique_ptr hyb_; // set iff R > 1 diff --git a/cpp/monoprop/detail/partition/PlacementReport.h b/cpp/monoprop/detail/partition/PlacementReport.h new file mode 100644 index 00000000..1bdbc9e1 --- /dev/null +++ b/cpp/monoprop/detail/partition/PlacementReport.h @@ -0,0 +1,84 @@ +// 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 + +/* ── COMMPLACE: what the launcher handed this rank ───────────────────────────── + * + * `--cpu-bind=none` and `--cpu-bind=cores` give the same thread count and the same partition count, + * and differ only in the mask the kernel enforces. A rank seeing 16 of a host's 128 CPUs is equally + * "Slurm gave me my own 16" and "eight of us share these 16"; only the peers' masks separate them, + * and the benchmark harness cannot substitute -- it samples /proc for the process it runs in, which + * says nothing about a peer rank on the same host. + * + * So the affinity-mask exchange PartitionGroup already runs to pick a placement also reports. + * Nothing here branches on any field below, and nothing below is computed when the knob is off. + * + * One line per rank, at propagator construction. A clone does not re-emit: the mask is a property + * of the process, and cloning does not change it. + */ + +namespace monoprop::detail::partition { + +struct PlacementReport { + int mpi_rank = 0; //!< rank in the propagator's parent communicator + int node_rank = 0; //!< this rank's index among the ranks sharing its host + int node_size = 1; //!< how many ranks share its host + + /*! How the co-located ranks' affinity masks relate. Four states, and the distinction between + * the last two is the whole point of exchanging them: + * "private" — pairwise disjoint: the launcher gave every rank its own share. + * "shared" — at least two ranks can be scheduled onto the same CPU. + * "alone" — this rank is the only one on its host; nothing to be disjoint from. + * "unknown" — some rank's mask did not fit the exchanged window, so no verdict is sound. + * A string literal, never freed and never copied. */ + const char *masks = "unknown"; + + size_t cpus = 0; //!< CPUs in THIS rank's mask + size_t node_cpus = 0; //!< CPUs in the union of the masks over this host + std::string node_cpu_list; //!< that union as ascending ranges, e.g. "0-15,64-79" +}; + +/*! @brief One COMMPLACE line on @p out when @p want, none otherwise; returns the lines written. + * + * A free function taking the flag rather than reading it, because monoprop_COMMPLACE is parsed once + * per process and cached: a test binary not launched with it set could not otherwise reach the + * emitting path, and the assertion would be skipped in the configuration everybody runs. + * + * The flag gates the printing and nothing else. The caller's mask exchange must NOT be gated on it: + * the environment is per-rank, so a predicate over it is not rank-uniform, and one rank skipping a + * collective its peers entered is a hang rather than a missing diagnostic. + */ +inline auto emit_place_line(std::FILE *out, bool want, const PlacementReport &r) -> int { + if (!want) { + return 0; + } + std::fprintf(out, + "COMMPLACE rank=%d node_rank=%d node_size=%d masks=%s cpus=%zu node_cpus=%zu cpu_list=%s\n", + r.mpi_rank, + r.node_rank, + r.node_size, + r.masks, + r.cpus, + r.node_cpus, + r.node_cpu_list.empty() ? "none" : r.node_cpu_list.c_str()); + std::fflush(out); + return 1; +} + +} // namespace monoprop::detail::partition diff --git a/cpp/tests/cpu_topology_tests.cpp b/cpp/tests/cpu_topology_tests.cpp index 1f64526e..95b214f9 100644 --- a/cpp/tests/cpu_topology_tests.cpp +++ b/cpp/tests/cpu_topology_tests.cpp @@ -24,17 +24,21 @@ #include #include +#include #include #include +#include #include +#include +#include #include #if defined(__linux__) #include #endif -#include "monoprop/detail/EnvConfig.h" // config::get().partition_pinning -- the one licensed empty placement #include "monoprop/detail/partition/CpuTopology.h" +#include "monoprop/detail/partition/PlacementReport.h" namespace partition = monoprop::detail::partition; using partition::topo_detail::placement_order; @@ -49,19 +53,6 @@ struct AffinityGuard { #endif }; -namespace { - -// An empty placement is licensed by pinning being off and by nothing else; "placed nothing" is the bug. -auto empty_placement_is_licensed() -> bool { - if (monoprop::config::get().partition_pinning) { - return false; - } - BOOST_TEST_MESSAGE("monoprop_PARTITION_PINNING is off; partition_cpusets places nothing"); - return true; -} - -} // namespace - /* ── Live smoke tests ─────────────────────────────────────────────────────── */ BOOST_AUTO_TEST_CASE(cpu_topology_enumerate_and_place) { @@ -76,8 +67,8 @@ BOOST_AUTO_TEST_CASE(cpu_topology_enumerate_and_place) { partition::pin_this_thread(one.front()); // guard restores affinity on scope exit } - // When topology discovery succeeds, a non-empty core list must produce a non-empty placement. - if (!cores.empty() && !(one.empty() && empty_placement_is_licensed())) { + // Pinning is unconditional, so a non-empty core list must produce a non-empty placement. + if (!cores.empty()) { BOOST_CHECK_EQUAL(one.size(), 1u); } @@ -119,9 +110,6 @@ BOOST_AUTO_TEST_CASE(cpu_topology_place_co_located_ranks) { /*group_index=*/1, /*group_count=*/2, partition::NodeMask::PerRank); - if (private_mask.empty() && empty_placement_is_licensed()) { - return; - } BOOST_REQUIRE_EQUAL(private_mask.size(), cores.size()); std::set placed; for (const auto &set : private_mask) { @@ -367,9 +355,6 @@ BOOST_AUTO_TEST_CASE(cpu_topology_per_rank_mask_still_places) { // What `srun --cpu-bind=cores` produces: our whole share, told there are eight sibling ranks. const auto sets = partition::partition_cpusets(/*n=*/k, /*group_index=*/3, /*group_count=*/8, partition::NodeMask::PerRank); - if (sets.empty() && empty_placement_is_licensed()) { - return; - } BOOST_REQUIRE_EQUAL(sets.size(), k); for (const auto &set : sets) { // Never pin outside the mask the launcher gave us. @@ -406,9 +391,6 @@ BOOST_AUTO_TEST_CASE(cpu_topology_shared_mask_keeps_co_located_ranks_disjoint) { /*group_index=*/1, /*group_count=*/2, partition::NodeMask::Shared); - if (rank0.empty() && rank1.empty() && empty_placement_is_licensed()) { - return; - } BOOST_REQUIRE_EQUAL(rank0.size(), per_rank); BOOST_REQUIRE_EQUAL(rank1.size(), per_rank); for (const auto &a : rank0) { @@ -420,3 +402,172 @@ BOOST_AUTO_TEST_CASE(cpu_topology_shared_mask_keeps_co_located_ranks_disjoint) { } #endif // __linux__ + +/* ── Mask arithmetic and the COMMPLACE line ───────────────────────────────── */ + +namespace { + +// A mask of the exchange width holding exactly `pus`, so the helpers see their real argument shape. +auto mask_of(const std::vector &pus) -> std::vector { + return packed_masks({pus}, partition::kAffinityMaskWords); +} + +/* A stream emit_place_line can be pointed at and read back. std::tmpfile rather than a named path: + * nothing here outlives the case, and CTest runs cases from a shared working directory. */ +class CaptureFile { +public: + CaptureFile() : f_(std::tmpfile()) { BOOST_REQUIRE(f_ != nullptr); } + ~CaptureFile() { + if (f_ != nullptr) { + std::fclose(f_); + } + } + CaptureFile(const CaptureFile &) = delete; + auto operator=(const CaptureFile &) -> CaptureFile & = delete; + + auto stream() const -> std::FILE * { return f_; } + + auto text() const -> std::string { + std::fflush(f_); + std::rewind(f_); + std::string out; + std::array buf{}; + while (const size_t n = std::fread(buf.data(), 1, buf.size(), f_)) { + out.append(buf.data(), n); + } + return out; + } + +private: + std::FILE *f_; +}; + +// Counted rather than searched for once: an instrument that fires the wrong number of times is +// invisible to a "contains" check. +auto count(std::string_view haystack, std::string_view needle) -> size_t { + size_t n = 0; + for (size_t at = haystack.find(needle); at != std::string_view::npos; at = haystack.find(needle, at + 1)) { + ++n; + } + return n; +} + +// Field extraction keyed on "name=", so a reordered line still reads. +auto field(std::string_view line, std::string_view name) -> std::string { + const size_t at = line.find(name); + BOOST_REQUIRE(at != std::string_view::npos); + const size_t start = at + name.size(); + const size_t end = line.find_first_of(" \n", start); + return std::string(line.substr(start, end - start)); +} + +} // namespace + +BOOST_AUTO_TEST_CASE(cpu_topology_mask_popcount_and_union) { + constexpr size_t kWords = partition::kAffinityMaskWords; + + const auto empty = mask_of({}); + BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(empty.data(), kWords), 0U); + BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(nullptr, kWords), 0U); + + const auto spread = mask_of({0, 63, 64, 4095}); + BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(spread.data(), kWords), 4U); + + // Two ranks holding 16 each and two ranks sharing the same 16 differ in exactly this number. + const auto two_private = packed_masks({{0, 1}, {2, 3}}, kWords); + std::vector u(kWords, 0); + partition::cpu_mask_union(u.data(), two_private.data(), 2, kWords); + BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(u.data(), kWords), 4U); + + const auto two_shared = packed_masks({{0, 1}, {0, 1}}, kWords); + partition::cpu_mask_union(u.data(), two_shared.data(), 2, kWords); + BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(u.data(), kWords), 2U); + + // The destination is cleared, not accumulated into: a reused buffer must not report the old CPUs. + const auto lone = packed_masks({{9}}, kWords); + partition::cpu_mask_union(u.data(), lone.data(), 1, kWords); + BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(u.data(), kWords), 1U); + + // n == 0 is a cleared destination, not untouched memory. + partition::cpu_mask_union(u.data(), two_private.data(), 0, kWords); + BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(u.data(), kWords), 0U); +} + +BOOST_AUTO_TEST_CASE(cpu_topology_mask_ranges_formatting) { + constexpr size_t kWords = partition::kAffinityMaskWords; + + // An empty mask is a WORD: a blank value in a KEY=value log line reads as a truncated line. + BOOST_CHECK_EQUAL(partition::cpu_mask_ranges(mask_of({}).data(), kWords), "none"); + BOOST_CHECK_EQUAL(partition::cpu_mask_ranges(nullptr, kWords), "none"); + + // A single CPU is the bare id, not "7-7". + BOOST_CHECK_EQUAL(partition::cpu_mask_ranges(mask_of({7}).data(), kWords), "7"); + + // The shape a correctly bound rank produces: one contiguous run. + std::vector block; + for (size_t i = 16; i < 32; ++i) { + block.push_back(i); + } + BOOST_CHECK_EQUAL(partition::cpu_mask_ranges(mask_of(block).data(), kWords), "16-31"); + + // A run crossing a 64-bit word boundary is ONE run: a per-word loop would split this into "63,64". + BOOST_CHECK_EQUAL(partition::cpu_mask_ranges(mask_of({63, 64}).data(), kWords), "63-64"); + BOOST_CHECK_EQUAL(partition::cpu_mask_ranges(mask_of({0, 1, 64, 65, 130}).data(), kWords), "0-1,64-65,130"); + + // Truncation is stated, never silent: a cut list that looked complete would be read as a smaller machine. + std::vector sparse; + for (size_t i = 0; i < partition::kMaxCpuRanges + 8; ++i) { + sparse.push_back(i * 2); // isolated bits ⇒ one run each + } + const auto truncated = partition::cpu_mask_ranges(mask_of(sparse).data(), kWords); + BOOST_CHECK_EQUAL(truncated.substr(truncated.size() - 3), ",+8"); + BOOST_CHECK_EQUAL(count(truncated, ","), partition::kMaxCpuRanges); + BOOST_CHECK_EQUAL(truncated.substr(0, 6), "0,2,4,"); +} + +// The flag is a parameter so this is reachable: monoprop_COMMPLACE is parsed once per process and +// cached, so a binary not launched with it set could not otherwise reach the emitting path. +BOOST_AUTO_TEST_CASE(cpu_topology_place_line_reports_every_field) { + const CaptureFile cap; + partition::PlacementReport rep; + rep.mpi_rank = 5; + rep.node_rank = 2; + rep.node_size = 8; + rep.masks = "private"; + rep.cpus = 16; + rep.node_cpus = 128; + rep.node_cpu_list = "0-127"; + + BOOST_CHECK_EQUAL(partition::emit_place_line(cap.stream(), true, rep), 1); + const auto text = cap.text(); + BOOST_CHECK_EQUAL(count(text, "COMMPLACE"), 1U); + BOOST_CHECK_EQUAL(count(text, "\n"), 1U); // terminated, so a second line cannot merge into it + BOOST_CHECK_EQUAL(field(text, "rank="), "5"); + BOOST_CHECK_EQUAL(field(text, "node_rank="), "2"); + BOOST_CHECK_EQUAL(field(text, "node_size="), "8"); + BOOST_CHECK_EQUAL(field(text, "masks="), "private"); + BOOST_CHECK_EQUAL(field(text, "cpus="), "16"); + BOOST_CHECK_EQUAL(field(text, "node_cpus="), "128"); + BOOST_CHECK_EQUAL(field(text, "cpu_list="), "0-127"); +} + +// The other half of the contract: "the knob was off" and "the instrument never fired" must be the +// same observation only when the knob really is off. +BOOST_AUTO_TEST_CASE(cpu_topology_place_line_is_gated) { + const CaptureFile cap; + const partition::PlacementReport rep; + BOOST_CHECK_EQUAL(partition::emit_place_line(cap.stream(), false, rep), 0); + BOOST_CHECK(cap.text().empty()); +} + +// A default report is the "could not classify" state and must SAY so rather than print a plausible zero. +BOOST_AUTO_TEST_CASE(cpu_topology_place_line_default_is_unknown_not_a_verdict) { + const CaptureFile cap; + const partition::PlacementReport rep; + BOOST_CHECK_EQUAL(partition::emit_place_line(cap.stream(), true, rep), 1); + const auto text = cap.text(); + BOOST_CHECK_EQUAL(field(text, "masks="), "unknown"); + BOOST_CHECK_EQUAL(field(text, "cpus="), "0"); + BOOST_CHECK_EQUAL(field(text, "node_size="), "1"); + BOOST_CHECK_EQUAL(field(text, "cpu_list="), "none"); // an unset list is still a word +} diff --git a/cpp/tests/env_config_tests.cpp b/cpp/tests/env_config_tests.cpp index 77ff0955..66118a30 100644 --- a/cpp/tests/env_config_tests.cpp +++ b/cpp/tests/env_config_tests.cpp @@ -18,30 +18,37 @@ #include "monoprop/detail/EnvConfig.h" -using monoprop::config::detail::parse_flag; +using monoprop::config::detail::parse_env_flag; using monoprop::config::detail::parse_positive_int; -BOOST_AUTO_TEST_CASE(env_config_parse_flag_default_when_unset_or_empty) { - BOOST_CHECK_EQUAL(parse_flag(nullptr, true), true); - BOOST_CHECK_EQUAL(parse_flag(nullptr, false), false); - BOOST_CHECK_EQUAL(parse_flag("", true), true); - BOOST_CHECK_EQUAL(parse_flag("", false), false); +BOOST_AUTO_TEST_CASE(env_config_parse_env_flag_unset_and_empty_are_off) { + BOOST_CHECK_EQUAL(parse_env_flag(nullptr), false); + BOOST_CHECK_EQUAL(parse_env_flag(""), false); } -BOOST_AUTO_TEST_CASE(env_config_parse_flag_falsey_first_char) { - // Only the first character decides, so "0abc" is falsey too. - for (const char *v : {"0", "f", "F", "n", "N"}) { - BOOST_CHECK_MESSAGE(parse_flag(v, true) == false, v); +BOOST_AUTO_TEST_CASE(env_config_parse_env_flag_falsey_words_in_any_case) { + // `off` and `OFF` are the cases the deleted first-character parser read as ON. + for (const char *v : {"0", "false", "FALSE", "False", "no", "NO", "No", "off", "OFF", "Off"}) { + BOOST_CHECK_MESSAGE(parse_env_flag(v) == false, v); } - BOOST_CHECK_EQUAL(parse_flag("0abc", true), false); } -BOOST_AUTO_TEST_CASE(env_config_parse_flag_truthy_first_char) { - for (const char *v : {"1", "t", "T", "y", "Y", "on", "true", "anything"}) { - BOOST_CHECK_MESSAGE(parse_flag(v, false) == true, v); +BOOST_AUTO_TEST_CASE(env_config_parse_env_flag_truthy_words) { + for (const char *v : {"1", "true", "TRUE", "yes", "on", "ON", "anything"}) { + BOOST_CHECK_MESSAGE(parse_env_flag(v), v); } } +BOOST_AUTO_TEST_CASE(env_config_parse_env_flag_compares_whole_words) { + // A first-character parser calls every one of these off; a whole-word one calls them all on. + for (const char *v : {"offbeat", "november", "0abc", "nope", "falsey", "f", "n"}) { + BOOST_CHECK_MESSAGE(parse_env_flag(v), v); + } + // The converse: a prefix of a falsey word is not that word either. + BOOST_CHECK_EQUAL(parse_env_flag("of"), true); + BOOST_CHECK_EQUAL(parse_env_flag("fals"), true); +} + BOOST_AUTO_TEST_CASE(env_config_parse_positive_int_null_and_malformed) { BOOST_CHECK(parse_positive_int(nullptr) == std::nullopt); BOOST_CHECK(parse_positive_int("") == std::nullopt); @@ -64,5 +71,5 @@ BOOST_AUTO_TEST_CASE(env_config_settings_cached_singleton) { const auto &b = monoprop::config::get(); BOOST_CHECK_EQUAL(&a, &b); // Touch a field so the Settings aggregate is actually read. - BOOST_CHECK(a.partition_pinning == true || a.partition_pinning == false); + BOOST_CHECK(a.commplace == true || a.commplace == false); } diff --git a/docs/content/docs/features/parallelism.mdx b/docs/content/docs/features/parallelism.mdx index bf57b046..7bc98c34 100644 --- a/docs/content/docs/features/parallelism.mdx +++ b/docs/content/docs/features/parallelism.mdx @@ -29,7 +29,10 @@ 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. Supported on platforms where hwloc can bind threads. | +| `monoprop_COMMPLACE` | `off` | Report-only. Writes one `COMMPLACE` line per rank to stderr when the propagator is built, naming the CPUs the launcher gave that rank and whether co-located ranks were handed disjoint masks. `0`, `false`, `no` and `off` (whole word, any case) disable it; any other non-empty value enables it. It changes no placement decision. | + +Pinning each partition to a core is not configurable: leaving placement to the +launcher measured `propagate[hubbard]` 2.90x slower, so the disabled arm is gone. ```bash # Run 8 partitions instead of one-per-core: From fc2757dc147c88582c9722c08245e2041021fec5 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Mon, 24 Aug 2026 14:56:32 +0100 Subject: [PATCH 2/4] =?UTF-8?q?refactor(partition):=20=F0=9F=8E=A8=20halve?= =?UTF-8?q?=20the=20COMMPLACE=20diagnostic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same COMMPLACE output, same four masks= verdicts, in 7 files +218/-51 instead of 9 files +464/-59, with comments back at house density. - summarize_masks replaces cpu_mask_popcount, cpu_mask_union and cpu_mask_ranges. hwloc_bitmap_weight / _or / _list_snprintf already do this in a TU that includes , so the hand-rolled run-length encoder, the public kMaxCpuRanges and the ",+N" run counter go. The truncation marker now fires on a 512-char line rather than a 32-run one, which prints MORE of a sparse mask, and cuts back to the last whole range so it never follows a half-written CPU id. - PlacementReport.h is deleted, with its struct and its PartitionGroup member. They existed only to carry state between two adjacent calls: the ctor emitted immediately after discover_node_peers_() filled the report, so report_placement_ now formats and writes where it classifies. That also removes the copy-ctor question of whether a clone re-emits -- it is reached only from the primary ctor. format_place_line lives beside summarize_masks in CpuTopology, so the PR adds no file and no CMakeLists entry. - One report_placement_ replaces fill_node_report_ and fill_own_report_; the `mine` argument was redundant, since row node_rank_ of the gathered array IS our own mask. - format_place_line returns the line instead of writing it to a FILE*, which is what makes the emitting path reachable in a binary launched without the knob set. The tests' tmpfile CaptureFile, count() and field() helpers go with it, and the checks are whole-line rather than seven field extractions. - parse_flag keeps its (value, default_value) signature; only the comparison changes to whole words. Narrowing it to parse_env_flag(value) rewrote four tests and cannot express a default-ON flag, which a sibling branch already has. - Corrected: `disabled` parses as ON under BOTH the old and the new rule, so it is not evidence of the fix. Only `off`/`OFF`/`Off` are, which is why the parser change is demoted to a consequence of adding a knob rather than presented as the repair of a bug that bit anything. Co-Authored-By: Claude Opus 5 --- cpp/monoprop/detail/EnvConfig.h | 15 +- cpp/monoprop/detail/partition/CMakeLists.txt | 1 - cpp/monoprop/detail/partition/CpuTopology.cpp | 112 +++++----- cpp/monoprop/detail/partition/CpuTopology.h | 40 ++-- .../detail/partition/PartitionGroup.h | 74 +++---- .../detail/partition/PlacementReport.h | 84 -------- cpp/tests/cpu_topology_tests.cpp | 200 ++++-------------- cpp/tests/env_config_tests.cpp | 31 ++- docs/content/docs/features/parallelism.mdx | 5 +- 9 files changed, 162 insertions(+), 400 deletions(-) delete mode 100644 cpp/monoprop/detail/partition/PlacementReport.h diff --git a/cpp/monoprop/detail/EnvConfig.h b/cpp/monoprop/detail/EnvConfig.h index 39e31cf2..e3d6c016 100644 --- a/cpp/monoprop/detail/EnvConfig.h +++ b/cpp/monoprop/detail/EnvConfig.h @@ -21,13 +21,9 @@ // Single home for runtime environment configuration. Kept dependency-free by design, because it is // pulled into hot-path headers. // -// monoprop_NUM_THREADS positive int (1..1e6), else ignored → num_threads +// monoprop_NUM_THREADS positive int (1..1e6), else ignored → num_threads // monoprop_COMMPLACE bool, default OFF; one COMMPLACE line per rank on stderr → commplace // monoprop_PARTITIONS int N | "auto" | "off"; parsed where it is used (resolve_partition_count_) -// -// monoprop_PARTITION_PINNING is deleted and pinning is unconditional. Its parser matched only the -// first CHARACTER, so `off`, `OFF` and `disabled` all parsed as ON; parse_env_flag compares whole -// words instead, which is the one thing that bug was about. namespace monoprop::config { @@ -44,12 +40,11 @@ inline auto iequals(const char *value, const char *lower) -> bool { return *value == *lower; } -// Off when unset, empty, or a WHOLE-WORD match on 0/false/no/off; on otherwise. Whole words because -// first-character matching is what read `off` as ON. -inline auto parse_env_flag(const char *value) -> bool { +inline auto parse_flag(const char *value, bool default_value) -> bool { if (value == nullptr || value[0] == '\0') { - return false; + return default_value; } + // Whole words: matching only the first character read `off` and `OFF` as ON. return !(iequals(value, "0") || iequals(value, "false") || iequals(value, "no") || iequals(value, "off")); } @@ -80,7 +75,7 @@ inline auto get() -> const Settings & { static const Settings settings = [] { Settings s; s.num_threads = detail::parse_positive_int(std::getenv("monoprop_NUM_THREADS")); - s.commplace = detail::parse_env_flag(std::getenv("monoprop_COMMPLACE")); + s.commplace = detail::parse_flag(std::getenv("monoprop_COMMPLACE"), false); return s; }(); return settings; diff --git a/cpp/monoprop/detail/partition/CMakeLists.txt b/cpp/monoprop/detail/partition/CMakeLists.txt index 339bb2f5..19d4b69b 100644 --- a/cpp/monoprop/detail/partition/CMakeLists.txt +++ b/cpp/monoprop/detail/partition/CMakeLists.txt @@ -6,7 +6,6 @@ target_sources( FILES "CpuTopology.h" "PartitionGroup.h" - "PlacementReport.h" ) target_sources(monoprop-objs PRIVATE CpuTopology.cpp) diff --git a/cpp/monoprop/detail/partition/CpuTopology.cpp b/cpp/monoprop/detail/partition/CpuTopology.cpp index ca1d4903..abc34255 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.cpp +++ b/cpp/monoprop/detail/partition/CpuTopology.cpp @@ -15,8 +15,9 @@ #include "monoprop/detail/partition/CpuTopology.h" #include -#include +#include #include +#include #include #include #include @@ -278,76 +279,63 @@ auto masks_are_pairwise_disjoint(const uint64_t *masks, size_t n, size_t words) return true; } -/* ── Mask arithmetic for the placement report ──────────────────────────────── */ +/* ── summarize_masks ──────────────────────────────────────────────────────── */ -auto cpu_mask_popcount(const uint64_t *mask, size_t words) -> size_t { - if (mask == nullptr) { - return 0; - } - size_t n = 0; - for (size_t w = 0; w < words; ++w) { - n += static_cast(std::popcount(mask[w])); - } - return n; -} +// hwloc indexes a bitmap in unsigned long units, so a 64-bit word must be one of them. +static_assert(sizeof(unsigned long) == sizeof(uint64_t), "the mask word is not an hwloc bitmap unit"); -auto cpu_mask_union(uint64_t *out, const uint64_t *masks, size_t n, size_t words) -> void { - if (out == nullptr) { - return; +auto summarize_masks(const uint64_t *masks, size_t n, size_t words, size_t self) -> std::optional { + if (masks == nullptr || n == 0 || words == 0 || self >= n) { + return std::nullopt; } - std::fill_n(out, words, uint64_t{0}); - if (masks == nullptr) { - return; + const hwloc_bitmap_t row = hwloc_bitmap_alloc(); + const hwloc_bitmap_t all = hwloc_bitmap_alloc(); + if (row == nullptr || all == nullptr) { + hwloc_bitmap_free(row); + hwloc_bitmap_free(all); + return std::nullopt; } - for (size_t r = 0; r < n; ++r) { + MaskSummary out; + bool ok = true; + for (size_t r = 0; r < n && ok; ++r) { + hwloc_bitmap_zero(row); for (size_t w = 0; w < words; ++w) { - out[w] |= masks[(r * words) + w]; - } - } -} - -auto cpu_mask_ranges(const uint64_t *mask, size_t words) -> std::string { - if (mask == nullptr) { - return "none"; - } - std::string text; - size_t runs = 0; - size_t dropped = 0; - const size_t bits = words * 64; - for (size_t bit = 0; bit < bits;) { - if (((mask[bit / 64] >> (bit % 64)) & 1U) == 0U) { - ++bit; - continue; - } - size_t last = bit; - while (last + 1 < bits && ((mask[(last + 1) / 64] >> ((last + 1) % 64)) & 1U) != 0U) { - ++last; + hwloc_bitmap_set_ith_ulong(row, static_cast(w), masks[(r * words) + w]); } - ++runs; - // Counted before it is formatted, so ",+N" names runs that exist and were left out. - if (runs > kMaxCpuRanges) { - ++dropped; + ok = hwloc_bitmap_weight(row) > 0; // an all-zero row is a mask that did not fit the exchange window + if (r == self) { + out.cpus = static_cast(hwloc_bitmap_weight(row)); } - else { - if (!text.empty()) { - text += ','; - } - // A single-CPU run is the bare id rather than "7-7". - text += std::to_string(bit); - if (last != bit) { - text += '-'; - text += std::to_string(last); - } + hwloc_bitmap_or(all, all, row); + } + if (ok) { + out.node_cpus = static_cast(hwloc_bitmap_weight(all)); + std::array text{}; + const int need = hwloc_bitmap_list_snprintf(text.data(), text.size(), all); + out.cpu_list = text.data(); + // Truncation is stated, never silent: a cut list read as complete is a smaller machine. Cut + // back to the last whole range first, so the marker never follows a half-written CPU id. + if (need >= static_cast(text.size())) { + const size_t last = out.cpu_list.rfind(','); + out.cpu_list.resize(last == std::string::npos ? 0 : last + 1); + out.cpu_list += "+"; } - bit = last + 1; - } - if (text.empty()) { - return "none"; - } - if (dropped != 0) { - text += ",+" + std::to_string(dropped); } - return text; + hwloc_bitmap_free(row); + hwloc_bitmap_free(all); + return ok ? std::optional{out} : std::nullopt; +} + +auto format_place_line(int mpi_rank, int node_rank, int node_size, const char *verdict, const MaskSummary &summary) + -> std::string { + return std::format("COMMPLACE rank={} node_rank={} node_size={} masks={} cpus={} node_cpus={} cpu_list={}\n", + mpi_rank, + node_rank, + node_size, + verdict, + summary.cpus, + summary.node_cpus, + summary.cpu_list); } /* ── partition_cpusets ─────────────────────────────────────────────────────── */ diff --git a/cpp/monoprop/detail/partition/CpuTopology.h b/cpp/monoprop/detail/partition/CpuTopology.h index fb5bc390..6bd6911d 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.h +++ b/cpp/monoprop/detail/partition/CpuTopology.h @@ -28,6 +28,7 @@ #include #include #include +#include #include #include @@ -100,22 +101,35 @@ auto affinity_mask_words(uint64_t *out, size_t nwords) -> bool; */ [[nodiscard]] auto masks_are_pairwise_disjoint(const uint64_t *masks, size_t n, size_t words) -> bool; -//! How many CPUs are set in the @p words -word mask at @p mask; 0 for a null mask. -[[nodiscard]] auto cpu_mask_popcount(const uint64_t *mask, size_t words) -> size_t; +//! What one host's exchanged affinity masks add up to. The union says what the JOB got, not what one rank got. +struct MaskSummary { + size_t cpus = 0; //!< CPUs in our own mask + size_t node_cpus = 0; //!< CPUs in the union over the host + std::string cpu_list = "none"; //!< that union as ascending ranges, "0-15,64-79" +}; -/*! @brief OR the @p n masks of @p words words laid end to end in @p masks into @p out (@p words words). - * The union says which CPUs the JOB got, as opposed to which this one rank got; @p out is cleared, not accumulated. +/*! @brief Summarize the @p n masks of @p words words laid end to end in @p masks, row @p self being ours. + * @returns nullopt for a null/empty argument or any all-zero row: a mask that did not fit the exchange + * window cannot be summed with the others. Diagnostic only; nothing branches on the result. */ -auto cpu_mask_union(uint64_t *out, const uint64_t *masks, size_t n, size_t words) -> void; - -//! At most this many ascending runs are spelled out by cpu_mask_ranges(); the rest are counted in a ",+N" suffix. -inline constexpr size_t kMaxCpuRanges = 32; - -/*! @brief The set bits of @p mask as an ascending, comma-separated range list: "0-15,64-79". - * A single-CPU run is the bare id ("7"), an empty or null mask is "none", and truncation past - * kMaxCpuRanges is stated as a trailing ",+N" so a cut list is never read as a complete one. +[[nodiscard]] auto summarize_masks(const uint64_t *masks, size_t n, size_t words, size_t self) + -> std::optional; + +/* COMMPLACE (monoprop_COMMPLACE): a rank seeing 16 of a host's 128 CPUs is equally "my own 16" and + * "eight of us share these 16", and only the co-located ranks' masks separate them, so the exchange + * PartitionGroup already runs to place also reports. Report-only; nothing branches on the line. */ + +/*! @brief One newline-terminated COMMPLACE line naming what the launcher handed this rank. + * @param verdict how the co-located masks relate: "private" (pairwise disjoint), "shared" (two ranks + * can land on one CPU), "alone" (the only rank on its host, which is NOT "private"), or + * "unknown" (a mask did not fit the exchanged window, so no verdict is sound). + * Returned, not written, so a binary launched without the knob set still reaches the formatting. */ -[[nodiscard]] auto cpu_mask_ranges(const uint64_t *mask, size_t words) -> std::string; +[[nodiscard]] auto format_place_line(int mpi_rank, + int node_rank, + int node_size, + const char *verdict, + const MaskSummary &summary) -> std::string; //! Whether the launcher has already handed this rank a private slice of the node, or the node's CPUs are shared. enum class NodeMask { Shared, PerRank }; diff --git a/cpp/monoprop/detail/partition/PartitionGroup.h b/cpp/monoprop/detail/partition/PartitionGroup.h index 05952b7e..d7a13590 100644 --- a/cpp/monoprop/detail/partition/PartitionGroup.h +++ b/cpp/monoprop/detail/partition/PartitionGroup.h @@ -35,7 +35,6 @@ #include "monoprop/detail/mpi/HybridComm.h" #endif #include "monoprop/detail/partition/CpuTopology.h" -#include "monoprop/detail/partition/PlacementReport.h" // Intra-process partition runtime: S master threads, each pinned to a core and running an independent // MonomialPropagator over one hash partition, with an in-process comm standing in for the network. @@ -64,7 +63,6 @@ class PartitionGroup { errs_(static_cast(n_partitions)) { make_transport_(); discover_node_peers_(); - report_placement_(); cpusets_ = topo_partition_cpusets(n_, node_rank_, node_size_, node_mask_); start_masters_(); // The masters are already running, so a ctor throw must not escape: ~PartitionGroup would never run, @@ -89,7 +87,6 @@ class PartitionGroup { partitions_(static_cast(src.n_)), errs_(static_cast(src.n_)) { make_transport_(); - // Deliberately no report_placement_(): the report describes the process, not the object. cpusets_ = topo_partition_cpusets(n_, node_rank_, node_size_, node_mask_); start_masters_(); try { // see the primary ctor: a throw past live masters would std::terminate @@ -176,7 +173,7 @@ class PartitionGroup { return; } #endif - fill_own_report_(); + report_placement_(nullptr, 0, "alone"); } #ifdef monoprop_ENABLE_MPI @@ -184,7 +181,7 @@ class PartitionGroup { auto classify_node_masks_(MPI_Comm node) -> void { node_mask_ = NodeMask::Shared; if (node_size_ <= 1) { - fill_own_report_(); + report_placement_(nullptr, 0, "alone"); return; // nobody to collide with; the normal split already handles group_count == 1 } constexpr size_t kMaskWords = monoprop::detail::partition::kAffinityMaskWords; @@ -197,59 +194,37 @@ class PartitionGroup { static_cast(node_size_), kMaskWords); node_mask_ = disjoint ? NodeMask::PerRank : NodeMask::Shared; - fill_node_report_(mine.data(), all.data(), disjoint); - } - - /* COMMPLACE only, over the array MPI_Allgather already filled: no extra collective, and no - * reduction either, because every rank reads the same gathered rows and so reaches the same - * verdict. A row of zeroes is a rank whose mask did not fit the window, which is "unknown" - * rather than "shared" -- and is also why masks_are_pairwise_disjoint refuses it. */ - auto fill_node_report_(const uint64_t *mine, const uint64_t *all, bool disjoint) -> void { - if (!config::get().commplace) { - return; - } - constexpr size_t kMaskWords = monoprop::detail::partition::kAffinityMaskWords; - const auto peers = static_cast(node_size_); - report_.mpi_rank = mpi::rank(parent_); - report_.node_rank = node_rank_; - report_.node_size = node_size_; - for (size_t r = 0; r < peers; ++r) { - if (cpu_mask_popcount(all + (r * kMaskWords), kMaskWords) == 0) { - return; // leaves masks=unknown, uniformly on every rank - } - } - report_.masks = disjoint ? "private" : "shared"; - report_.cpus = cpu_mask_popcount(mine, kMaskWords); - std::vector node_union(kMaskWords, 0); - cpu_mask_union(node_union.data(), all, peers, kMaskWords); - report_.node_cpus = cpu_mask_popcount(node_union.data(), kMaskWords); - report_.node_cpu_list = cpu_mask_ranges(node_union.data(), kMaskWords); + report_placement_(all.data(), static_cast(node_size_), disjoint ? "private" : "shared"); } #endif - /* COMMPLACE only, for the arm with no peers: one rank on the host, or a non-MPI parent. Nobody - * to be disjoint from, so the verdict is "alone" -- which is NOT "private", and must not be read - * as evidence that a multi-rank launcher did the right thing. */ - auto fill_own_report_() -> void { + /* COMMPLACE only, over the array MPI_Allgather already filled: no extra collective, and no + * reduction either, since every rank reads the same rows and so reaches the same verdict. `masks` + * nullptr means no peers, so measure our own mask; the verdict is then "alone", which is NOT + * evidence that a multi-rank launcher did the right thing. Reached only from the primary ctor, so + * a clone does not re-emit -- the mask belongs to the process, not the object. */ + auto report_placement_(const uint64_t *masks, size_t peers, const char *verdict) -> void { if (!config::get().commplace) { return; } - constexpr size_t kMaskWords = monoprop::detail::partition::kAffinityMaskWords; - report_.mpi_rank = mpi::rank(parent_); - report_.node_rank = node_rank_; - report_.node_size = node_size_; - std::array mine{}; - if (affinity_mask_words(mine.data(), kMaskWords)) { - report_.masks = "alone"; - report_.cpus = cpu_mask_popcount(mine.data(), kMaskWords); - report_.node_cpus = report_.cpus; - report_.node_cpu_list = cpu_mask_ranges(mine.data(), kMaskWords); + constexpr size_t kWords = monoprop::detail::partition::kAffinityMaskWords; + std::array own{}; + if (masks == nullptr && affinity_mask_words(own.data(), kWords)) { + masks = own.data(); + peers = 1; } + // No summary is "unknown" rather than a plausible zero: some mask did not fit the window. + const auto sum = summarize_masks(masks, peers, kWords, static_cast(node_rank_)); + std::fputs(format_place_line(mpi::rank(parent_), + node_rank_, + node_size_, + sum ? verdict : "unknown", + sum.value_or(MaskSummary{})) + .c_str(), + stderr); + std::fflush(stderr); } - // Reports what the launcher did; decides nothing. Costs one branch when monoprop_COMMPLACE is unset. - auto report_placement_() -> void { (void)emit_place_line(stderr, config::get().commplace, report_); } - auto make_transport_() -> void { #ifdef monoprop_ENABLE_MPI if (parent_.kind == mpi::Comm::Kind::Mpi && mpi::size(parent_) > 1) { @@ -337,7 +312,6 @@ class PartitionGroup { 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) NodeMask node_mask_ = NodeMask::Shared; // set by classify_node_masks_; copied, never re-derived, by the copy ctor - PlacementReport report_; // filled only when monoprop_COMMPLACE is set; never read by any decision std::unique_ptr shm_; // set iff R == 1 #ifdef monoprop_ENABLE_MPI std::unique_ptr hyb_; // set iff R > 1 diff --git a/cpp/monoprop/detail/partition/PlacementReport.h b/cpp/monoprop/detail/partition/PlacementReport.h deleted file mode 100644 index 1bdbc9e1..00000000 --- a/cpp/monoprop/detail/partition/PlacementReport.h +++ /dev/null @@ -1,84 +0,0 @@ -// 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 - -/* ── COMMPLACE: what the launcher handed this rank ───────────────────────────── - * - * `--cpu-bind=none` and `--cpu-bind=cores` give the same thread count and the same partition count, - * and differ only in the mask the kernel enforces. A rank seeing 16 of a host's 128 CPUs is equally - * "Slurm gave me my own 16" and "eight of us share these 16"; only the peers' masks separate them, - * and the benchmark harness cannot substitute -- it samples /proc for the process it runs in, which - * says nothing about a peer rank on the same host. - * - * So the affinity-mask exchange PartitionGroup already runs to pick a placement also reports. - * Nothing here branches on any field below, and nothing below is computed when the knob is off. - * - * One line per rank, at propagator construction. A clone does not re-emit: the mask is a property - * of the process, and cloning does not change it. - */ - -namespace monoprop::detail::partition { - -struct PlacementReport { - int mpi_rank = 0; //!< rank in the propagator's parent communicator - int node_rank = 0; //!< this rank's index among the ranks sharing its host - int node_size = 1; //!< how many ranks share its host - - /*! How the co-located ranks' affinity masks relate. Four states, and the distinction between - * the last two is the whole point of exchanging them: - * "private" — pairwise disjoint: the launcher gave every rank its own share. - * "shared" — at least two ranks can be scheduled onto the same CPU. - * "alone" — this rank is the only one on its host; nothing to be disjoint from. - * "unknown" — some rank's mask did not fit the exchanged window, so no verdict is sound. - * A string literal, never freed and never copied. */ - const char *masks = "unknown"; - - size_t cpus = 0; //!< CPUs in THIS rank's mask - size_t node_cpus = 0; //!< CPUs in the union of the masks over this host - std::string node_cpu_list; //!< that union as ascending ranges, e.g. "0-15,64-79" -}; - -/*! @brief One COMMPLACE line on @p out when @p want, none otherwise; returns the lines written. - * - * A free function taking the flag rather than reading it, because monoprop_COMMPLACE is parsed once - * per process and cached: a test binary not launched with it set could not otherwise reach the - * emitting path, and the assertion would be skipped in the configuration everybody runs. - * - * The flag gates the printing and nothing else. The caller's mask exchange must NOT be gated on it: - * the environment is per-rank, so a predicate over it is not rank-uniform, and one rank skipping a - * collective its peers entered is a hang rather than a missing diagnostic. - */ -inline auto emit_place_line(std::FILE *out, bool want, const PlacementReport &r) -> int { - if (!want) { - return 0; - } - std::fprintf(out, - "COMMPLACE rank=%d node_rank=%d node_size=%d masks=%s cpus=%zu node_cpus=%zu cpu_list=%s\n", - r.mpi_rank, - r.node_rank, - r.node_size, - r.masks, - r.cpus, - r.node_cpus, - r.node_cpu_list.empty() ? "none" : r.node_cpu_list.c_str()); - std::fflush(out); - return 1; -} - -} // namespace monoprop::detail::partition diff --git a/cpp/tests/cpu_topology_tests.cpp b/cpp/tests/cpu_topology_tests.cpp index 95b214f9..a129be56 100644 --- a/cpp/tests/cpu_topology_tests.cpp +++ b/cpp/tests/cpu_topology_tests.cpp @@ -24,13 +24,10 @@ #include #include -#include #include #include -#include #include #include -#include #include #if defined(__linux__) @@ -38,7 +35,6 @@ #endif #include "monoprop/detail/partition/CpuTopology.h" -#include "monoprop/detail/partition/PlacementReport.h" namespace partition = monoprop::detail::partition; using partition::topo_detail::placement_order; @@ -403,171 +399,49 @@ BOOST_AUTO_TEST_CASE(cpu_topology_shared_mask_keeps_co_located_ranks_disjoint) { #endif // __linux__ -/* ── Mask arithmetic and the COMMPLACE line ───────────────────────────────── */ +/* ── summarize_masks and the COMMPLACE line ───────────────────────────────── */ -namespace { - -// A mask of the exchange width holding exactly `pus`, so the helpers see their real argument shape. -auto mask_of(const std::vector &pus) -> std::vector { - return packed_masks({pus}, partition::kAffinityMaskWords); -} - -/* A stream emit_place_line can be pointed at and read back. std::tmpfile rather than a named path: - * nothing here outlives the case, and CTest runs cases from a shared working directory. */ -class CaptureFile { -public: - CaptureFile() : f_(std::tmpfile()) { BOOST_REQUIRE(f_ != nullptr); } - ~CaptureFile() { - if (f_ != nullptr) { - std::fclose(f_); - } - } - CaptureFile(const CaptureFile &) = delete; - auto operator=(const CaptureFile &) -> CaptureFile & = delete; - - auto stream() const -> std::FILE * { return f_; } - - auto text() const -> std::string { - std::fflush(f_); - std::rewind(f_); - std::string out; - std::array buf{}; - while (const size_t n = std::fread(buf.data(), 1, buf.size(), f_)) { - out.append(buf.data(), n); - } - return out; - } - -private: - std::FILE *f_; -}; - -// Counted rather than searched for once: an instrument that fires the wrong number of times is -// invisible to a "contains" check. -auto count(std::string_view haystack, std::string_view needle) -> size_t { - size_t n = 0; - for (size_t at = haystack.find(needle); at != std::string_view::npos; at = haystack.find(needle, at + 1)) { - ++n; - } - return n; -} - -// Field extraction keyed on "name=", so a reordered line still reads. -auto field(std::string_view line, std::string_view name) -> std::string { - const size_t at = line.find(name); - BOOST_REQUIRE(at != std::string_view::npos); - const size_t start = at + name.size(); - const size_t end = line.find_first_of(" \n", start); - return std::string(line.substr(start, end - start)); -} - -} // namespace - -BOOST_AUTO_TEST_CASE(cpu_topology_mask_popcount_and_union) { +BOOST_AUTO_TEST_CASE(cpu_topology_summarize_masks) { constexpr size_t kWords = partition::kAffinityMaskWords; - const auto empty = mask_of({}); - BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(empty.data(), kWords), 0U); - BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(nullptr, kWords), 0U); - - const auto spread = mask_of({0, 63, 64, 4095}); - BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(spread.data(), kWords), 4U); - - // Two ranks holding 16 each and two ranks sharing the same 16 differ in exactly this number. - const auto two_private = packed_masks({{0, 1}, {2, 3}}, kWords); - std::vector u(kWords, 0); - partition::cpu_mask_union(u.data(), two_private.data(), 2, kWords); - BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(u.data(), kWords), 4U); - - const auto two_shared = packed_masks({{0, 1}, {0, 1}}, kWords); - partition::cpu_mask_union(u.data(), two_shared.data(), 2, kWords); - BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(u.data(), kWords), 2U); - - // The destination is cleared, not accumulated into: a reused buffer must not report the old CPUs. - const auto lone = packed_masks({{9}}, kWords); - partition::cpu_mask_union(u.data(), lone.data(), 1, kWords); - BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(u.data(), kWords), 1U); - - // n == 0 is a cleared destination, not untouched memory. - partition::cpu_mask_union(u.data(), two_private.data(), 0, kWords); - BOOST_CHECK_EQUAL(partition::cpu_mask_popcount(u.data(), kWords), 0U); -} - -BOOST_AUTO_TEST_CASE(cpu_topology_mask_ranges_formatting) { - constexpr size_t kWords = partition::kAffinityMaskWords; - - // An empty mask is a WORD: a blank value in a KEY=value log line reads as a truncated line. - BOOST_CHECK_EQUAL(partition::cpu_mask_ranges(mask_of({}).data(), kWords), "none"); - BOOST_CHECK_EQUAL(partition::cpu_mask_ranges(nullptr, kWords), "none"); - - // A single CPU is the bare id, not "7-7". - BOOST_CHECK_EQUAL(partition::cpu_mask_ranges(mask_of({7}).data(), kWords), "7"); - - // The shape a correctly bound rank produces: one contiguous run. - std::vector block; - for (size_t i = 16; i < 32; ++i) { - block.push_back(i); - } - BOOST_CHECK_EQUAL(partition::cpu_mask_ranges(mask_of(block).data(), kWords), "16-31"); - - // A run crossing a 64-bit word boundary is ONE run: a per-word loop would split this into "63,64". - BOOST_CHECK_EQUAL(partition::cpu_mask_ranges(mask_of({63, 64}).data(), kWords), "63-64"); - BOOST_CHECK_EQUAL(partition::cpu_mask_ranges(mask_of({0, 1, 64, 65, 130}).data(), kWords), "0-1,64-65,130"); - - // Truncation is stated, never silent: a cut list that looked complete would be read as a smaller machine. - std::vector sparse; - for (size_t i = 0; i < partition::kMaxCpuRanges + 8; ++i) { - sparse.push_back(i * 2); // isolated bits ⇒ one run each - } - const auto truncated = partition::cpu_mask_ranges(mask_of(sparse).data(), kWords); - BOOST_CHECK_EQUAL(truncated.substr(truncated.size() - 3), ",+8"); - BOOST_CHECK_EQUAL(count(truncated, ","), partition::kMaxCpuRanges); - BOOST_CHECK_EQUAL(truncated.substr(0, 6), "0,2,4,"); + // Two ranks holding one CPU each and two ranks sharing the same CPUs differ in exactly node_cpus. + const auto private_ = packed_masks({{0, 1}, {2, 3}}, kWords); + const auto priv = partition::summarize_masks(private_.data(), 2, kWords, 0); + BOOST_REQUIRE(priv.has_value()); + BOOST_CHECK_EQUAL(priv->cpus, 2U); + BOOST_CHECK_EQUAL(priv->node_cpus, 4U); + BOOST_CHECK_EQUAL(priv->cpu_list, "0-3"); + + const auto shared = packed_masks({{0, 1}, {0, 1}}, kWords); + const auto shd = partition::summarize_masks(shared.data(), 2, kWords, 1); + BOOST_REQUIRE(shd.has_value()); + BOOST_CHECK_EQUAL(shd->cpus, 2U); + BOOST_CHECK_EQUAL(shd->node_cpus, 2U); + + // A run crossing a 64-bit word boundary is ONE run: a per-word loop would print "63,64". + const auto cross = packed_masks({{63, 64}}, kWords); + BOOST_CHECK_EQUAL(partition::summarize_masks(cross.data(), 1, kWords, 0).value().cpu_list, "63-64"); + + // No summary rather than a plausible zero: an all-zero row is a mask that did not fit the window. + const auto with_empty = packed_masks({{0, 1}, {}}, kWords); + BOOST_CHECK(!partition::summarize_masks(with_empty.data(), 2, kWords, 0).has_value()); + BOOST_CHECK(!partition::summarize_masks(nullptr, 1, kWords, 0).has_value()); + BOOST_CHECK(!partition::summarize_masks(private_.data(), 2, kWords, 2).has_value()); // self out of range } -// The flag is a parameter so this is reachable: monoprop_COMMPLACE is parsed once per process and -// cached, so a binary not launched with it set could not otherwise reach the emitting path. +// The formatter returns a string rather than writing one, so this is reachable in a binary launched +// without monoprop_COMMPLACE set -- the knob gates only PartitionGroup's write. BOOST_AUTO_TEST_CASE(cpu_topology_place_line_reports_every_field) { - const CaptureFile cap; - partition::PlacementReport rep; - rep.mpi_rank = 5; - rep.node_rank = 2; - rep.node_size = 8; - rep.masks = "private"; - rep.cpus = 16; - rep.node_cpus = 128; - rep.node_cpu_list = "0-127"; - - BOOST_CHECK_EQUAL(partition::emit_place_line(cap.stream(), true, rep), 1); - const auto text = cap.text(); - BOOST_CHECK_EQUAL(count(text, "COMMPLACE"), 1U); - BOOST_CHECK_EQUAL(count(text, "\n"), 1U); // terminated, so a second line cannot merge into it - BOOST_CHECK_EQUAL(field(text, "rank="), "5"); - BOOST_CHECK_EQUAL(field(text, "node_rank="), "2"); - BOOST_CHECK_EQUAL(field(text, "node_size="), "8"); - BOOST_CHECK_EQUAL(field(text, "masks="), "private"); - BOOST_CHECK_EQUAL(field(text, "cpus="), "16"); - BOOST_CHECK_EQUAL(field(text, "node_cpus="), "128"); - BOOST_CHECK_EQUAL(field(text, "cpu_list="), "0-127"); -} - -// The other half of the contract: "the knob was off" and "the instrument never fired" must be the -// same observation only when the knob really is off. -BOOST_AUTO_TEST_CASE(cpu_topology_place_line_is_gated) { - const CaptureFile cap; - const partition::PlacementReport rep; - BOOST_CHECK_EQUAL(partition::emit_place_line(cap.stream(), false, rep), 0); - BOOST_CHECK(cap.text().empty()); + const partition::MaskSummary sum{.cpus = 16, .node_cpus = 128, .cpu_list = "0-127"}; + // Whole line, not field lookups: a reordered or unterminated line has to fail too. + BOOST_CHECK_EQUAL(partition::format_place_line(5, 2, 8, "private", sum), + "COMMPLACE rank=5 node_rank=2 node_size=8 masks=private cpus=16 node_cpus=128 " + "cpu_list=0-127\n"); } -// A default report is the "could not classify" state and must SAY so rather than print a plausible zero. -BOOST_AUTO_TEST_CASE(cpu_topology_place_line_default_is_unknown_not_a_verdict) { - const CaptureFile cap; - const partition::PlacementReport rep; - BOOST_CHECK_EQUAL(partition::emit_place_line(cap.stream(), true, rep), 1); - const auto text = cap.text(); - BOOST_CHECK_EQUAL(field(text, "masks="), "unknown"); - BOOST_CHECK_EQUAL(field(text, "cpus="), "0"); - BOOST_CHECK_EQUAL(field(text, "node_size="), "1"); - BOOST_CHECK_EQUAL(field(text, "cpu_list="), "none"); // an unset list is still a word +// The state summarize_masks refuses to classify must SAY unknown rather than print a plausible zero. +BOOST_AUTO_TEST_CASE(cpu_topology_place_line_unknown_is_not_a_verdict) { + BOOST_CHECK_EQUAL(partition::format_place_line(0, 0, 1, "unknown", partition::MaskSummary{}), + "COMMPLACE rank=0 node_rank=0 node_size=1 masks=unknown cpus=0 node_cpus=0 " + "cpu_list=none\n"); } diff --git a/cpp/tests/env_config_tests.cpp b/cpp/tests/env_config_tests.cpp index 66118a30..e5168161 100644 --- a/cpp/tests/env_config_tests.cpp +++ b/cpp/tests/env_config_tests.cpp @@ -18,35 +18,34 @@ #include "monoprop/detail/EnvConfig.h" -using monoprop::config::detail::parse_env_flag; +using monoprop::config::detail::parse_flag; using monoprop::config::detail::parse_positive_int; -BOOST_AUTO_TEST_CASE(env_config_parse_env_flag_unset_and_empty_are_off) { - BOOST_CHECK_EQUAL(parse_env_flag(nullptr), false); - BOOST_CHECK_EQUAL(parse_env_flag(""), false); +BOOST_AUTO_TEST_CASE(env_config_parse_flag_default_when_unset_or_empty) { + BOOST_CHECK_EQUAL(parse_flag(nullptr, true), true); + BOOST_CHECK_EQUAL(parse_flag(nullptr, false), false); + BOOST_CHECK_EQUAL(parse_flag("", true), true); + BOOST_CHECK_EQUAL(parse_flag("", false), false); } -BOOST_AUTO_TEST_CASE(env_config_parse_env_flag_falsey_words_in_any_case) { - // `off` and `OFF` are the cases the deleted first-character parser read as ON. +BOOST_AUTO_TEST_CASE(env_config_parse_flag_falsey_words_in_any_case) { + // `off`, `OFF` and `Off` are the cases the old first-character parser read as ON. for (const char *v : {"0", "false", "FALSE", "False", "no", "NO", "No", "off", "OFF", "Off"}) { - BOOST_CHECK_MESSAGE(parse_env_flag(v) == false, v); + BOOST_CHECK_MESSAGE(parse_flag(v, true) == false, v); } } -BOOST_AUTO_TEST_CASE(env_config_parse_env_flag_truthy_words) { +BOOST_AUTO_TEST_CASE(env_config_parse_flag_truthy_words) { for (const char *v : {"1", "true", "TRUE", "yes", "on", "ON", "anything"}) { - BOOST_CHECK_MESSAGE(parse_env_flag(v), v); + BOOST_CHECK_MESSAGE(parse_flag(v, false), v); } } -BOOST_AUTO_TEST_CASE(env_config_parse_env_flag_compares_whole_words) { - // A first-character parser calls every one of these off; a whole-word one calls them all on. - for (const char *v : {"offbeat", "november", "0abc", "nope", "falsey", "f", "n"}) { - BOOST_CHECK_MESSAGE(parse_env_flag(v), v); +BOOST_AUTO_TEST_CASE(env_config_parse_flag_compares_whole_words) { + // Whole words, so a falsey word's prefix, extension and initial are all ON. `f` and `n` were falsey. + for (const char *v : {"offbeat", "november", "0abc", "nope", "falsey", "f", "n", "of", "fals"}) { + BOOST_CHECK_MESSAGE(parse_flag(v, false), v); } - // The converse: a prefix of a falsey word is not that word either. - BOOST_CHECK_EQUAL(parse_env_flag("of"), true); - BOOST_CHECK_EQUAL(parse_env_flag("fals"), true); } BOOST_AUTO_TEST_CASE(env_config_parse_positive_int_null_and_malformed) { diff --git a/docs/content/docs/features/parallelism.mdx b/docs/content/docs/features/parallelism.mdx index 7bc98c34..70d89d9c 100644 --- a/docs/content/docs/features/parallelism.mdx +++ b/docs/content/docs/features/parallelism.mdx @@ -29,7 +29,10 @@ 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_COMMPLACE` | `off` | Report-only. Writes one `COMMPLACE` line per rank to stderr when the propagator is built, naming the CPUs the launcher gave that rank and whether co-located ranks were handed disjoint masks. `0`, `false`, `no` and `off` (whole word, any case) disable it; any other non-empty value enables it. It changes no placement decision. | +| `monoprop_COMMPLACE` | `off` | Report-only. One `COMMPLACE` line per rank on stderr when the propagator is built, naming the CPUs the launcher gave that rank and whether co-located ranks got disjoint masks. Changes no placement decision. | + +Boolean variables are off for `0`, `false`, `no` or `off` (whole word, any case) +and on for any other non-empty value. Pinning each partition to a core is not configurable: leaving placement to the launcher measured `propagate[hubbard]` 2.90x slower, so the disabled arm is gone. From 82c7496decba7f4054969b0ac7a95694626ac65c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Tue, 25 Aug 2026 15:27:32 +0200 Subject: [PATCH 3/4] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Roberto Di Remigio Eikås Signed-off-by: Roberto Di Remigio Eikås --- docs/content/docs/features/parallelism.mdx | 3 --- 1 file changed, 3 deletions(-) diff --git a/docs/content/docs/features/parallelism.mdx b/docs/content/docs/features/parallelism.mdx index 70d89d9c..91e72e98 100644 --- a/docs/content/docs/features/parallelism.mdx +++ b/docs/content/docs/features/parallelism.mdx @@ -34,9 +34,6 @@ whose partner lives in another partition are resolved through a per-gate exchang Boolean variables are off for `0`, `false`, `no` or `off` (whole word, any case) and on for any other non-empty value. -Pinning each partition to a core is not configurable: leaving placement to the -launcher measured `propagate[hubbard]` 2.90x slower, so the disabled arm is gone. - ```bash # Run 8 partitions instead of one-per-core: export monoprop_NUM_THREADS=8 From 928d31c4a800ab3d2ba16a973107285ff9346786 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Tue, 25 Aug 2026 15:01:18 +0100 Subject: [PATCH 4/4] =?UTF-8?q?refactor(partition):=20=F0=9F=94=8A=20repor?= =?UTF-8?q?t=20placement=20unconditionally,=20and=20fix=20the=20CpuTopolog?= =?UTF-8?q?y=20lints?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review asked for both. The COMMPLACE line goes to stderr, so the reader can redirect it: the `monoprop_COMMPLACE` knob bought nothing and is gone, and with it the last boolean env var, so `parse_flag`/`iequals` and their tests go too. CpuTopology.cpp lints, clang-tidy and SonarQube, pre-existing ones included: braced return, three `auto *const topo`, two misplaced-const hwloc handles, two more on the summarize_masks bitmaps, designated PhysicalCore init, a signed comparison, and cpp:S886 -- the row loop's stop condition read `ok`, which its own body writes; it now tests the counter and breaks. Co-Authored-By: Claude Opus 5 (1M context) --- cpp/monoprop/detail/EnvConfig.h | 22 ------------- cpp/monoprop/detail/partition/CpuTopology.cpp | 33 +++++++++++-------- cpp/monoprop/detail/partition/CpuTopology.h | 8 ++--- .../detail/partition/PartitionGroup.h | 4 --- cpp/tests/cpu_topology_tests.cpp | 3 +- cpp/tests/env_config_tests.cpp | 30 +---------------- docs/content/docs/features/parallelism.mdx | 23 ++++++++++--- 7 files changed, 44 insertions(+), 79 deletions(-) diff --git a/cpp/monoprop/detail/EnvConfig.h b/cpp/monoprop/detail/EnvConfig.h index e3d6c016..30120417 100644 --- a/cpp/monoprop/detail/EnvConfig.h +++ b/cpp/monoprop/detail/EnvConfig.h @@ -22,32 +22,12 @@ // pulled into hot-path headers. // // monoprop_NUM_THREADS positive int (1..1e6), else ignored → num_threads -// monoprop_COMMPLACE bool, default OFF; one COMMPLACE line per rank on stderr → commplace // monoprop_PARTITIONS int N | "auto" | "off"; parsed where it is used (resolve_partition_count_) namespace monoprop::config { namespace detail { -// Case-insensitive whole-string compare; `lower` must already be lowercase. -inline auto iequals(const char *value, const char *lower) -> bool { - for (; *value != '\0' && *lower != '\0'; ++value, ++lower) { - const char c = (*value >= 'A' && *value <= 'Z') ? static_cast(*value - 'A' + 'a') : *value; - if (c != *lower) { - return false; - } - } - return *value == *lower; -} - -inline auto parse_flag(const char *value, bool default_value) -> bool { - if (value == nullptr || value[0] == '\0') { - return default_value; - } - // Whole words: matching only the first character read `off` and `OFF` as ON. - return !(iequals(value, "0") || iequals(value, "false") || iequals(value, "no") || iequals(value, "off")); -} - inline auto parse_positive_int(const char *text) -> std::optional { if (text == nullptr) { return std::nullopt; @@ -67,7 +47,6 @@ inline auto parse_positive_int(const char *text) -> std::optional { struct Settings { std::optional num_threads; - bool commplace = false; }; // Parse the environment once; the Settings are cached and shared across TUs. @@ -75,7 +54,6 @@ inline auto get() -> const Settings & { static const Settings settings = [] { Settings s; s.num_threads = detail::parse_positive_int(std::getenv("monoprop_NUM_THREADS")); - s.commplace = detail::parse_flag(std::getenv("monoprop_COMMPLACE"), false); return s; }(); return settings; diff --git a/cpp/monoprop/detail/partition/CpuTopology.cpp b/cpp/monoprop/detail/partition/CpuTopology.cpp index abc34255..84ef0cdd 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.cpp +++ b/cpp/monoprop/detail/partition/CpuTopology.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -147,8 +148,8 @@ auto placement_order(const std::vector &cores, size_t n, size_t gr if (offset + n > order.size()) { return {}; } - return std::vector(order.begin() + static_cast(offset), - order.begin() + static_cast(offset + n)); + return {order.begin() + static_cast(offset), + order.begin() + static_cast(offset + n)}; } } // namespace topo_detail @@ -156,7 +157,7 @@ auto placement_order(const std::vector &cores, size_t n, size_t gr /* ── enumerate_physical_cores ──────────────────────────────────────────────── */ auto enumerate_physical_cores() -> std::vector { - const auto topo = get_topology(); + auto *const topo = get_topology(); if (!topo) { return {}; } @@ -178,7 +179,7 @@ auto enumerate_physical_cores() -> std::vector { const unsigned num_cores = hwloc_get_nbobjs_by_depth(topo, core_depth); for (unsigned i = 0; i < num_cores; ++i) { - const hwloc_obj_t core = hwloc_get_obj_by_depth(topo, core_depth, i); + auto *const core = hwloc_get_obj_by_depth(topo, core_depth, i); if (!core || !core->cpuset) { continue; } @@ -205,7 +206,7 @@ auto enumerate_physical_cores() -> std::vector { * receive their own singleton domain so the placement algorithm can still spread across * whatever structure the topology does have. */ int domain; - const hwloc_obj_t l3 = hwloc_get_ancestor_obj_by_type(topo, HWLOC_OBJ_L3CACHE, core); + auto *const l3 = hwloc_get_ancestor_obj_by_type(topo, HWLOC_OBJ_L3CACHE, core); if (l3) { const auto [it, inserted] = l3_domain_map.emplace(l3->logical_index, next_domain_id); if (inserted) { @@ -217,7 +218,7 @@ auto enumerate_physical_cores() -> std::vector { domain = next_domain_id++; } - cores.push_back(PhysicalCore{rep, domain}); + cores.push_back(PhysicalCore{.cpu = rep, .l3_domain = domain}); } hwloc_bitmap_free(allowed); @@ -231,7 +232,7 @@ auto affinity_mask_words(uint64_t *out, size_t nwords) -> bool { return false; } std::fill_n(out, nwords, uint64_t{0}); - const auto topo = get_topology(); + auto *const topo = get_topology(); if (!topo) { return false; } @@ -288,8 +289,8 @@ auto summarize_masks(const uint64_t *masks, size_t n, size_t words, size_t self) if (masks == nullptr || n == 0 || words == 0 || self >= n) { return std::nullopt; } - const hwloc_bitmap_t row = hwloc_bitmap_alloc(); - const hwloc_bitmap_t all = hwloc_bitmap_alloc(); + auto *const row = hwloc_bitmap_alloc(); + auto *const all = hwloc_bitmap_alloc(); if (row == nullptr || all == nullptr) { hwloc_bitmap_free(row); hwloc_bitmap_free(all); @@ -297,14 +298,18 @@ auto summarize_masks(const uint64_t *masks, size_t n, size_t words, size_t self) } MaskSummary out; bool ok = true; - for (size_t r = 0; r < n && ok; ++r) { + for (size_t r = 0; r < n; ++r) { hwloc_bitmap_zero(row); for (size_t w = 0; w < words; ++w) { hwloc_bitmap_set_ith_ulong(row, static_cast(w), masks[(r * words) + w]); } - ok = hwloc_bitmap_weight(row) > 0; // an all-zero row is a mask that did not fit the exchange window + const int weight = hwloc_bitmap_weight(row); + if (weight <= 0) { // an all-zero row is a mask that did not fit the exchange window + ok = false; + break; + } if (r == self) { - out.cpus = static_cast(hwloc_bitmap_weight(row)); + out.cpus = static_cast(weight); } hwloc_bitmap_or(all, all, row); } @@ -315,7 +320,7 @@ auto summarize_masks(const uint64_t *masks, size_t n, size_t words, size_t self) out.cpu_list = text.data(); // Truncation is stated, never silent: a cut list read as complete is a smaller machine. Cut // back to the last whole range first, so the marker never follows a half-written CPU id. - if (need >= static_cast(text.size())) { + if (std::cmp_greater_equal(need, text.size())) { const size_t last = out.cpu_list.rfind(','); out.cpu_list.resize(last == std::string::npos ? 0 : last + 1); out.cpu_list += "+"; @@ -376,7 +381,7 @@ auto pin_this_thread(const CpuSet &set) -> void { if (set.pu < 0) { return; } - const auto topo = get_topology(); + auto *const topo = get_topology(); if (!topo) { return; } diff --git a/cpp/monoprop/detail/partition/CpuTopology.h b/cpp/monoprop/detail/partition/CpuTopology.h index 6bd6911d..e862fb51 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.h +++ b/cpp/monoprop/detail/partition/CpuTopology.h @@ -115,15 +115,15 @@ struct MaskSummary { [[nodiscard]] auto summarize_masks(const uint64_t *masks, size_t n, size_t words, size_t self) -> std::optional; -/* COMMPLACE (monoprop_COMMPLACE): a rank seeing 16 of a host's 128 CPUs is equally "my own 16" and - * "eight of us share these 16", and only the co-located ranks' masks separate them, so the exchange - * PartitionGroup already runs to place also reports. Report-only; nothing branches on the line. */ +/* COMMPLACE: a rank seeing 16 of a host's 128 CPUs is equally "my own 16" and "eight of us share + * these 16", and only the co-located ranks' masks separate them, so the exchange PartitionGroup + * already runs to place also reports. Report-only, unconditional; nothing branches on the line. */ /*! @brief One newline-terminated COMMPLACE line naming what the launcher handed this rank. * @param verdict how the co-located masks relate: "private" (pairwise disjoint), "shared" (two ranks * can land on one CPU), "alone" (the only rank on its host, which is NOT "private"), or * "unknown" (a mask did not fit the exchanged window, so no verdict is sound). - * Returned, not written, so a binary launched without the knob set still reaches the formatting. + * Returned, not written, so the formatting is testable without a live rank. */ [[nodiscard]] auto format_place_line(int mpi_rank, int node_rank, diff --git a/cpp/monoprop/detail/partition/PartitionGroup.h b/cpp/monoprop/detail/partition/PartitionGroup.h index d7a13590..89ce2f93 100644 --- a/cpp/monoprop/detail/partition/PartitionGroup.h +++ b/cpp/monoprop/detail/partition/PartitionGroup.h @@ -27,7 +27,6 @@ #include #include -#include "monoprop/detail/EnvConfig.h" // config::get().commplace -- gates the COMMPLACE line only #include "monoprop/detail/mpi/Comm.h" #include "monoprop/detail/mpi/MPICompat.h" // mpi::size for the transport choice #include "monoprop/detail/mpi/ShmComm.h" @@ -204,9 +203,6 @@ class PartitionGroup { * evidence that a multi-rank launcher did the right thing. Reached only from the primary ctor, so * a clone does not re-emit -- the mask belongs to the process, not the object. */ auto report_placement_(const uint64_t *masks, size_t peers, const char *verdict) -> void { - if (!config::get().commplace) { - return; - } constexpr size_t kWords = monoprop::detail::partition::kAffinityMaskWords; std::array own{}; if (masks == nullptr && affinity_mask_words(own.data(), kWords)) { diff --git a/cpp/tests/cpu_topology_tests.cpp b/cpp/tests/cpu_topology_tests.cpp index a129be56..5b0334f9 100644 --- a/cpp/tests/cpu_topology_tests.cpp +++ b/cpp/tests/cpu_topology_tests.cpp @@ -429,8 +429,7 @@ BOOST_AUTO_TEST_CASE(cpu_topology_summarize_masks) { BOOST_CHECK(!partition::summarize_masks(private_.data(), 2, kWords, 2).has_value()); // self out of range } -// The formatter returns a string rather than writing one, so this is reachable in a binary launched -// without monoprop_COMMPLACE set -- the knob gates only PartitionGroup's write. +// The formatter returns a string rather than writing one, so the line is testable without a live rank. BOOST_AUTO_TEST_CASE(cpu_topology_place_line_reports_every_field) { const partition::MaskSummary sum{.cpus = 16, .node_cpus = 128, .cpu_list = "0-127"}; // Whole line, not field lookups: a reordered or unterminated line has to fail too. diff --git a/cpp/tests/env_config_tests.cpp b/cpp/tests/env_config_tests.cpp index e5168161..fe46afe7 100644 --- a/cpp/tests/env_config_tests.cpp +++ b/cpp/tests/env_config_tests.cpp @@ -18,36 +18,8 @@ #include "monoprop/detail/EnvConfig.h" -using monoprop::config::detail::parse_flag; using monoprop::config::detail::parse_positive_int; -BOOST_AUTO_TEST_CASE(env_config_parse_flag_default_when_unset_or_empty) { - BOOST_CHECK_EQUAL(parse_flag(nullptr, true), true); - BOOST_CHECK_EQUAL(parse_flag(nullptr, false), false); - BOOST_CHECK_EQUAL(parse_flag("", true), true); - BOOST_CHECK_EQUAL(parse_flag("", false), false); -} - -BOOST_AUTO_TEST_CASE(env_config_parse_flag_falsey_words_in_any_case) { - // `off`, `OFF` and `Off` are the cases the old first-character parser read as ON. - for (const char *v : {"0", "false", "FALSE", "False", "no", "NO", "No", "off", "OFF", "Off"}) { - BOOST_CHECK_MESSAGE(parse_flag(v, true) == false, v); - } -} - -BOOST_AUTO_TEST_CASE(env_config_parse_flag_truthy_words) { - for (const char *v : {"1", "true", "TRUE", "yes", "on", "ON", "anything"}) { - BOOST_CHECK_MESSAGE(parse_flag(v, false), v); - } -} - -BOOST_AUTO_TEST_CASE(env_config_parse_flag_compares_whole_words) { - // Whole words, so a falsey word's prefix, extension and initial are all ON. `f` and `n` were falsey. - for (const char *v : {"offbeat", "november", "0abc", "nope", "falsey", "f", "n", "of", "fals"}) { - BOOST_CHECK_MESSAGE(parse_flag(v, false), v); - } -} - BOOST_AUTO_TEST_CASE(env_config_parse_positive_int_null_and_malformed) { BOOST_CHECK(parse_positive_int(nullptr) == std::nullopt); BOOST_CHECK(parse_positive_int("") == std::nullopt); @@ -70,5 +42,5 @@ BOOST_AUTO_TEST_CASE(env_config_settings_cached_singleton) { const auto &b = monoprop::config::get(); BOOST_CHECK_EQUAL(&a, &b); // Touch a field so the Settings aggregate is actually read. - BOOST_CHECK(a.commplace == true || a.commplace == false); + BOOST_CHECK(a.num_threads == std::nullopt || *a.num_threads >= 1); } diff --git a/docs/content/docs/features/parallelism.mdx b/docs/content/docs/features/parallelism.mdx index 91e72e98..e9e56ed5 100644 --- a/docs/content/docs/features/parallelism.mdx +++ b/docs/content/docs/features/parallelism.mdx @@ -29,16 +29,31 @@ 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_COMMPLACE` | `off` | Report-only. One `COMMPLACE` line per rank on stderr when the propagator is built, naming the CPUs the launcher gave that rank and whether co-located ranks got disjoint masks. Changes no placement decision. | - -Boolean variables are off for `0`, `false`, `no` or `off` (whole word, any case) -and on for any other non-empty value. ```bash # Run 8 partitions instead of one-per-core: export monoprop_NUM_THREADS=8 ``` +## Placement report + +Building a propagator writes one `COMMPLACE` line per rank to stderr, naming the +CPUs the launcher gave that rank and whether co-located ranks got disjoint masks. +It is report-only — no placement decision reads it — and there is no knob: redirect +stderr to drop it. + +```text +COMMPLACE rank=0 node_rank=0 node_size=2 masks=private cpus=64 node_cpus=128 cpu_list=0-63 +``` + +`masks` is `private` when the co-located ranks' affinity masks are pairwise +disjoint, `shared` when two ranks can land on the same CPU, `alone` when this rank +is the only one on its host (which is *not* evidence a multi-rank launcher bound +correctly), and `unknown` when a mask did not fit the exchanged window. + +Pinning each partition to a core is not configurable: leaving placement to the +launcher measured `propagate[hubbard]` 2.90x slower, so the disabled arm is gone. + ## MPI distribution (multi-node) MPI partitions the operator and graph across ranks, composing with per-rank