From 69d58ddcc65690151b40c23fad845c5c1c43aa55 Mon Sep 17 00:00:00 2001 From: Debra Date: Thu, 27 Aug 2026 17:59:11 +0800 Subject: [PATCH 1/6] bench: sweep the non-blocking path across capacity, on one clock Adds a try_ counterpart to the blocking throughput harness and sweeps it over capacity for the three cq queues, plus the retry diagnostics needed to read the result. Failed attempts are retried inside the timed iteration, so they cost time but never count as transferred work: items/s stays the same unit as the blocking rows. What produced those failures is reported separately as retries/op, with push_retries/push and pop_retries/pop splitting it by side -- the aggregate alone cannot say which side is under pressure, which is the thing a capacity sweep exists to expose. The retry loop yields after each failure. That is load-bearing, not politeness: without it a spinning side keeps barging the lock back from its counterpart, which then cannot make the progress that would let the spinner succeed. At capacity 1 or 2 every op depends on the counterpart, so one starvation episode dominates a whole run -- measured on an earlier draft, the reported rate at capacity 1 swung 396x across five identical invocations. What the sweep shows (10 repetitions, load ~4.2, items/s): capacity MutexQueue SpscQueue MpmcQueue Spsc/Mutex 1 5.5M 7.8M 6.7M 1.42x 2 8.8M 14.4M 13.5M 1.64x 8 24.6M 59.8M 47.9M 2.43x 64 19.6M 469.6M 179.6M 23.95x 1024 53.9M 580.9M 180.2M 10.78x The lock-free advantage is not a constant: it nearly vanishes at capacity 1 and only opens up once the ring is deep enough for a producer to run ahead. A shallow queue makes every op wait on its counterpart no matter how the waiting is implemented, so both designs converge on the cost of a cross-core handoff. retries/op tracks it exactly -- 1.07-1.31 at capacity 1, 0.001-0.005 at 1024. Quoting a single lock-free speedup without naming the depth it was measured at says very little. Also fixes a clock inconsistency this sweep would otherwise inherit: the five single_thread_roundtrip registrations set no UseRealTime(), so Google Benchmark normalised their rate counter by CPU time while every threaded row used wall time. The comment on their SetItemsProcessed claims the unit is kept "identical to the threaded benchmark so the rates compare directly" -- true of the item count, false of the clock. The sweep covers only the cq queues: moodycamel is unbounded and so has no capacity to vary, and the tbb adapter has no try_pop yet. Co-Authored-By: Claude Opus 5 (1M context) --- bench/CMakeLists.txt | 16 ++++++ bench/queue_bench.cpp | 108 +++++++++++++++++++++++++++++++++++ bench/try_operation.hpp | 43 ++++++++++++++ tests/CMakeLists.txt | 5 +- tests/try_operation_test.cpp | 23 ++++++++ 5 files changed, 194 insertions(+), 1 deletion(-) create mode 100644 bench/try_operation.hpp create mode 100644 tests/try_operation_test.cpp diff --git a/bench/CMakeLists.txt b/bench/CMakeLists.txt index 27cff11..4a69f5c 100644 --- a/bench/CMakeLists.txt +++ b/bench/CMakeLists.txt @@ -4,3 +4,19 @@ target_link_libraries(queue_bench PRIVATE cq::cq cq_warnings benchmark::benchmar # One-iteration smoke run; CI's Release job runs it via ctest. add_test(NAME bench_smoke COMMAND queue_bench --benchmark_min_time=1x) + +# The shallowest non-blocking run must report non-zero retry pressure, or the +# diagnostic has gone dead. Kept separate from bench_smoke so that test still +# gates the process exit code (PASS_REGULAR_EXPRESSION overrides that check). +# +# [^\n] keeps the match inside the _mean row: CTest's `.` crosses newlines, so +# without it the pattern can start on _mean and satisfy its tail from a later +# row. Asserting non-zero rather than a magnitude keeps the gate off the +# scheduler's back. +add_test(NAME bench_retry_pressure + COMMAND queue_bench + "--benchmark_filter=^MutexQueue/try_throughput/capacity:1/" + --benchmark_min_time=1000x + --benchmark_repetitions=10) +set_tests_properties(bench_retry_pressure PROPERTIES + PASS_REGULAR_EXPRESSION "_mean[^\n]*retries/op=[0-9.]*[1-9]") diff --git a/bench/queue_bench.cpp b/bench/queue_bench.cpp index 5940655..6441474 100644 --- a/bench/queue_bench.cpp +++ b/bench/queue_bench.cpp @@ -11,11 +11,21 @@ // iteration, so pushes and pops balance. items/s = total ops/s (a push and // its pop count as two). Spawn cost sits outside the timed region; add // --benchmark_repetitions=10 for a spread. +// +// The /try_throughput sweeps use the non-blocking API instead, retrying a +// failed attempt inside the same iteration. Failures therefore cost time but +// never count as transferred work, so items/s stays the same unit as the +// blocking rows, and the pressure that caused them is reported separately as +// retries/op. Sweeping capacity is the point: it is where a lock-free ring's +// advantage over the mutex should narrow, since a shallow queue makes every +// op wait on its counterpart no matter how the waiting is implemented. #include #include #include +#include "try_operation.hpp" + #include #include #include @@ -97,6 +107,22 @@ void teardown_queue(const benchmark::State& /*state*/) { shared_queue.reset(); } +// Same as setup_queue, but takes the capacity from the registered Arg so one +// benchmark can sweep it. +template +void setup_queue_at_capacity(const benchmark::State& state) { + const auto capacity = static_cast(state.range(0)); + shared_queue = std::make_unique(capacity); + // Half-full where the half exists: at capacity 1 it is 0, so that sweep + // point deliberately starts empty -- which is the retry pressure it is + // there to measure. + for (std::uint64_t i = 0; i < capacity / 2; ++i) { + if (!shared_queue->try_push(i)) { + std::abort(); // unreachable: fresh queue, stays below capacity + } + } +} + template void BM_QueueThroughput(benchmark::State& state) { const bool is_producer = state.thread_index() < state.threads() / 2; @@ -119,6 +145,42 @@ void BM_QueueThroughput(benchmark::State& state) { state.SetItemsProcessed(state.iterations() * state.threads()); } +// Non-blocking counterpart of BM_QueueThroughput. +template +void BM_QueueTryThroughput(benchmark::State& state) { + const bool is_producer = state.thread_index() < state.threads() / 2; + auto& queue = *shared_queue; + std::size_t retries = 0; + + if (is_producer) { + std::uint64_t item = 0; + for (auto _ : state) { + retries += cq::bench::count_failures_until_success([&] { return queue.try_push(item); }); + ++item; + } + } else { + std::uint64_t value = 0; + for (auto _ : state) { + retries += cq::bench::count_failures_until_success([&] { return queue.try_pop(value); }); + } + } + + state.SetItemsProcessed(state.iterations() * state.threads()); + // Counters sum across threads, and kAvgIterations divides by the iteration + // total over *all* threads. Producers and consumers each own half of that + // total, so doubling one side's retries before the divide yields that side's + // retries per its own op -- for any even producer/consumer split, not just + // the 1+1 these sweeps register. + const auto side_retries = 2.0 * static_cast(retries); + state.counters.emplace( + "push_retries/push", + benchmark::Counter(is_producer ? side_retries : 0.0, benchmark::Counter::kAvgIterations)); + state.counters.emplace("pop_retries/pop", benchmark::Counter(is_producer ? 0.0 : side_retries, + benchmark::Counter::kAvgIterations)); + state.counters.emplace("retries/op", benchmark::Counter(static_cast(retries), + benchmark::Counter::kAvgIterations)); +} + // Uncontended single-thread round trip: the queue's raw per-op cost with no // other thread in the picture. template @@ -229,23 +291,69 @@ BENCHMARK(BM_QueueThroughput) ->MinTime(kMinTimeSeconds) ->Name("MoodycamelQueue/throughput"); +// Non-blocking sweeps. Capacity is the independent variable here, not a +// tuning constant, and every value is a power of two because MpmcQueue masks +// its indices. Only the cq queues are swept: moodycamel is unbounded, so it +// has no capacity to vary, and tbb's adapter has no try_pop yet. +// NOLINTBEGIN(readability-magic-numbers) +BENCHMARK(BM_QueueTryThroughput) + ->Setup(setup_queue_at_capacity) + ->Teardown(teardown_queue) + ->ArgName("capacity") + ->ArgsProduct({{1, 2, 8, 64, 1024}}) + ->Threads(kSpscThreads) + ->UseRealTime() + ->MinTime(kMinTimeSeconds) + ->Name("MutexQueue/try_throughput"); + +BENCHMARK(BM_QueueTryThroughput) + ->Setup(setup_queue_at_capacity) + ->Teardown(teardown_queue) + ->ArgName("capacity") + ->ArgsProduct({{1, 2, 8, 64, 1024}}) + ->Threads(kSpscThreads) + ->UseRealTime() + ->MinTime(kMinTimeSeconds) + ->Name("SpscQueue/try_throughput"); + +BENCHMARK(BM_QueueTryThroughput) + ->Setup(setup_queue_at_capacity) + ->Teardown(teardown_queue) + ->ArgName("capacity") + ->ArgsProduct({{1, 2, 8, 64, 1024}}) + ->Threads(kSpscThreads) + ->UseRealTime() + ->MinTime(kMinTimeSeconds) + ->Name("MpmcQueue/try_throughput"); +// NOLINTEND(readability-magic-numbers) + +// UseRealTime on the round trips too: Google Benchmark divides a rate counter +// by whichever clock the benchmark selected, so without it these rows report +// items/s per CPU-second while every threaded row above is per wall-second -- +// not the same number, and not comparable, which is what the comment on +// SetItemsProcessed above claims they are. BENCHMARK(BM_QueuePushPopSingleThread) + ->UseRealTime() ->MinTime(kMinTimeSeconds) ->Name("MutexQueue/single_thread_roundtrip"); BENCHMARK(BM_QueuePushPopSingleThread) + ->UseRealTime() ->MinTime(kMinTimeSeconds) ->Name("SpscQueue/single_thread_roundtrip"); BENCHMARK(BM_QueuePushPopSingleThread) + ->UseRealTime() ->MinTime(kMinTimeSeconds) ->Name("MpmcQueue/single_thread_roundtrip"); BENCHMARK(BM_QueuePushPopSingleThread) + ->UseRealTime() ->MinTime(kMinTimeSeconds) ->Name("TbbBoundedQueue/single_thread_roundtrip"); BENCHMARK(BM_QueuePushPopSingleThread) + ->UseRealTime() ->MinTime(kMinTimeSeconds) ->Name("MoodycamelQueue/single_thread_roundtrip"); diff --git a/bench/try_operation.hpp b/bench/try_operation.hpp new file mode 100644 index 0000000..578f67e --- /dev/null +++ b/bench/try_operation.hpp @@ -0,0 +1,43 @@ +// Retry helper shared by the non-blocking throughput benchmarks. +#ifndef CQ_BENCH_TRY_OPERATION_HPP_ +#define CQ_BENCH_TRY_OPERATION_HPP_ + +#include +#include +#include +#include + +namespace cq::bench { + +// Retry until the operation succeeds, returning how many attempts failed. +// +// The yield is load-bearing, not politeness: without it a spinning side keeps +// barging the lock back from its counterpart, which then cannot make the +// progress that would let the spinner succeed. At capacity 1 or 2 every op +// depends on the counterpart, so one starvation episode dominates a whole run +// and the reported rate swings by orders of magnitude between invocations. +// +// The operation is taken by value, following the std:: algorithm convention: +// it is invoked repeatedly, so a forwarding reference would never actually be +// forwarded. Callers needing to observe its state changes capture by +// reference, as the benchmark call sites do. +// +// Deliberately not constrained with std::predicate: that subsumes +// std::regular_invocable, whose contract is equality-preserving invocation, +// and this helper exists precisely to call something whose answer changes +// between calls. It also constrains an rvalue F&&, while the body invokes an +// lvalue. +template + requires std::invocable && std::convertible_to, bool> +[[nodiscard]] std::size_t count_failures_until_success(Operation operation) { + std::size_t failures = 0; + while (!operation()) { + ++failures; + std::this_thread::yield(); + } + return failures; +} + +} // namespace cq::bench + +#endif // CQ_BENCH_TRY_OPERATION_HPP_ diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 560eb4c..c0a4137 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -2,8 +2,11 @@ add_executable(queue_tests mutex_queue_test.cpp queue_contract_test.cpp spsc_queue_test.cpp - stress_test.cpp) + stress_test.cpp + try_operation_test.cpp) target_link_libraries(queue_tests PRIVATE cq::cq cq_warnings cq_sanitizers GTest::gtest_main) +# The retry helper lives with the benchmarks that use it; the test reaches it. +target_include_directories(queue_tests PRIVATE ${PROJECT_SOURCE_DIR}/bench) include(GoogleTest) # PRE_TEST + a generous timeout: never run the (possibly TSan-instrumented) diff --git a/tests/try_operation_test.cpp b/tests/try_operation_test.cpp new file mode 100644 index 0000000..b795a48 --- /dev/null +++ b/tests/try_operation_test.cpp @@ -0,0 +1,23 @@ +#include "try_operation.hpp" + +#include + +#include + +namespace cq::bench { +namespace { + +TEST(TryOperation, CountsFailuresBeforeSuccess) { + std::size_t attempts = 0; + + const auto failures = count_failures_until_success([&] { + ++attempts; + return attempts == 3; + }); + + EXPECT_EQ(attempts, 3U); + EXPECT_EQ(failures, 2U); +} + +} // namespace +} // namespace cq::bench From e0082ace621fa106eb9e88ebdf527faa77b1ae1c Mon Sep 17 00:00:00 2001 From: Debra Date: Thu, 27 Aug 2026 19:00:06 +0800 Subject: [PATCH 2/6] bench: name the retry policy, because it moves the numbers more than the queues Review follow-ups on the sweep this branch added. The headline reading was wrong, and the code comments oversold one queue's behaviour as all three. The yield in count_failures_until_success was justified as stopping a spinner from barging the lock back from its counterpart. That mechanism exists only for MutexQueue, which takes a blocking lock_guard inside try_push/try_pop. SpscQueue and MpmcQueue have no lock to barge, so for them the yield is pure overhead. Measured at capacity 1: MutexQueue 0.58 M/s without -> 5.11 M/s with (yield buys 8.8x) SpscQueue 20.9 M/s without -> 7.84 M/s with (yield costs 2.7x) MpmcQueue 21.1 M/s without -> 6.84 M/s with (yield costs 3.1x) So the conclusion this sweep appeared to support -- that the lock-free advantage nearly vanishes at shallow depth -- is a property of the retry policy, not of the queues. Spsc/Mutex at capacity 1 is ~1.5x with the yield and ~36x without it. The yield stays, because without it the mutex row degenerates to 155 retries per op and a two-vCPU CI runner would be far worse than this 12-core box; but it is now named where a reader will meet it, and the sweep's own comment says any ratio taken from these rows is a statement about the policy too. "Every value is a power of two because MpmcQueue masks its indices" was false: mpmc_queue.ipp falls back to % when capacity is not a power of two, and QueueContract/Mpmc.FillsToExactlyCapacity exercises capacity 3. The masking is a fast path, not a constraint, and the sweep values are a choice. Cleanups alongside: hoist the two prefill loops, which had drifted into two different failure idioms fifteen lines apart, into install_half_full_queue; name the sweep list once instead of repeating it at three registrations, which also retires the NOLINT(readability-magic-numbers) bracket, since clang-tidy exempts literals in a const initializer; use Google Benchmark's documented counters["name"] = Counter(...) idiom so the counter names land at the left margin; trim two rationale paragraphs that restated what the code beside them already showed; and note that an odd producer/consumer split would misreport the per-side counters but deadlocks the harness first. Co-Authored-By: Claude Opus 5 (1M context) --- bench/queue_bench.cpp | 92 ++++++++++++++++++++++------------------- bench/try_operation.hpp | 37 ++++++++++------- 2 files changed, 73 insertions(+), 56 deletions(-) diff --git a/bench/queue_bench.cpp b/bench/queue_bench.cpp index 6441474..89e3f3e 100644 --- a/bench/queue_bench.cpp +++ b/bench/queue_bench.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include @@ -88,39 +89,35 @@ class MoodycamelQueue { template std::unique_ptr shared_queue; +// Half-full start: neither side begins blocked or spinning on the other, so +// the measurement starts in steady state. At capacity 1 the half is 0, so that +// sweep point deliberately starts empty — which is the retry pressure it is +// there to measure. template -void setup_queue(const benchmark::State& /*state*/) { - shared_queue = std::make_unique(kCapacity); - // Half-full start: neither side begins blocked or spinning on the other, so - // the measurement starts in steady state. - bool prefilled = true; - for (std::uint64_t i = 0; i < kCapacity / 2; ++i) { - prefilled = prefilled && shared_queue->try_push(i); - } - if (!prefilled) { - std::abort(); // unreachable: fresh queue, stays below capacity +void install_half_full_queue(std::size_t capacity) { + shared_queue = std::make_unique(capacity); + for (std::uint64_t i = 0; i < capacity / 2; ++i) { + if (!shared_queue->try_push(i)) { + std::abort(); // unreachable: fresh queue, stays below capacity + } } } template -void teardown_queue(const benchmark::State& /*state*/) { - shared_queue.reset(); +void setup_queue(const benchmark::State& /*state*/) { + install_half_full_queue(kCapacity); } -// Same as setup_queue, but takes the capacity from the registered Arg so one -// benchmark can sweep it. +// Same, but takes the capacity from the registered Arg so one benchmark can +// sweep it. template void setup_queue_at_capacity(const benchmark::State& state) { - const auto capacity = static_cast(state.range(0)); - shared_queue = std::make_unique(capacity); - // Half-full where the half exists: at capacity 1 it is 0, so that sweep - // point deliberately starts empty -- which is the retry pressure it is - // there to measure. - for (std::uint64_t i = 0; i < capacity / 2; ++i) { - if (!shared_queue->try_push(i)) { - std::abort(); // unreachable: fresh queue, stays below capacity - } - } + install_half_full_queue(static_cast(state.range(0))); +} + +template +void teardown_queue(const benchmark::State& /*state*/) { + shared_queue.reset(); } template @@ -169,16 +166,16 @@ void BM_QueueTryThroughput(benchmark::State& state) { // Counters sum across threads, and kAvgIterations divides by the iteration // total over *all* threads. Producers and consumers each own half of that // total, so doubling one side's retries before the divide yields that side's - // retries per its own op -- for any even producer/consumer split, not just - // the 1+1 these sweeps register. + // retries per its own op — for any even producer/consumer split, verified + // bit-exact at 2 and at 8 threads. An odd split would misreport, but it + // deadlocks this harness first: pushes and pops would no longer balance. + using benchmark::Counter; const auto side_retries = 2.0 * static_cast(retries); - state.counters.emplace( - "push_retries/push", - benchmark::Counter(is_producer ? side_retries : 0.0, benchmark::Counter::kAvgIterations)); - state.counters.emplace("pop_retries/pop", benchmark::Counter(is_producer ? 0.0 : side_retries, - benchmark::Counter::kAvgIterations)); - state.counters.emplace("retries/op", benchmark::Counter(static_cast(retries), - benchmark::Counter::kAvgIterations)); + state.counters["push_retries/push"] = + Counter(is_producer ? side_retries : 0.0, Counter::kAvgIterations); + state.counters["pop_retries/pop"] = + Counter(is_producer ? 0.0 : side_retries, Counter::kAvgIterations); + state.counters["retries/op"] = Counter(static_cast(retries), Counter::kAvgIterations); } // Uncontended single-thread round trip: the queue's raw per-op cost with no @@ -239,6 +236,14 @@ static_assert(kSpscThreads % 2 == 0 && kMpmcThreads % 2 == 0, // iteration form overrides, which is what the ctest smoke run uses (1x). constexpr double kMinTimeSeconds = 1.0; +// Capacity sweep points for the non-blocking runs: 1 and 2 force every op to +// wait on its counterpart, 8 sits at the knee, and 64 / 1024 are deep enough +// for the two sides to decouple — 1024 being the capacity the blocking rows +// use, so those rows stay comparable. Powers of two keep MpmcQueue on its mask +// fast path; it supports other capacities via % and is tested at 3. +// Not constexpr: ArgsProduct takes vector>. +const std::vector capacity_sweep{1, 2, 8, 64, 1024}; + BENCHMARK(BM_QueueThroughput) ->Setup(setup_queue) ->Teardown(teardown_queue) @@ -291,16 +296,20 @@ BENCHMARK(BM_QueueThroughput) ->MinTime(kMinTimeSeconds) ->Name("MoodycamelQueue/throughput"); -// Non-blocking sweeps. Capacity is the independent variable here, not a -// tuning constant, and every value is a power of two because MpmcQueue masks -// its indices. Only the cq queues are swept: moodycamel is unbounded, so it -// has no capacity to vary, and tbb's adapter has no try_pop yet. -// NOLINTBEGIN(readability-magic-numbers) +// Non-blocking sweeps. Only the cq queues are swept: neither external adapter +// has a try_pop, and moodycamel is unbounded besides, so it has no capacity to +// vary. +// +// Any ratio read across these rows is a statement about the retry policy as +// much as about the queues. With the yield, Spsc/Mutex at capacity 1 is ~1.5x; +// without it, ~36x. See try_operation.hpp. The shallow points are +// scheduler-sensitive by construction, so they need --benchmark_repetitions +// more than the rows above do, not less. BENCHMARK(BM_QueueTryThroughput) ->Setup(setup_queue_at_capacity) ->Teardown(teardown_queue) ->ArgName("capacity") - ->ArgsProduct({{1, 2, 8, 64, 1024}}) + ->ArgsProduct({capacity_sweep}) ->Threads(kSpscThreads) ->UseRealTime() ->MinTime(kMinTimeSeconds) @@ -310,7 +319,7 @@ BENCHMARK(BM_QueueTryThroughput) ->Setup(setup_queue_at_capacity) ->Teardown(teardown_queue) ->ArgName("capacity") - ->ArgsProduct({{1, 2, 8, 64, 1024}}) + ->ArgsProduct({capacity_sweep}) ->Threads(kSpscThreads) ->UseRealTime() ->MinTime(kMinTimeSeconds) @@ -320,12 +329,11 @@ BENCHMARK(BM_QueueTryThroughput) ->Setup(setup_queue_at_capacity) ->Teardown(teardown_queue) ->ArgName("capacity") - ->ArgsProduct({{1, 2, 8, 64, 1024}}) + ->ArgsProduct({capacity_sweep}) ->Threads(kSpscThreads) ->UseRealTime() ->MinTime(kMinTimeSeconds) ->Name("MpmcQueue/try_throughput"); -// NOLINTEND(readability-magic-numbers) // UseRealTime on the round trips too: Google Benchmark divides a rate counter // by whichever clock the benchmark selected, so without it these rows report diff --git a/bench/try_operation.hpp b/bench/try_operation.hpp index 578f67e..5a92703 100644 --- a/bench/try_operation.hpp +++ b/bench/try_operation.hpp @@ -11,22 +11,31 @@ namespace cq::bench { // Retry until the operation succeeds, returning how many attempts failed. // -// The yield is load-bearing, not politeness: without it a spinning side keeps -// barging the lock back from its counterpart, which then cannot make the -// progress that would let the spinner succeed. At capacity 1 or 2 every op -// depends on the counterpart, so one starvation episode dominates a whole run -// and the reported rate swings by orders of magnitude between invocations. +// The yield is a deliberate policy choice, and it is not neutral — it is worth +// naming because it moves the numbers more than the queues do. Measured at +// capacity 1, where every op waits on its counterpart: // -// The operation is taken by value, following the std:: algorithm convention: -// it is invoked repeatedly, so a forwarding reference would never actually be -// forwarded. Callers needing to observe its state changes capture by -// reference, as the benchmark call sites do. +// MutexQueue 0.58 M/s without → 5.11 M/s with (yield buys 8.8x) +// SpscQueue 20.9 M/s without → 7.84 M/s with (yield costs 2.7x) +// MpmcQueue 21.1 M/s without → 6.84 M/s with (yield costs 3.1x) // -// Deliberately not constrained with std::predicate: that subsumes -// std::regular_invocable, whose contract is equality-preserving invocation, -// and this helper exists precisely to call something whose answer changes -// between calls. It also constrains an rvalue F&&, while the body invokes an -// lvalue. +// The mechanism only exists for the mutex queue: try_push and try_pop take a +// blocking lock_guard, so an unyielding spinner keeps barging the lock back +// from the counterpart that would have made room. The lock-free queues have no +// lock to barge, so there the yield is pure overhead. +// +// It stays anyway: without it the mutex row degenerates (155 retries per op at +// capacity 1) and a CI runner with two vCPUs would be far worse than this +// 12-core box. But any ratio taken from these rows is a statement about this +// policy as much as about the queues — see the sweep's comment in +// queue_bench.cpp. +// +// By value, per the std:: algorithm convention: the operation is invoked +// repeatedly, so a forwarding reference would never actually be forwarded. +// +// Not std::predicate: that subsumes std::regular_invocable, whose contract is +// equality-preserving invocation, and this helper exists precisely to call +// something whose answer changes between calls. template requires std::invocable && std::convertible_to, bool> [[nodiscard]] std::size_t count_failures_until_success(Operation operation) { From d22be2c802d4a4b8e60c71ad3a2503699fdc9315 Mon Sep 17 00:00:00 2001 From: Debra Date: Thu, 27 Aug 2026 22:32:13 +0800 Subject: [PATCH 3/6] bench: quote ratios, not rates, for the yield's effect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The measured table this comment carried was itself the mistake it was added to fix. Re-measuring at a different machine load moved every one of its six absolute figures by 10-25%: MutexQueue 0.58 -> 0.46 M/s without the yield, SpscQueue 20.9 -> 18.2, MpmcQueue 21.1 -> 16.6, and the headline Spsc/Mutex ratio from ~36x to ~40x. The "155 retries per op" measured 194. The directions and rough magnitudes reproduce on every run; the rates do not. So the comment now states only what survives — removing the yield costs MutexQueue about a factor of ten and gains each lock-free queue two to three — and says why no rates are quoted. Same correction in the sweep's comment: Spsc/Mutex at capacity 1 is under 2x with the yield and around 40x without, rather than the ~1.5x / ~36x taken from a single run. Co-Authored-By: Claude Opus 5 (1M context) --- bench/queue_bench.cpp | 8 ++++---- bench/try_operation.hpp | 21 ++++++++++----------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/bench/queue_bench.cpp b/bench/queue_bench.cpp index 89e3f3e..3576305 100644 --- a/bench/queue_bench.cpp +++ b/bench/queue_bench.cpp @@ -301,10 +301,10 @@ BENCHMARK(BM_QueueThroughput) // vary. // // Any ratio read across these rows is a statement about the retry policy as -// much as about the queues. With the yield, Spsc/Mutex at capacity 1 is ~1.5x; -// without it, ~36x. See try_operation.hpp. The shallow points are -// scheduler-sensitive by construction, so they need --benchmark_repetitions -// more than the rows above do, not less. +// much as about the queues: at capacity 1, Spsc/Mutex is under 2x with the +// yield and around 40x without it. See try_operation.hpp. The shallow points +// are scheduler-sensitive by construction, so they need +// --benchmark_repetitions more than the rows above do, not less. BENCHMARK(BM_QueueTryThroughput) ->Setup(setup_queue_at_capacity) ->Teardown(teardown_queue) diff --git a/bench/try_operation.hpp b/bench/try_operation.hpp index 5a92703..c962357 100644 --- a/bench/try_operation.hpp +++ b/bench/try_operation.hpp @@ -12,23 +12,22 @@ namespace cq::bench { // Retry until the operation succeeds, returning how many attempts failed. // // The yield is a deliberate policy choice, and it is not neutral — it is worth -// naming because it moves the numbers more than the queues do. Measured at -// capacity 1, where every op waits on its counterpart: -// -// MutexQueue 0.58 M/s without → 5.11 M/s with (yield buys 8.8x) -// SpscQueue 20.9 M/s without → 7.84 M/s with (yield costs 2.7x) -// MpmcQueue 21.1 M/s without → 6.84 M/s with (yield costs 3.1x) +// naming because it moves the numbers more than the queue choice does. At +// capacity 1, where every op waits on its counterpart, removing it costs +// MutexQueue roughly a factor of ten and gains each lock-free queue a factor +// of two to three. Ratios only: the absolute rates move 10-25% with machine +// load, so quoting them would bake one afternoon's conditions into the source. // // The mechanism only exists for the mutex queue: try_push and try_pop take a // blocking lock_guard, so an unyielding spinner keeps barging the lock back // from the counterpart that would have made room. The lock-free queues have no // lock to barge, so there the yield is pure overhead. // -// It stays anyway: without it the mutex row degenerates (155 retries per op at -// capacity 1) and a CI runner with two vCPUs would be far worse than this -// 12-core box. But any ratio taken from these rows is a statement about this -// policy as much as about the queues — see the sweep's comment in -// queue_bench.cpp. +// It stays anyway: without it the mutex row degenerates to a couple of hundred +// retries per op at capacity 1, and a CI runner with two vCPUs would fare far +// worse than this 12-core box. But any ratio taken from these rows is a +// statement about this policy as much as about the queues — see the sweep's +// comment in queue_bench.cpp. // // By value, per the std:: algorithm convention: the operation is invoked // repeatedly, so a forwarding reference would never actually be forwarded. From 32f662a56333916c9a752f2a319140d3bafd9fcf Mon Sep 17 00:00:00 2001 From: Debra Date: Thu, 27 Aug 2026 22:38:32 +0800 Subject: [PATCH 4/6] bench: correct the mask claim, soften the odd-split claim, lint the new header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review follow-ups. "Powers of two keep MpmcQueue on its mask fast path" is false at capacity 1 — the sweep's shallowest point, and the one bench_retry_pressure keys on. has_single_bit(1) is true, so the ctor sets mask_ = 1 - 1 = 0, and slot_index() gates on mask_ != 0, sending capacity 1 down the % path: a runtime modulo by a runtime divisor, not the fast path the comment promises. "An odd split would misreport, but it deadlocks this harness first" was overstated twice. It is a spin-with-yield livelock, not a deadlock. And "first" is conditional: with N iterations per thread against a prefill of capacity/2, an odd split completes normally whenever the surplus pops fit inside the prefill — bench_smoke's own regime (1x at capacity 1024, prefill 512) is exactly that, so there it would report silently wrong counters rather than hang. bench/try_operation.hpp is the first header this repo has placed outside include/cq, and so the first one clang-tidy never sees: HeaderFilterRegex was scoped to 'include/cq/.*' and CI lints only *.cpp, so a diagnostic in the new header was silently dropped. Widened to '(include/cq|bench)/.*'. Verified both directions: an injected BadlyNamedVar in that header now produces readability-identifier-naming, and with the probe removed every tracked .cpp is still clean under the wider filter. Also record why capacity_sweep is const rather than only why it is not constexpr — the const is what makes clang-tidy exempt its literals, so removing it as redundant would resurrect the magic-numbers error the same commit deleted a NOLINT for. Co-Authored-By: Claude Opus 5 (1M context) --- .clang-tidy | 4 +++- bench/queue_bench.cpp | 16 ++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 28b0d3a..fc62652 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -14,7 +14,9 @@ Checks: > -modernize-use-trailing-return-type, -misc-include-cleaner WarningsAsErrors: '*' -HeaderFilterRegex: 'include/cq/.*' +# bench/ is included because this is where the first header outside include/cq +# landed; without it that header receives no diagnostics at all. +HeaderFilterRegex: '(include/cq|bench)/.*' CheckOptions: # Complexity contributed by macro expansion (GTest asserts, etc.) is not # the author's complexity. diff --git a/bench/queue_bench.cpp b/bench/queue_bench.cpp index 3576305..b0b1223 100644 --- a/bench/queue_bench.cpp +++ b/bench/queue_bench.cpp @@ -167,8 +167,11 @@ void BM_QueueTryThroughput(benchmark::State& state) { // total over *all* threads. Producers and consumers each own half of that // total, so doubling one side's retries before the divide yields that side's // retries per its own op — for any even producer/consumer split, verified - // bit-exact at 2 and at 8 threads. An odd split would misreport, but it - // deadlocks this harness first: pushes and pops would no longer balance. + // bit-exact at 2 and at 8 threads. An odd split would misreport (the divisor + // would need to be the thread count, not 2). At the shallow sweep points it + // livelocks instead, spinning on a counterpart that never arrives; deeper in, + // the prefill can absorb the imbalance and it would report silently wrong + // numbers. Only even splits are registered. using benchmark::Counter; const auto side_retries = 2.0 * static_cast(retries); state.counters["push_retries/push"] = @@ -240,8 +243,13 @@ constexpr double kMinTimeSeconds = 1.0; // wait on its counterpart, 8 sits at the knee, and 64 / 1024 are deep enough // for the two sides to decouple — 1024 being the capacity the blocking rows // use, so those rows stay comparable. Powers of two keep MpmcQueue on its mask -// fast path; it supports other capacities via % and is tested at 3. -// Not constexpr: ArgsProduct takes vector>. +// fast path — except capacity 1, where the mask degenerates to 0 and it falls +// back to % anyway. Non-powers of two are supported and tested at 3; these +// values are a choice, not a requirement. +// +// const, not constexpr: ArgsProduct takes vector>. The const +// is load-bearing beyond style — clang-tidy exempts literals in a const +// initializer, which is what retires the magic-numbers NOLINT here. const std::vector capacity_sweep{1, 2, 8, 64, 1024}; BENCHMARK(BM_QueueThroughput) From bb8f5e7c7c70642873bdf2e397c5c7d419f32250 Mon Sep 17 00:00:00 2001 From: Debra Date: Thu, 27 Aug 2026 22:59:19 +0800 Subject: [PATCH 5/6] bench: close the lint gap properly, and fix the odd-split scale factor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review follow-ups. Both are claims the previous commit introduced while fixing round 2 — the third round running in which a fix has produced a new false statement about the same subject. "The first header this repo has placed outside include/cq" was wrong three times over: in the .clang-tidy comment, the commit message, and the PR body. tests/queue_test_util.hpp already sat outside it on main, and the widened regex left it out — so the gap the commit claimed to close was only half closed, and the stated rationale ("without it that header receives no diagnostics at all") applied verbatim to a header the fix skipped. The filter now covers include/cq, bench and tests, and is anchored: it was an unanchored substring match, and googletest and benchmark are not declared SYSTEM here, so a checkout under a path containing a bench/ segment would have started linting dependency headers. Verified an injected bad name in both headers now produces diagnostics, and that every tracked .cpp is still clean with the probes removed. "The divisor would need to be the thread count, not 2" was also wrong. kAvgIterations divides by the iteration total across all threads, so the scale is threads/producers on the push side and threads/consumers on the pop side; the thread count is right only when a side has exactly one thread. Measured at threads:3, capacity:1024: pop_retries/pop reported 1.30208m against a ground truth of 976.6u, over by exactly 4/3. The livelock qualifier was also lost between the commit message and the comment. The discriminator is iterations against prefill, not depth: under the registered MinTime, iterations-per-thread grows far past any prefill, so an odd split livelocks at every capacity including the deepest. Only an iteration-capped smoke run completes, and only there would it report silently wrong numbers. Co-Authored-By: Claude Opus 5 (1M context) --- .clang-tidy | 9 ++++++--- bench/queue_bench.cpp | 12 +++++++----- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index fc62652..6f5c4e3 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -14,9 +14,12 @@ Checks: > -modernize-use-trailing-return-type, -misc-include-cleaner WarningsAsErrors: '*' -# bench/ is included because this is where the first header outside include/cq -# landed; without it that header receives no diagnostics at all. -HeaderFilterRegex: '(include/cq|bench)/.*' +# Cover every first-party directory that holds headers, not just include/cq: +# tests/queue_test_util.hpp and bench/try_operation.hpp both sit outside it and +# would otherwise receive no diagnostics at all. Anchored so a checkout under a +# path containing one of these segments cannot pull in dependency headers — +# googletest and benchmark are not declared SYSTEM. +HeaderFilterRegex: '(^|/)(include/cq|bench|tests)/' CheckOptions: # Complexity contributed by macro expansion (GTest asserts, etc.) is not # the author's complexity. diff --git a/bench/queue_bench.cpp b/bench/queue_bench.cpp index b0b1223..73daed4 100644 --- a/bench/queue_bench.cpp +++ b/bench/queue_bench.cpp @@ -167,11 +167,13 @@ void BM_QueueTryThroughput(benchmark::State& state) { // total over *all* threads. Producers and consumers each own half of that // total, so doubling one side's retries before the divide yields that side's // retries per its own op — for any even producer/consumer split, verified - // bit-exact at 2 and at 8 threads. An odd split would misreport (the divisor - // would need to be the thread count, not 2). At the shallow sweep points it - // livelocks instead, spinning on a counterpart that never arrives; deeper in, - // the prefill can absorb the imbalance and it would report silently wrong - // numbers. Only even splits are registered. + // bit-exact at 2 and at 8 threads. An odd split would misreport: the scale + // would have to be threads/producers on the push side and threads/consumers + // on the pop side, not a constant 2. It also livelocks under the registered + // MinTime, at every capacity, because iterations-per-thread grows far past + // the prefill that would otherwise absorb the imbalance; only an + // iteration-capped smoke run completes, and there the numbers would be + // silently wrong. Only even splits are registered. using benchmark::Counter; const auto side_retries = 2.0 * static_cast(retries); state.counters["push_retries/push"] = From ec4d13bb1f8fa4577258c21498c3b833a3ab0ceb Mon Sep 17 00:00:00 2001 From: Debra Date: Thu, 27 Aug 2026 23:16:59 +0800 Subject: [PATCH 6/6] build: make benchmark a SYSTEM dependency, closing the lint gap this PR opened MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The anchoring rationale in the previous commit was false, and the thing it claimed to prevent is a real breakage this PR introduced. `(^|/)` only excludes segment-substring matches like microbench/. It still matches a bench/ segment anywhere above the repo root — so widening HeaderFilterRegex to bench|tests exposed dependency headers to WarningsAsErrors wherever such a segment appears in the path. A GitHub Actions repo named `bench` checks out to /home/runner/work/bench/bench/, and `cmake -B tests/build` does it locally. Reproduced by relocating the compile database under /tmp/bench/link: 199 hard errors out of benchmark/benchmark.h. main's include/cq/.* filter was immune; this branch was not. The fix belongs in CMakeLists, not the regex. benchmark is the only dependency arriving as a plain -I: concurrentqueue and tbb already declare SYSTEM, and googletest self-marks its interface SYSTEM regardless. Adding SYSTEM to its FetchContent_Declare takes the same relocated build from 199 diagnostics to 0, while the ordinary path stays clean and every tracked .cpp still lints without a diagnostic. The .clang-tidy comment now says dependencies stay out because they are SYSTEM, which is true, instead of crediting the anchor, which is not. Also replace the odd-split sentence that three rounds have now rewritten with the single condition it was circling: an odd split completes only while (consumers - producers) * iterations-per-thread <= capacity / 2. The "every capacity under MinTime" case and the smoke-run case both fall out of it, and the previous phrasing was wrong at capacity 1, where the prefill is 0 and an odd split hangs even under an iteration cap. Co-Authored-By: Claude Opus 5 (1M context) --- .clang-tidy | 7 ++++--- CMakeLists.txt | 9 ++++++++- bench/queue_bench.cpp | 12 +++++++----- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 6f5c4e3..5f33529 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -16,9 +16,10 @@ Checks: > WarningsAsErrors: '*' # Cover every first-party directory that holds headers, not just include/cq: # tests/queue_test_util.hpp and bench/try_operation.hpp both sit outside it and -# would otherwise receive no diagnostics at all. Anchored so a checkout under a -# path containing one of these segments cannot pull in dependency headers — -# googletest and benchmark are not declared SYSTEM. +# would otherwise receive no diagnostics at all. Dependency headers stay out +# because every dependency is SYSTEM, not because of the anchor — the anchor +# only excludes segment-substring matches like microbench/, and still matches a +# bench/ segment above the repo root. HeaderFilterRegex: '(^|/)(include/cq|bench|tests)/' CheckOptions: # Complexity contributed by macro expansion (GTest asserts, etc.) is not diff --git a/CMakeLists.txt b/CMakeLists.txt index 9be0d09..fe1c200 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -66,11 +66,18 @@ if(CQ_BUILD_TESTS) endif() if(CQ_BUILD_BENCHMARKS) + # SYSTEM, like the two below: benchmark is the only dependency here that + # arrives as a plain -I (googletest self-marks its interface SYSTEM), so it is + # the only one clang-tidy's header filter can reach. That matters because the + # filter matches a path segment anywhere above the repo, so a checkout under + # e.g. /home/runner/work/bench/bench/ would otherwise lint benchmark.h under + # WarningsAsErrors. FetchContent_Declare( benchmark URL https://github.com/google/benchmark/archive/refs/tags/v1.9.1.tar.gz URL_HASH SHA256=32131c08ee31eeff2c8968d7e874f3cb648034377dfc32a4c377fa8796d84981 - DOWNLOAD_EXTRACT_TIMESTAMP TRUE) + DOWNLOAD_EXTRACT_TIMESTAMP TRUE + SYSTEM) set(BENCHMARK_ENABLE_TESTING OFF CACHE BOOL "" FORCE) set(BENCHMARK_ENABLE_INSTALL OFF CACHE BOOL "" FORCE) diff --git a/bench/queue_bench.cpp b/bench/queue_bench.cpp index 73daed4..fe6d88e 100644 --- a/bench/queue_bench.cpp +++ b/bench/queue_bench.cpp @@ -169,11 +169,13 @@ void BM_QueueTryThroughput(benchmark::State& state) { // retries per its own op — for any even producer/consumer split, verified // bit-exact at 2 and at 8 threads. An odd split would misreport: the scale // would have to be threads/producers on the push side and threads/consumers - // on the pop side, not a constant 2. It also livelocks under the registered - // MinTime, at every capacity, because iterations-per-thread grows far past - // the prefill that would otherwise absorb the imbalance; only an - // iteration-capped smoke run completes, and there the numbers would be - // silently wrong. Only even splits are registered. + // on the pop side, not a constant 2. Whether it misreports or simply hangs + // is one condition: an odd split completes only while + // (consumers - producers) * iterations-per-thread <= capacity / 2, the + // prefill being the only slack. Under the registered MinTime that product + // reaches millions, so every sweep point livelocks; an iteration-capped run + // completes only at the capacities whose prefill covers it, and there the + // counters would be silently wrong. Only even splits are registered. using benchmark::Counter; const auto side_retries = 2.0 * static_cast(retries); state.counters["push_retries/push"] =