Real-time front end: atomic scalars, pooled snapshots, allocation-free takeSnapshot() - #75
Merged
Conversation
Collaborator
Author
|
Added a same-machine benchmark comparison of
Pinned to CPUs 2–7, runs interleaved, Release builds, GCC 15.2. Medians at a paced 1 kHz are within noise; the report lists caveats (frequency scaling, hybrid cores, 3 reps). |
Design spec (pool + refcounted SnapshotRef handoff, priority-inheriting WriteMutex for transactions, atomic scalars, epoch-based reclamation) and the implementation plan for spec steps 0-3. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
With a ROS environment sourced, gtest_vendor was found even for non-ROS builds and the ament test branch compiled ros2_publisher_tests.cpp without rclcpp. Also run add_remove_sink_tests.cpp in the ament branch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Tests read latest_snapshot and snapshots_count while the sink thread wrote them. Replace the public members with mutex-protected accessors so the suite is clean under ThreadSanitizer. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
TSAN runs the test binary under setarch -R (kernel ASLR conflict) and with -Wno-tsan (moodycamel's atomic_thread_fence). Tests are no longer registered twice with ctest. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The example was missed when DummySink's public members became mutex-protected accessors; it is only built by the release preset. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Replaces global operator new/delete in the test binary; counting is active only inside an AllocCounter::Scope on the current thread, so background sink threads do not affect measurements. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…enchmarks; latency harness; baseline Records p50/p99/p99.9/max of takeSnapshot() at 1 kHz plus allocations per call on the unmodified library, so later steps have a reference. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Standalone component for the lock-free front end (spec §4.2). Not wired yet. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…pshot slots) Standalone component for the lock-free front end (spec §6.1). Not wired yet. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…dd std::atomic<T> constructor Wire bytes are unchanged (pinned by golden tests). sizeof(ValuePtr) drops from 104 to 56 bytes (<= 64), by replacing the two std::function members (serialize_impl_, get_size_impl_) with plain function pointers (SerializeFn, SizeFn); the CustomSerializer::Ptr shared_ptr member keeps the custom serializer alive. The new `ValuePtr(const std::atomic<T>*)` constructor (arithmetic/enum T) serializes with a relaxed load and carries the same schema identity (type_index_ = typeid(T)) as the plain scalar constructor, so a std::atomic<T> and a plain T produce identical bytes and compare equal via operator==. Plan 2 will register LoggedValue<T>::value_ (an std::atomic<T>) through it. Pre-existing bug, not touched by this task: SerializeMe.hpp:558 has the memcpy arguments swapped for std::array of 1-byte-element types, which does not compile against a const array today (hence no std::array<uint8_t, N> test here). Plan 2 fixes it with its own golden test. Benchmark (release preset, dt_benchmark, --benchmark_min_time=0.5s), before (commit a851887, see docs/benchmarks/2026-09-baseline.md) / after: DT_Doubles/1000 5557 ns / 4836 ns cpu -> 3744 ns / 3041 ns cpu (allocs/op=2, unchanged) DT_PoseType/1000 11851 ns / 10827 ns cpu -> 8622 ns / 8293 ns cpu (allocs/op=2, unchanged) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A moved-from LoggedValue left the channel pointing at its old value_. createLoggedValue already returns a shared_ptr, which remains movable. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Default-constructed ValuePtr left serialize_fn_/size_fn_ null, so serialize()/getSerializedSize() called through a null function pointer (UB/segfault). Initialise both to no-op statics (serializeNone/sizeNone) so the default state is inert. Also move is_std_atomic into DataTamer::details and reference it as details::is_std_atomic<T>::value. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
using Mutex = std::shared_mutex and its <shared_mutex> include were unused in this file; the mutex_ member is a plain std::mutex, and the real Mutex alias used elsewhere already lives in details/locked_reference.hpp. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The previous baseline was taken under load average 4.1 with CPU scaling on and no core pinning, and its DT_MultiSink numbers were internally inconsistent. Re-record from the unmodified library (a851887) with HEAD's benchmark sources, pinned to CPUs 0-5 (P-cores) via taskset, with 5 repetitions per micro-benchmark case. Note that item 3 (malloc-level AllocCounter) is BLOCKED, so allocs/op here still only counts operator new/delete, not the moodycamel queue's malloc calls. DT_MultiSink/1 vs /2 medians remain slightly non-monotonic after a retry; documented as a known limitation rather than iterated further, per the recording instructions. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
DummySink's public members (schemas, schema_names, snapshots_count, latest_snapshot) were replaced by mutex-protected accessors but the spec's public API delta table (§8) didn't record this source-breaking change; also update §6.3's DummySink wording to point at it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- WriteMutex: platform primitive isolated in details::PriorityInheritingMutex, removing six #if blocks; lockWithSpin checks the clock every 16 try_locks instead of every iteration (a clock read is comparable to the whole spin budget otherwise). - SnapshotPool::tryAcquire: wrap-around branch instead of modulo per slot. - ValuePtr: the generic constructor is constrained with enable_if so std::atomic<T>* can only match the atomic constructor (same idiom as the container overloads); dead memory_size_ member removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- tests/CMakeLists.txt: one DATATAMER_TEST_SOURCES list for both build flavours; the TSAN launcher goes through CROSSCOMPILING_EMULATOR so gtest_discover_tests keeps per-test entries (57 instead of 1 wrapped run). - benchmarks: CompileBenchmark() function, shared null_sink.hpp, one measureSnapshots() helper replacing four copies of the allocs/op loop, Threads::Threads instead of bare pthread, plain.size() hoisted out of the timed loop. - alloc_counter.cpp: one countedMalloc() shared by throwing and nothrow new. - DummySink: latestPayloadSize()/latestActiveMask() so tests stop copying the whole snapshot to read one number. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ions, sink PIMPL) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Shared between a channel and its LoggedValues so writer-side operations never touch the channel object (spec §2.2, §5.2). Not wired yet. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…atomic proxies ConstPtr/MutablePtr hold the exclusive transaction lock for their lifetime (spec §4.2). AtomicProxy/AtomicConstProxy are the write-back proxies scalar LoggedValues will return. The ROS2 sink's schema mutex becomes a plain std::mutex. Fixes a double-unlock in ConstPtr's move constructor. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…dState
set()/get() on scalars are single relaxed stores/loads; setEnabled() is
wait-free and works without the channel. Fixes the pre-existing data race
between LoggedValue::set() and takeSnapshot() (spec §4.1, §4.4, §5.2).
Also fixes a latent bug in the ScalarWriterRacesSnapshotCleanly test: the
"every byte identical" trick (x * 0x0101010101010101) only holds for x < 256;
once the tight writer loop pushes the counter past that, the multiplication
legitimately carries between byte positions and produces non-uniform bytes
unrelated to any tearing. Masked x to a single byte before broadcasting so
the invariant holds indefinitely.
Benchmarks (release, pinned to cores 0-5, idle machine):
Before (HEAD~0, i.e. pre-Task-3):
DT_LoggedValueSet/10_mean 405 ns items_per_second=26.6M/s
DT_LoggedValueSet/100_mean 6068 ns items_per_second=16.5M/s
DT_LoggedValueSet/1000_mean 58458 ns items_per_second=17.2M/s
DT_SnapshotWithWriter_mean 4370 ns allocs/op=2
rt_latency --values 1000 --sinks 1 --writers 2 --seconds 10:
p50=13780 ns p99=39588 ns p99.9=51441 ns max=313599 ns
After (this commit):
DT_LoggedValueSet/10_mean 12.1 ns items_per_second=830M/s
DT_LoggedValueSet/100_mean 128 ns items_per_second=782M/s
DT_LoggedValueSet/1000_mean 2022 ns items_per_second=495M/s
DT_SnapshotWithWriter_mean 4267 ns allocs/op=2
rt_latency --values 1000 --sinks 1 --writers 2 --seconds 10:
p50=11954 ns p99=21879 ns p99.9=24672 ns max=27389 ns
DT_LoggedValueSet/100 is up ~47x (16.5M/s -> 782M/s). The writer-contention
harness's tail collapses: max latency drops from 313599 ns to 27389 ns,
consistent with the removal of lock convoying between LoggedValue::set() and
takeSnapshot().
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
They now return write-back / copy proxies, not locked references; set() and get() express the same thing without the surprise (spec §4.1, §8). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Add scopedWrite and blocking contention counters, keep scalar stores wait-free, and make non-scalar set/get reuse the current thread's transaction. Size only enabled values and release the write lock if a serializer throws. Use an allocation-free guard chain instead of the plan's unsafe fixed-depth fallback. All debug, ASAN/UBSAN, TSAN and release suites pass (privileged PI check skipped). On this i7-13700H/GCC15 host, pinned transaction-writer snapshot median is 3830 ns; the 1000-value/2-writer transaction harness records p50 29687 ns, p99 85285 ns, max 196361 ns, 268 blocking acquisitions, max wait 74188 ns. Historical baseline uses different hardware; these are not controlled speedups. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Preserve public constructor inputs by delegating normalized ROS interfaces to an out-of-line constructor. Stop the worker before private state destruction and protect the ROS schema-changed flag with the existing schema mutex. Pin both sink layouts to one pointer beyond DataSinkBase. Validated debug, ASAN/UBSAN, TSAN and release suites, seven ROS/ABI runtime tests, and MCAP example output (1500 messages, two channels, doctor exit 0). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Add the standalone mutex and pool measurements carried over from Plan 1. Reject missing, malformed and invalid latency-harness operands with a tested exit status instead of crashing. Export Conan benchmark sources and forward the build option. Probe mutex ownership from another thread in proxy tests. Final review passes. Debug, ASAN/UBSAN and TSAN pass 88 tests each; Release passes 89 including the invalid-CLI regression. Privileged PI checks skip. All ten new benchmark cases execute; Conan inspect and recipe syntax pass. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Adds a deterministic overlap test: the reader is parked on the write mutex after loading the sink links, removal is observed not completing, and the push lands on the still-alive unpublished link. Records the pinned A/B showing no Plan 3 -> final latency regression and no gain from dropping the spin, plus the resulting decisions on the other follow-ups. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The paused serializer already parks the reader inside its epoch, so the procfs/gettid variant was redundant. One 50 ms negative wait on the removal future proves the overlap portably. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Legacy tests slept a fixed time hoping the sink worker had delivered; four failed under 16 CPU hogs. DummySink::flush() drains through the base class and replaces every sleep. Also fixes a lost-wakeup race in the pool contention test, counts over-aligned operator new (PoolSlot is alignas(64)) so zero-allocation assertions can see pool re-creation, suppresses the fence-based TSAN false positive in the vendored queue, turns several assertions that could not fail into real ones, and adds coverage for proxy move semantics, ValuePtr::detach, shape inequality and MCAP double stop. MCAPSink: stopRecording() and addChannel() no longer dereference a null writer after the recording was already stopped. README: replace the refuted median-regression paragraph with the measured A/B and drop a duplicated paragraph. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Same-machine, pinned, interleaved: hot-loop takeSnapshot() cost drops to about a third; the 1 kHz probe shows zero caller-thread allocations (was 2 per sink) and a lower p99.9 with four sinks, medians within noise. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
facontidavide
force-pushed
the
lockfree-frontend
branch
from
September 12, 2026 13:14
3194cbc to
2049aba
Compare
# Conflicts: # data_tamer_cpp/tests/CMakeLists.txt
…ense the README docs/superpowers (specs, plans, review notes) and the <cstdint> addition to the vendored MCAP header are removed from this branch; benchmark notes no longer link into the removed material. The README section on the real-time contract is reduced to the essentials, with pointers to the changelog and benchmarks. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…g it did On a two-core CI runner the snapshot loop finished its 2000 calls before the writer thread was scheduled, and the progress assertion added to catch a disarmed race failed for the wrong reason. Snapshot until the writer has iterated at least once, with a 10 s deadline. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…th atomics Production: - SnapshotPool already counts exhaustion; the channel no longer keeps a second counter. Pimpl::schema_hash mirror of schema.hash removed. - The mask-dirty handshake loads before exchanging, so the common snapshot path pays a plain SC load instead of a locked RMW; EpochGuard lives next to waitQuiescent(); unregister uses a setRegistered(RegistrationID) mirror of setEnabled. - WriteMutex: priority inheritance is keyed on _POSIX_THREAD_PRIO_INHERIT rather than __linux__ (QNX and other POSIX RTOSes qualify); the spin loop pauses between attempts instead of stealing the owner's cache line; the per-include pragma is gone. - ConstPtr/MutablePtr hold a std::unique_lock instead of hand-written move/unlock logic (about 90 lines fewer); the no-op mutex() on the atomic proxies is removed. - DataSinkBase: one kDefaultQueueCapacity; admission uses fetch_add instead of a CAS loop; the worker drains everything queued per wake-up; the thread-local behind retainSnapshot() is documented as a source-compat concession. Eight-sink cap documented on addDataSink. - DummySink: dead schemaName() and its map removed. MCAPSink reads the clock only when rollover is enabled. Tests and benchmarks: - Writer-race tests share one helper; tautological stats/getter and deprecation tests removed; DrainingDummySink replaced by flush(). - Google Benchmark allocation scope moved outside the timed region. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Sep 12, 2026
Closed
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.
Summary
Replaces the snapshot front end with a design that never allocates on the snapshot thread after warm-up and never makes it wait for control work.
LoggedValue::set()/get()are wait-free relaxed atomics. Non-scalar values andscopedWrite()transactions share onePTHREAD_PRIO_INHERITmutex with the snapshot thread.takeSnapshot()freezes the schema and reserves 64 slots. Later snapshots serialize directly into a free slot; sinks receive a reference-countedSnapshotRefinstead of a copy.Full write-up with diagram, measurements, review verdicts and the protobuf direction: https://claude.ai/code/artifact/e7f74b15-45a3-4ac5-93cd-7f132cceb882
Results
takeSnapshot(), 501 fields / 8 KB, 2 sinks, 2 transaction writers, pinnedA 60 s compressed MCAP soak at 1 kHz: zero allocations, zero drops, 60,010 records validated by the
mcapCLI.API changes
scopedWrite()new;Mutexnow aliases the exclusiveWriteMutex(nolock_shared()).getMutablePtr()/getConstPtr()deprecated in favor ofset()/get();LoggedValueno longer movable.setPoolCapacity(),setPayloadCapacity(),setStrictMode(),stats(),poolExhausted(),droppedSnapshots(sink),DataSinkBase::storeErrors().DataSinkBase(queue_capacity);pushSnapshotremoved; derived destructors must callstopThread(); callback-onlyretainSnapshot().MCAPSinkandROS2PublisherSinkare Pimpl now (ABI change).finishQueueAndStop()no longer polls; stopping twice is harmless.See
CHANGELOG.rstand the README "Real-time snapshot contract" section for the full list.Test hardening
Running the suite under 16 CPU hogs exposed ~25 legacy tests that slept a fixed time hoping the sink worker had delivered; four failed under load. All now drain through
DummySink::flush(). Also fixed: a lost-wakeup race in the pool contention test, an allocation counter blind to over-alignedoperator new(the pool slot array isalignas(64)), a TSAN false positive from the vendored queue's fences (file-scoped suppression), several assertions that could not fail, and missing coverage for proxy move semantics,ValuePtr::detach()and removal overlapping a reader inside its epoch.One production defect found in the process:
MCAPSink::stopRecording()/addChannel()dereferenced a null writer after the recording was already stopped.Verification
CAP_SYS_NICE).removeDataSink()fails the overlap test on the first run.Follow-ups considered and closed
Recorded in
docs/superpowers/reviews/2026-09-11-frontend-refactoring-followups.md: the earlier "median regression" does not reproduce; immediate blocking instead of the 2 µs spin doubles blocking acquisitions for no gain; weaker atomic ordering has nothing to win on x86-64; the stop/join/drain/restart backend needs more lifecycle code than it removes.Proposed next direction (not in this PR): protobuf output as a transcoding mode inside
MCAPSink, keeping the real-time path unchanged.🤖 Generated with Claude Code