feat: add MutexQueue v1 with tests and benchmarks - #2
Merged
Conversation
jadecubes
force-pushed
the
feat/mutex-queue
branch
from
August 16, 2026 08:25
5560be8 to
b650d31
Compare
Bounded mutex+condvar FIFO ring (v1 baseline), its GoogleTest unit and TSan stress tests, and Google Benchmark targets. The CMake scaffolding from the tooling PR picks these directories up automatically. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- factor the hand-negated full/empty conditions into can_enqueue_locked / can_dequeue_locked so each contract exists once - drop the now-unneeded EXISTS scaffolding guards and make ctest fail on zero discovered tests (--no-tests=error): a missing directory should fail loudly now that the sources exist - shared benchmark queue: optional + defensive guard -> unique_ptr (bugprone-unchecked-optional-access wants the guard the review flagged as dead; unique_ptr needs neither) - close() contract documented once, on close() - '^$_' identifier carve-out now inherited from the root .clang-tidy Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jadecubes
force-pushed
the
feat/mutex-queue
branch
from
August 16, 2026 08:31
b650d31 to
fc8abd9
Compare
Queue: - Inline the can_enqueue/can_dequeue predicates: push's wait no longer re-tests closed_ inside the predicate, pop no longer evaluates the predicate twice per dequeue, and closure is checked exactly once per entry point - close() skips both notify_all()s on repeat calls (std::exchange) - Class doc states the thread-safety contract (STYLE.md requires it); drop the .ipp build-layout note from the published API docs Tests: - Shared tests/queue_test_util.hpp holds kSettleTime, run_blocked, and spawn_threads for both TUs (and future queue variants' tests) - tests/.clang-tidy narrows identifier-length via Ignored*Names regexes instead of disabling the whole check Benchmarks: - One queue op per harness iteration: Google Benchmark regains control of run length (the 100k inner loop defeated --benchmark_min_time and made the CI smoke run execute ~500k ops), and the producer/consumer branch moves out of the timed region - Guard odd ->Threads() counts with SkipWithError instead of a comment CMake: - cq INTERFACE links Threads::Threads (the library that creates the thread requirement owns it, not each consumer) - gtest PRE_TEST discovery set project-wide instead of per-target Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # .github/workflows/ci.yml
…uracy Tests: - run_blocked now asserts the operation was actually still blocked before unblocking (atomic finished flag); previously the four blocking tests passed even if the op returned immediately without blocking - join_all helper (inline; the header is multi-TU) makes the stress test's shutdown ordering read as intent: join_all(producers); close(); join_all(consumers) - Stress producers assert once per thread instead of expanding an AssertionResult per item (25k per thread under TSan) - tests/.clang-tidy ignore lists trimmed to names the code actually uses Benchmarks (both defects found by measurement): - SetItemsProcessed now reported from every thread; producer-only reporting inflated items/s by ~100000/threads (measured 1.33M/s reported vs 106 items/s actual at threads:8) - setup prefills the ring half-full so measurement starts in steady state instead of on a condition-variable wakeup storm - File header documents that thread spawn sits inside the timed region and recommends --benchmark_min_time=1s for meaningful numbers CMake: comment accuracy fixes (garbled INTERFACE note, discovery-timeout rationale, drop speculative sentence). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- [[nodiscard]] on push/try_push/pop/try_pop — the operations whose discarded result is always a bug (reading out after an ignored try_pop, silent data loss after an ignored try_push); accessors already had it - Class doc gains the two missing contracts STYLE.md requires: lifetime (close() + join before destruction; destroying with blocked threads is UB) and exception behavior (throwing T move-assign leaves the queue consistent) - size()/closed() documented as advisory snapshots that must not drive control flow — the try_ operations are the atomic alternative - try_pop doc states out is untouched on failure - Bench prefill explicitly discards try_push's result with a cannot-fail note Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… harness Benchmarks (measured: reported scaling was wrong by a factor of threads): - SetItemsProcessed(iterations * threads): Benchmark sums real time across threads, so per-thread iteration counts divided the true aggregate rate by the thread count (2->8 slowdown reported 6.9x, actually 1.9x) - Single-thread roundtrip counts push+pop as two ops, making its unit identical to the threaded benchmark - Odd-thread guard moved from a per-run runtime check to a static_assert beside the thread-count constants - Prefill self-checks via one postcondition instead of 512 (void) casts Tests: - run_blocked rewritten on std::async/std::future: wait_for IS the still-blocked check and get() IS the join — drops the atomic flag, the memory orders, and the hand-rolled comment; return type now deduced - kSettleTime 20ms -> 5ms (measured: the sleeps were 66% of the Debug suite wall time; 5ms is still ~250x the spawn-to-block latency) - Stress producers report failures via ADD_FAILURE on the failure path only; [[nodiscard]] on both test helpers - tests/.clang-tidy states the policy (check off) instead of allowlisting each identifier in use Docs: dedupe the class comment (snapshot caveat said once at class level, notification note moved beside the notify calls in the .ipp, lifetime rationale said once); gtest discovery settings co-located on the single gtest_discover_tests call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t harness README (the published numbers were measured before round 3 changed the benchmark's accounting, so every row and the story built on them was stale): - Re-measured on this machine: 105.1M ops/s single-thread round trip, 35.7M SPSC, 21.6M MPMC, with CV per row - The 2->8 thread slowdown is 1.65x, not the ~6x claimed; the SPSC run's CV is 1.4%, not ~18%, so the lock-convoy-jitter reading is gone - Define the unit once (an op is one push or one pop) and drop the 'counted on the producer side only' claim, which the benchmark never did - Note the measured cost of unconditional notify (~20% MPMC, ~26-37% SPSC) so v1 is not read as the fastest single-mutex design - Lint commands note CI's version pin Benchmarks: - MinTime(1s) on the registrations: run length is a property of the benchmark, so the documented command reproduces the table (the ctest smoke run still overrides with 1x) - Drop the file header's claim that thread spawn is inside the timed region — StartKeepRunning hits the start barrier before ResumeTiming Tests: - Delete join_all: spawn_threads already returns jthreads, whose destructor joins; the stress test says producers.clear() / consumers.clear() - spawn_threads uses jthread's forwarding constructor instead of a wrapper lambda per thread - Test helpers move to namespace cq::testing, out of the shipping namespace CMake/lint: - Sanitizer-vs-benchmark policy stated once, with a status message, instead of silently dropping benchmark targets from an explicit -DCQ_BUILD_BENCHMARKS=ON - The 'auto _' identifier carve-out moves to bench/.clang-tidy beside the other two benchmark-loop exceptions Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
All four items are defects in the previous round's own changes: - tests: rename cq::testing -> cq::test_util. Inside namespace cq, the old name shadowed GoogleTest's ::testing, so any future fixture written as ': public testing::Test' (or TestWithParam, or a gmock matcher import) would fail to compile; gtest's macros only survive it by expanding to fully-qualified ::testing - README: the Per-op column mixed denominators — per round trip on row 1, per-thread on rows 2-3 — so it read as a 6.6x MPMC penalty, silently contradicting the 1.65x in the prose two lines below. Now 1/throughput throughout (9.5 / 28.0 / 46.3 ns), stated above the table - bench: MinTime() on the registration beats --benchmark_min_time=<N>s (ComputeMinTime prefers a non-zero registered value), so that flag is accepted and ignored; only the Nx form overrides. Comment says so - CMake: CQ_BUILD_BENCHMARKS defaults to ON, so the policy block fired on the README's own TSan command and could never detect an explicit -D as the comment claimed; set() also shadowed the cache entry. Replaced with cmake_dependent_option so the cache reports what was built Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- CMakeLists: the previous comment credited cmake_dependent_option with a cache-visibility benefit it does not provide. Verified: under TSan the default path writes no cache entry at all, and an explicit -DCQ_BUILD_BENCHMARKS=ON is demoted to CQ_BUILD_BENCHMARKS:INTERNAL=ON — recording ON while OFF was built. The STATUS line is the real signal - README: 19.1 ns predated the column's redefinition as 1/throughput; 1/105.1M ops/s makes the round trip 19.0 ns Co-Authored-By: Claude Fable 5 <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
1 participant
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.
Adds the v1 baseline queue: a bounded FIFO ring guarded by a single mutex, with its tests and benchmarks. Was stacked on #1 (now merged);
mainhas since been merged back in and theci.ymlconflict resolved. Verified locally: all tests pass under TSan; clang-format and clang-tidy clean.Changed files
include/cq/mutex_queue.hppMutexQueue<T>public API: fixed-capacity ring, one mutex +not_full/not_emptycondition variables, blockingpush/pop, non-blockingtry_variants,close()shutdown semantics; copy/move deleted; class doc states the thread-safety contractinclude/cq/mutex_queue.ippclose()is idempotent and skips re-notifyingtests/queue_test_util.hppkSettleTime,run_blocked(spawn → settle → unblock → join),spawn_threads; reused by both test files and by future queue variants' teststests/mutex_queue_test.cpptry_failure paths, move-only element types, blocking behaviour and close-wakes viarun_blockedtests/stress_test.cpptests/CMakeLists.txtqueue_teststarget; discovery mode comes from the root (PRE_TEST, so the TSan binary never runs at build time)tests/.clang-tidyreadability-identifier-lengthto specific idiomatic names (q,p,c, …) instead of disabling the checkbench/queue_bench.cpp--benchmark_min_time/1xsmoke runs scale properly; odd thread counts rejected withSkipWithErrorbench/CMakeLists.txtqueue_benchtarget plus a one-iteration ctest smoke runbench/.clang-tidyDeadStoreson the canonicalfor (auto _ : state)loop; allowsBM_namesCMakeLists.txtEXISTSguards (both directories exist as of this PR);cqlinksThreads::Threads; project-widePRE_TESTgtest discovery.github/workflows/ci.yml--no-tests=error— the dynamic "does the directory exist" guard from the scaffolding era is obsolete. Conflict withmainresolved via merge🤖 Generated with Claude Code