From d7ff6003ef2086a9aca81def92f377f23a626987 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Tue, 25 Aug 2026 15:36:05 +0100 Subject: [PATCH 1/2] =?UTF-8?q?fix(partition):=20=F0=9F=94=87=20collapse?= =?UTF-8?q?=20identical=20COMMPLACE=20repeats?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Making the line unconditional exposed its volume: the Python suite emitted 437 COMMPLACE lines in one 585-test serial run, and all 437 were the same string. One process has one placement, so the repeats are noise, not data. `place_line_is_new` compares the whole LINE rather than setting a once-flag: a process that builds an `alone` propagator before an MPI one has two placements and both are worth saying. Non-template so the guard is process-wide rather than one per NumModes instantiation, and returned rather than written so the collapse is testable without capturing stderr. Co-Authored-By: Claude Opus 5 (1M context) --- cpp/monoprop/detail/partition/CpuTopology.cpp | 11 +++++++++++ cpp/monoprop/detail/partition/CpuTopology.h | 7 +++++++ cpp/monoprop/detail/partition/PartitionGroup.h | 18 ++++++++++-------- cpp/tests/cpu_topology_tests.cpp | 13 +++++++++++++ 4 files changed, 41 insertions(+), 8 deletions(-) diff --git a/cpp/monoprop/detail/partition/CpuTopology.cpp b/cpp/monoprop/detail/partition/CpuTopology.cpp index 84ef0cdd..ed5cb73f 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.cpp +++ b/cpp/monoprop/detail/partition/CpuTopology.cpp @@ -343,6 +343,17 @@ auto format_place_line(int mpi_rank, int node_rank, int node_size, const char *v summary.cpu_list); } +auto place_line_is_new(const std::string &line) -> bool { + static std::mutex mu; + static std::string last; + const std::lock_guard lock(mu); + if (line == last) { + return false; + } + last = line; + return true; +} + /* ── partition_cpusets ─────────────────────────────────────────────────────── */ auto partition_cpusets(size_t n, size_t group_index, size_t group_count, NodeMask mask) -> std::vector { diff --git a/cpp/monoprop/detail/partition/CpuTopology.h b/cpp/monoprop/detail/partition/CpuTopology.h index e862fb51..cb6e1ce3 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.h +++ b/cpp/monoprop/detail/partition/CpuTopology.h @@ -131,6 +131,13 @@ struct MaskSummary { const char *verdict, const MaskSummary &summary) -> std::string; +/*! @brief True the first time @p line is seen, and whenever it differs from the previous call. + * One process has one placement, so a propagator built 437 times reports it 437 times identically; + * the whole line is compared rather than a once-flag set, because a process that builds an `alone` + * propagator before an MPI one has two placements and both are worth saying. Process-wide, locked. + */ +[[nodiscard]] auto place_line_is_new(const std::string &line) -> bool; + //! 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 89ce2f93..2a76970f 100644 --- a/cpp/monoprop/detail/partition/PartitionGroup.h +++ b/cpp/monoprop/detail/partition/PartitionGroup.h @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -211,14 +212,15 @@ class PartitionGroup { } // 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); + const std::string line = format_place_line(mpi::rank(parent_), + node_rank_, + node_size_, + sum ? verdict : "unknown", + sum.value_or(MaskSummary{})); + if (place_line_is_new(line)) { + std::fputs(line.c_str(), stderr); + std::fflush(stderr); + } } auto make_transport_() -> void { diff --git a/cpp/tests/cpu_topology_tests.cpp b/cpp/tests/cpu_topology_tests.cpp index 5b0334f9..782fe82c 100644 --- a/cpp/tests/cpu_topology_tests.cpp +++ b/cpp/tests/cpu_topology_tests.cpp @@ -438,6 +438,19 @@ BOOST_AUTO_TEST_CASE(cpu_topology_place_line_reports_every_field) { "cpu_list=0-127\n"); } +// 437 identical lines in one 585-test run is noise; a CHANGED line still has to get through. +BOOST_AUTO_TEST_CASE(cpu_topology_place_line_repeats_are_collapsed) { + // Distinctive, so a real COMMPLACE line emitted by some other case cannot be the previous value. + const std::string a = "COMMPLACE test-a\n"; + const std::string b = "COMMPLACE test-b\n"; + BOOST_CHECK(partition::place_line_is_new(a)); + BOOST_CHECK(!partition::place_line_is_new(a)); + BOOST_CHECK(!partition::place_line_is_new(a)); + BOOST_CHECK(partition::place_line_is_new(b)); // a different placement is not a repeat + BOOST_CHECK(!partition::place_line_is_new(b)); + BOOST_CHECK(partition::place_line_is_new(a)); // and neither is coming back to the first one +} + // 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{}), From f87b662084f616dfa37db3cbd7547b98aa8178af Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Tue, 25 Aug 2026 16:05:21 +0100 Subject: [PATCH 2/2] =?UTF-8?q?refactor(partition):=20=F0=9F=8E=A8=20take?= =?UTF-8?q?=20the=20COMMPLACE=20line=20by=20string=5Fview?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SonarQube cpp:S6009 on the new signature. The function only compares and assigns, so it never needed to own or bind a std::string. Co-Authored-By: Claude Opus 5 (1M context) --- cpp/monoprop/detail/partition/CpuTopology.cpp | 3 ++- cpp/monoprop/detail/partition/CpuTopology.h | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/cpp/monoprop/detail/partition/CpuTopology.cpp b/cpp/monoprop/detail/partition/CpuTopology.cpp index ed5cb73f..81f0be4d 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 @@ -343,7 +344,7 @@ auto format_place_line(int mpi_rank, int node_rank, int node_size, const char *v summary.cpu_list); } -auto place_line_is_new(const std::string &line) -> bool { +auto place_line_is_new(std::string_view line) -> bool { static std::mutex mu; static std::string last; const std::lock_guard lock(mu); diff --git a/cpp/monoprop/detail/partition/CpuTopology.h b/cpp/monoprop/detail/partition/CpuTopology.h index cb6e1ce3..40a0583a 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.h +++ b/cpp/monoprop/detail/partition/CpuTopology.h @@ -30,6 +30,7 @@ #include #include #include +#include #include namespace monoprop::detail::partition { @@ -136,7 +137,7 @@ struct MaskSummary { * the whole line is compared rather than a once-flag set, because a process that builds an `alone` * propagator before an MPI one has two placements and both are worth saying. Process-wide, locked. */ -[[nodiscard]] auto place_line_is_new(const std::string &line) -> bool; +[[nodiscard]] auto place_line_is_new(std::string_view line) -> bool; //! 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 };