bench: sweep the non-blocking path across capacity, on one clock - #14
Merged
Merged
Conversation
jadecubes
force-pushed
the
fix/benchmark-success-accounting
branch
from
August 27, 2026 09:08
e72a97d to
da2874c
Compare
jadecubes
pushed a commit
that referenced
this pull request
Aug 27, 2026
Decision 1 settled: SpscQueue ships try_push/try_pop only, with no blocking push()/pop() and no close(). Blocking would need a condition variable, which needs a mutex, which contaminates the claim v2 exists to test. The fairness half is already handled on the v1 side -- PR #14 added a MutexQueue try_ throughput sweep -- so the comparison is like-for-like without giving v2 a lock on its slow path. Folded in two findings from that PR that constrain v2's benchmark: a retry loop without backoff makes shallow capacities swing ~400x between runs, and every registration must set UseRealTime() or its throughput is normalised by a different clock than the rows beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
jadecubes
force-pushed
the
fix/benchmark-success-accounting
branch
from
August 27, 2026 09:59
da2874c to
69d58dd
Compare
…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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…ew header 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…PR opened 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this adds
mainonly benchmarks the blockingpush/poppath. This adds the non-blocking one:BM_QueueTryThroughput<Queue>, swept across capacities 1, 2, 8, 64 and 1024 forMutexQueue,SpscQueueandMpmcQueue.Failed attempts are retried inside the timed iteration, so they cost time but never count as transferred work —
items/sstays the same unit as the blocking rows. The pressure that caused them is reported separately asretries/op, split intopush_retries/pushandpop_retries/pop.Result
5 invocations × 10 repetitions, load 3.5–6.1,
items/sas mean (min–max):The lock-free advantage is not a constant. It is small while the ring is shallow and opens up once a producer can run ahead.
retries/optracks it: 1.05–1.30 at capacity 1, 0.0005–0.0054 at 1024.Read the table with the retry policy in mind. The loop yields after each failure, which only helps
MutexQueue— itstry_calls take a blockinglock_guard, so an unyielding spinner barges the lock back from the counterpart that would have made room. The lock-free queues have no lock to barge, so the yield is pure overhead there. Removing it at capacity 1 costsMutexQueue~10× and gains each lock-free queue 2–3×, movingSpsc/Mutexfrom under 2× to around 40×.The yield stays (without it the mutex row degenerates, and a 2-vCPU runner would be worse than this 12-core box), but it is now named in the helper and in the sweep's comment. Ratios rather than rates throughout: absolute figures move 10–25% with machine load.
Two defects fixed along the way
Clock. The five
single_thread_roundtripregistrations set noUseRealTime(), so they reporteditems/sper CPU-second while every threaded row is per wall-second — despite a comment claiming the units matched. Small (1.004–1.015) but two different quantities.Lint coverage.
HeaderFilterRegexcovered onlyinclude/cq/, sotests/queue_test_util.hppand the newbench/try_operation.hppgot no clang-tidy diagnostics at all. Widening it exposed a second problem: the filter matches a path segment above the repo root, so a checkout under/home/runner/work/bench/bench/lintsbenchmark.hunderWarningsAsErrors(reproduced at 187 errors). Fixed by declaringbenchmarkSYSTEM, asconcurrentqueueandtbbalready are.One open question
MutexQueueis slower at capacity 64 than at capacity 8. Paired sampling — both points inside the same invocation, eight invocations — gives 8/8 in the same direction, difference +9.47 ± 2.36 M/s. It is real, and I have no explanation for it.Scope
Only the
cqqueues are swept: neither external adapter has atry_pop, and moodycamel is unbounded so it has no capacity to vary.Files
bench/queue_bench.cppBM_QueueTryThroughput<Queue>— same thread split as the blocking benchmark, but each iteration retries until it succeeds and accumulates the failures, then publishes threekAvgIterationscounters. Addssetup_queue_at_capacity<Queue>, which reads the capacity from the registeredArgso one benchmark can sweep it, and hoists the prefill both setups share intoinstall_half_full_queue<Queue>. Registers the sweep three times against a namedcapacity_sweeplist. Adds->UseRealTime()to the fivesingle_thread_roundtripregistrations.bench/try_operation.hppcount_failures_until_success(op)— retry until success, return the failure count,std::this_thread::yield()after each failure. Constrained withinvocable<Operation&> && convertible_to<invoke_result_t<Operation&>, bool>rather thanstd::predicate, which would demand equality-preserving invocation this deliberately violates. Most of the file is the rationale for the yield: what it buys, what it costs, and why the ratios and not the rates are quoted.bench/CMakeLists.txtbench_retry_pressurectest, which runs the capacity-1 sweep point and assertsretries/opis non-zero on the_meanrow. Separate frombench_smokebecausePASS_REGULAR_EXPRESSIONsuppresses CTest's exit-code check, andbench_smokeshould keep it.tests/try_operation_test.cpptests/CMakeLists.txtbench/so it can include the helper..clang-tidyHeaderFilterRegex: 'include/cq/.*'→'(^|/)(include/cq|bench|tests)/'. Bothtests/queue_test_util.hppand the newbench/try_operation.hppsit outsideinclude/cq/and were receiving no diagnostics at all.CMakeLists.txtSYSTEMtobenchmark'sFetchContent_Declare. Required by the row above: it is the only dependency arriving as a plain-I, so it is the only one the widened filter can reach.Verification
Release
ctest68/68 (66 before), TSan 66/66,clang-formatandclang-tidyclean. Stubbing the helper toreturn 0;failsbench_retry_pressurewhilebench_smokestill passes.Originally opened against
feat/mutex-queue, a branch already merged as #2 thatmainhas since moved past. Rebuilt onmainand re-targeted; pre-rebuild head wasda2874c.🤖 Generated with Claude Code