Svc/TlmChan: track updated buckets in a set so Run scales with updated channels, not table size by bitWarrior · Pull Request #5994 · nasa/fprime

bitWarrior · 2026-09-17T16:12:49Z


*Related Issue(s)*	#5253, #5144
*Has Unit Tests (y/n)*	y
*Documentation Included (y/n)*	y (SDD functional description and change log)
*Generative AI was used in this contribution (y/n)*	AI

Change Description

Run_handler made two full passes over every hash bucket each cycle: one under the component mutex to clear the newly active buffer's updated flags, and one over the inactive buffer to find entries to send. The cost was O(TLMCHAN_HASH_BUCKETS) per cycle regardless of how many channels had changed, and TlmRecv callers blocked for the full clearing pass.

Each buffer now keeps an Fw::RedBlackTreeSet<FwChanIdType, TLMCHAN_HASH_BUCKETS> of the bucket indices whose updated flag is set:

TlmRecv_handler inserts the bucket index when it sets the flag. Inserting an index already present is a no-op, so repeated updates of one channel within a cycle do not grow the set. The set holds one slot per bucket, so insertion cannot fail.
Run_handler walks the newly active buffer's set to clear flags on the swap (then empties the set), and walks the inactive buffer's set to serialize entries. Both passes are O(m) in the number of updated channels.
The set iterates in ascending bucket order, matching the former full-table scan, so packet contents and ordering are unchanged. The per-entry guarded flag clear and the deferred-entry semantics of TLMCHAN_MAX_ENTRIES_PER_RUN are preserved: deferred entries stay in the set and are cleared at the next swap, exactly as before.

Memory cost: two sets of TLMCHAN_HASH_BUCKETS nodes, measured at about 56 KB total with the default 500 buckets.

Rationale

Addresses #5144 (closed as superseded by #5253). #5253 proposes replacing the inline hash map with a RedBlackTreeSet; this PR is a smaller first step that uses the same data structure to remove the per-cycle scans while leaving the existing hash table, configuration constants, and lookup path untouched. It can be presented on #5253 as an incremental option, or superseded by the full replacement later. The earlier attempt in #5246 used an ArraySet with O(m) insertion; the tree set gives O(log m) insertion and ordered iteration.

Testing/Review Recommendations

Svc/TlmChan unit tests: 15 pass, including two new tests. UpdatedSetTracking verifies the set mirrors the updated flags, that rewriting a channel does not grow it, that Run drains in bucket order, and that the swap empties the set and clears every remaining flag. UpdatedSetSparseUpdate fills all buckets, drains them, then updates one channel and verifies exactly one set entry and one packet holding only that channel.
The default config makes deferral impossible, so the suite was also run with TLMCHAN_MAX_ENTRIES_PER_RUN temporarily lowered to 100. The existing ProcGuardTest fired with 400 deferred entries and the new tests confirmed deferred flags are cleared at the swap. The config change was not committed.
Overnight soak: a two-deployment reference project (TlmChan as CdhCore.tlmSend on a 1 Hz rate group, default config) ran under fprime-gds for 11.75 hours. Resident memory, thread count, and file descriptor count were flat across 142 five-minute samples; CPU time grew linearly; 8 million channel samples were logged; no FATAL, assert, TlmChanEpochProcessingCapReached, or TlmChanBucketPoolExhausted events.
Review focus: the swap-time clearing loop in Run_handler now runs under the mutex over the set rather than the table, and bucketNo (previously marked "for testing") is now the set key. TlmSet gained a member with a constructor, which is fine since TlmEntry already holds non-trivial members.

Future Work

Switch TlmChan Storage for RedBlackTree Set #5253 remains open for replacing the hash table itself with a RedBlackTreeSet, which would also remove the per-slot chain walks in TlmRecv and TlmGet.

AI Usage (see policy)

Claude Code was used to analyze the issue history, implement the change, write the unit tests and SDD update, run the soak, and draft this description, under human direction and review. IAMAI

🤖 Generated with Claude Code

…s, not table size Run_handler made two full passes over every hash bucket each cycle: one under the component mutex to clear the newly active buffer's updated flags, and one over the inactive buffer to find entries to send. The cost was O(TLMCHAN_HASH_BUCKETS) per cycle regardless of how many channels had changed, and TlmRecv callers blocked for the full clearing pass. Each buffer now keeps an Fw::RedBlackTreeSet of the bucket indices whose updated flag is set. TlmRecv inserts the index when it sets the flag (a no-op for repeats within a cycle). Run walks that set to clear flags on the swap and again to serialize the inactive buffer, so both passes cost O(m) in the number of updated channels. Iteration is in ascending bucket order, matching the former scan, so packet contents are unchanged. The per-entry guarded flag clear and the deferred-entry semantics of TLMCHAN_MAX_ENTRIES_PER_RUN are preserved. Cost: two sets of TLMCHAN_HASH_BUCKETS nodes, about 56 KB at the default of 500 buckets. Tests: add UpdatedSetTracking (set mirrors the flags, repeats do not grow it, drain order, swap empties it and clears deferred flags) and UpdatedSetSparseUpdate (after every bucket is populated, one updated channel yields one set entry and one packet). Verified with the default cap and with TLMCHAN_MAX_ENTRIES_PER_RUN lowered to 100 so the deferred path is exercised. Relates to nasa#5253 and nasa#5144 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

github-actions · 2026-09-17T16:32:18Z

Coverage report — base `devel`

Overall (line): 85.12% → 85.14% (+0.02)
Regression threshold: 0.50% (line).

Regressions

(none over threshold)

Modules changed

Module	Line	Δ	Function	Δ	Branch	Δ
`Os/Posix`	70.64	-0.39	87.50	+0.00	50.67	-0.45
`Fw/DataStructures`	97.60	-0.36	97.14	+0.00	82.12	-0.54
`Os/Generic`	90.14	-0.10	89.13	+0.00	75.10	+0.00
`Svc/TlmChan`	81.12	+1.45	85.71	+0.00	71.53	+3.53

New modules

Module	Line	Function	Branch
`Fw/Prm`	0.00	0.00	0.00

Modules without UTs

CFDP/Checksum/GTest, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/Ports/DataTypes, Drv/PosixUartDriver, FppTestProject/FppTest/topology/async, FppTestProject/FppTest/topology/components/Comp, FppTestProject/FppTest/topology/components/Framework, FppTestProject/FppTest/topology/components/Receiver, FppTestProject/FppTest/topology/components/Sender, FppTestProject/FppTest/topology/guarded, FppTestProject/FppTest/topology/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Obj, Fw/Port, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Ccsds/Types, Svc/Ccsds/Utils, Svc/FatalHandler, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComCcsdsSdls, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/DpCompression, Svc/Subtopologies/FileHandling, Svc/Subtopologies/FileHandlingCfdp, Svc/Subtopologies/FileHandlingCfdp/FileHandlingCfdpConfig, TestDeploymentsProject/Ref/DpDemo, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestConfigDeployment, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestDuplicateDirective, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestLinkDepends, cmake/test/data/TestDeployment/TestRelative, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent

lestarch-autobot · 2026-09-17T18:27:13Z

+    // Processed entries had their flags cleared; the set is retained until the
+    // buffer is swapped back in, when any deferred flags are cleared with it
+    ASSERT_EQ(0u, this->countUpdatedFlags(0));
+    ASSERT_EQ(numUnique, set0.updated.getSize());


[Test Quality] suggestion Deferred-flag clearing at the swap is asserted in the doc-comment but never exercised.

With the default config (TLMCHAN_MAX_ENTRIES_PER_RUN == TLMCHAN_HASH_BUCKETS) no entry is ever deferred, so the new set-walk clearing path only ever sees buckets whose flags are already false; ProcGuardTest also skips. A flag left set by a deferred entry is the one case where walking the set (rather than the table) could miss a bucket, and countUpdatedFlags(0) == 0 after the swap-back would catch it if a flag is re-armed here. (best-effort fix; verify before applying)

Suggested change

ASSERT_EQ(numUnique, set0.updated.getSize());

ASSERT_EQ(numUnique, set0.updated.getSize());

// Mimic an entry deferred by the per-run cap: flag set, bucket still in the set

set0.buckets[*set0.updated.begin()].updated = true;

ASSERT_EQ(1u, this->countUpdatedFlags(0));

[Test Quality] Fixed in 47fe24b.

lestarch-autobot · 2026-09-17T18:27:25Z

-The `Svc::TlmChan` component has an input port `TlmRecv` that receives channel updates from other components in the system. These calls from the other components are made by the component implementation classes, but the generated code in the base classes takes the type specific channel value and serializes it, then makes the call to the output port. The `Svc::TlmChan` component can then store the channel value as generic data. The channel values are stored in an internal double-buffered table, and a flag is set when a new value is written to the channel entry.
+The `Svc::TlmChan` component has an input port `TlmRecv` that receives channel updates from other components in the system. These calls from the other components are made by the component implementation classes, but the generated code in the base classes takes the type specific channel value and serializes it, then makes the call to the output port. The `Svc::TlmChan` component can then store the channel value as generic data. The channel values are stored in an internal double-buffered table, and a flag is set when a new value is written to the channel entry. Each buffer also keeps a set of the bucket indices whose flag is set (an `Fw::RedBlackTreeSet` sized to the bucket count). `TlmRecv` inserts the bucket index when it sets the flag; inserting an index that is already present is a no-op, so repeated updates of one channel within a cycle do not grow the set.
+
+The `Run` handler swaps the active buffer under the component mutex, then clears the flags of only the buckets recorded in the newly active buffer's set and empties that set. It then walks the inactive buffer's set, in ascending bucket order, to serialize and send the updated channels. Both steps therefore cost time proportional to the number of channels updated in a cycle rather than to `TLMCHAN_HASH_BUCKETS`, and the mutex is held for the shorter of the two.


[Documentation] suggestion SDD says the mutex is held "for the shorter of the two" passes; the code holds it only for the clearing pass, whose size is not bounded by the send pass.

In Run_handler the lock covers the swap plus the walk over the newly active buffer's set (buckets updated in the previous cycle, including deferred ones); the send pass over the inactive buffer's set (this cycle's updates) runs unlocked. Neither set is necessarily the shorter one, so the sentence misstates the locking contract.

Suggested change

The `Run` handler swaps the active buffer under the component mutex, then clears the flags of only the buckets recorded in the newly active buffer's set and empties that set. It then walks the inactive buffer's set, in ascending bucket order, to serialize and send the updated channels. Both steps therefore cost time proportional to the number of channels updated in a cycle rather than to `TLMCHAN_HASH_BUCKETS`, and the mutex is held for the shorter of the two.

The `Run` handler swaps the active buffer under the component mutex, then clears the flags of only the buckets recorded in the newly active buffer's set and empties that set. It then walks the inactive buffer's set, in ascending bucket order, to serialize and send the updated channels. Both steps therefore cost time proportional to the number of channels updated in a cycle rather than to `TLMCHAN_HASH_BUCKETS`; the mutex is held only for the clearing pass, whose length is the number of buckets updated in the previous cycle (those the buffer received before it was last swapped out).

[Documentation] Fixed in 47fe24b.

lestarch-autobot · 2026-09-17T18:27:25Z

 #### 3.2 Functional Description

-The `Svc::TlmChan` component has an input port `TlmRecv` that receives channel updates from other components in the system. These calls from the other components are made by the component implementation classes, but the generated code in the base classes takes the type specific channel value and serializes it, then makes the call to the output port. The `Svc::TlmChan` component can then store the channel value as generic data. The channel values are stored in an internal double-buffered table, and a flag is set when a new value is written to the channel entry.
+The `Svc::TlmChan` component has an input port `TlmRecv` that receives channel updates from other components in the system. These calls from the other components are made by the component implementation classes, but the generated code in the base classes takes the type specific channel value and serializes it, then makes the call to the output port. The `Svc::TlmChan` component can then store the channel value as generic data. The channel values are stored in an internal double-buffered table, and a flag is set when a new value is written to the channel entry. Each buffer also keeps a set of the bucket indices whose flag is set (an `Fw::RedBlackTreeSet` sized to the bucket count). `TlmRecv` inserts the bucket index when it sets the flag; inserting an index that is already present is a no-op, so repeated updates of one channel within a cycle do not grow the set.


[Documentation] could fix New per-buffer RedBlackTreeSet storage changes the memory cost of TLMCHAN_HASH_BUCKETS, but the SDD's sizing guidance (§3.5.1: buckets >= channel count) does not mention it.

The PR description quotes ~56 KB for two sets at the default 500 buckets; deployments that size buckets per §3.5.1 need that figure in the SDD (or in TlmChanImplCfg.hpp next to TLMCHAN_HASH_BUCKETS) to budget memory.

Suggested change

The `Svc::TlmChan` component has an input port `TlmRecv` that receives channel updates from other components in the system. These calls from the other components are made by the component implementation classes, but the generated code in the base classes takes the type specific channel value and serializes it, then makes the call to the output port. The `Svc::TlmChan` component can then store the channel value as generic data. The channel values are stored in an internal double-buffered table, and a flag is set when a new value is written to the channel entry. Each buffer also keeps a set of the bucket indices whose flag is set (an `Fw::RedBlackTreeSet` sized to the bucket count). `TlmRecv` inserts the bucket index when it sets the flag; inserting an index that is already present is a no-op, so repeated updates of one channel within a cycle do not grow the set.

The `Svc::TlmChan` component has an input port `TlmRecv` that receives channel updates from other components in the system. These calls from the other components are made by the component implementation classes, but the generated code in the base classes takes the type specific channel value and serializes it, then makes the call to the output port. The `Svc::TlmChan` component can then store the channel value as generic data. The channel values are stored in an internal double-buffered table, and a flag is set when a new value is written to the channel entry. Each buffer also keeps a set of the bucket indices whose flag is set (an `Fw::RedBlackTreeSet` sized to the bucket count). `TlmRecv` inserts the bucket index when it sets the flag; inserting an index that is already present is a no-op, so repeated updates of one channel within a cycle do not grow the set. Each set holds `TLMCHAN_HASH_BUCKETS` statically allocated nodes, so raising the bucket count in `TlmChanImplCfg.hpp` now also grows the two updated sets (about 56 KB total at the default of 500 buckets).

[Operational] Concur — also in scope for operational consequences: static RAM grows ~112 B per bucket on 64-bit targets (2 sets × (40 B node + 8 B free index)), ~56 KB at the default 500, scaling linearly with TLMCHAN_HASH_BUCKETS; deployments must re-budget when sizing buckets per §3.5.1.

[Design] could fix Concur (resource-margin-erosion): two statically sized RedBlackTreeSet nodes arrays (~56 KB at 500 buckets) roughly double TlmChan's static footprint with no independent sizing knob; the cost belongs in the SDD/config guidance as suggested above.

[Design] Fixed in 47fe24b (memory cost now stated in SDD §3.5.1 and TlmChanImplCfg.hpp).

[Documentation] Fixed in 47fe24b.

[Operational] Fixed in 47fe24b (memory cost documented in SDD §3.5.1 and TlmChanImplCfg.hpp; ~112 B/bucket verified: 48 B node + 8 B free index per set).

lestarch-autobot · 2026-09-17T18:27:31Z

+    const TlmSet& inactiveSet = this->m_tlmEntries[1 - static_cast<U8>(this->m_activeBuffer)];
+    for (const FwChanIdType bucketNo : inactiveSet.updated) {
+        FW_ASSERT(bucketNo < TLMCHAN_HASH_BUCKETS, static_cast<FwAssertArgType>(bucketNo));
+        TlmEntry* p_entry = &this->m_tlmEntries[1 - static_cast<U8>(this->m_activeBuffer)].buckets[bucketNo];


[Maintainability] suggestion maint-code-duplication: inactiveSet is introduced on the line above and then bypassed: p_entry re-derives the same buffer via the full m_tlmEntries[1 - ...] index expression.

Two spellings of "the inactive buffer" in one loop must be kept in sync by hand; if a future edit points inactiveSet elsewhere, p_entry silently stays on the old expression. Dropping const (the loop writes p_entry->updated) lets the alias be used for both.

Suggested change

const TlmSet& inactiveSet = this->m_tlmEntries[1 - static_cast<U8>(this->m_activeBuffer)];

for (const FwChanIdType bucketNo : inactiveSet.updated) {

FW_ASSERT(bucketNo < TLMCHAN_HASH_BUCKETS, static_cast<FwAssertArgType>(bucketNo));

TlmEntry* p_entry = &this->m_tlmEntries[1 - static_cast<U8>(this->m_activeBuffer)].buckets[bucketNo];

TlmSet& inactiveSet = this->m_tlmEntries[1 - static_cast<U8>(this->m_activeBuffer)];

for (const FwChanIdType bucketNo : inactiveSet.updated) {

FW_ASSERT(bucketNo < TLMCHAN_HASH_BUCKETS, static_cast<FwAssertArgType>(bucketNo));

TlmEntry* p_entry = &inactiveSet.buckets[bucketNo];

[Maintainability] Fixed in 47fe24b.

lestarch-autobot · 2026-09-17T18:27:31Z

    // which preserves Run_handler's bounded execution-time guarantee.
-    for (U32 entry = 0; entry < TLMCHAN_HASH_BUCKETS; entry++) {
-        this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)].buckets[entry].updated = false;
+    TlmSet& newActiveSet = this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)];


[Maintainability] could fix maint-unclear-naming: newActiveSet / inactiveSet name a TlmSet (a whole double-buffer half), while the PR's new UpdatedSet is the member .updated inside it.

"Set" now means two things in this function, and newActiveSet.updated reads like the TlmEntry::updated flag rather than a container. newActiveBuffer / inactiveBuffer (matching m_activeBuffer and the comments' own wording) would keep the two apart.

[Maintainability] Fixed in 47fe24b.

lestarch-autobot · 2026-09-17T18:27:31Z

+    // Walk only the buckets updated since this buffer was last drained.  The
+    // set iterates in ascending bucket order, matching the order of the
+    // former full-table scan.  The inactive buffer receives no writes, so
+    // the set is stable while it is iterated.


[Maintainability] could fix maint-misleading-comment: Comment describes the code relative to history ("matching the order of the former full-table scan").

Once the PR merges there is no "former" scan to compare against; the next reader can only resolve the reference via git blame. State the current property only, and keep it to two lines.

Suggested change

// Walk only the buckets updated since this buffer was last drained. The

// set iterates in ascending bucket order, matching the order of the

// former full-table scan. The inactive buffer receives no writes, so

// the set is stable while it is iterated.

// Walk only the buckets updated since this buffer was last drained, in

// ascending bucket order. The inactive buffer receives no writes meanwhile.

[Maintainability] Fixed in 47fe24b.

lestarch-autobot · 2026-09-17T18:27:31Z

+    // Record the bucket in the active buffer's updated set. Inserting an index
+    // that is already present is a no-op, so repeated updates of the same
+    // channel within a cycle do not grow the set. The set holds one slot per
+    // bucket, so insertion cannot run out of room.


[Maintainability] could fix maint-misleading-comment: Four-line comment restating what the UpdatedSet Doxygen in TlmChan.hpp and the FW_ASSERT below already say.

Two copies of the same rationale drift independently (the .hpp block already carries it); one or two lines here is enough.

Suggested change

// Record the bucket in the active buffer's updated set. Inserting an index

// that is already present is a no-op, so repeated updates of the same

// channel within a cycle do not grow the set. The set holds one slot per

// bucket, so insertion cannot run out of room.

// Track the bucket in the active buffer's updated set; re-inserting an

// existing index is a no-op and the set has one slot per bucket.

[Maintainability] Fixed in 47fe24b.

lestarch-autobot · 2026-09-17T18:28:43Z

-The `Svc::TlmChan` component has an input port `TlmRecv` that receives channel updates from other components in the system. These calls from the other components are made by the component implementation classes, but the generated code in the base classes takes the type specific channel value and serializes it, then makes the call to the output port. The `Svc::TlmChan` component can then store the channel value as generic data. The channel values are stored in an internal double-buffered table, and a flag is set when a new value is written to the channel entry.
+The `Svc::TlmChan` component has an input port `TlmRecv` that receives channel updates from other components in the system. These calls from the other components are made by the component implementation classes, but the generated code in the base classes takes the type specific channel value and serializes it, then makes the call to the output port. The `Svc::TlmChan` component can then store the channel value as generic data. The channel values are stored in an internal double-buffered table, and a flag is set when a new value is written to the channel entry. Each buffer also keeps a set of the bucket indices whose flag is set (an `Fw::RedBlackTreeSet` sized to the bucket count). `TlmRecv` inserts the bucket index when it sets the flag; inserting an index that is already present is a no-op, so repeated updates of one channel within a cycle do not grow the set.
+
+The `Run` handler swaps the active buffer under the component mutex, then clears the flags of only the buckets recorded in the newly active buffer's set and empties that set. It then walks the inactive buffer's set, in ascending bucket order, to serialize and send the updated channels. Both steps therefore cost time proportional to the number of channels updated in a cycle rather than to `TLMCHAN_HASH_BUCKETS`, and the mutex is held for the shorter of the two.


[Operational] must fix ops-doc-reality: the claim that both Run steps cost O(updated channels) is false — newActiveSet.updated.clear() (TlmChan.cpp:317) is O(TLMCHAN_HASH_BUCKETS) and runs under the mutex.

Fw::RedBlackTreeSetOrMapImpl::clear() pushes every one of the C=500 free-node indices back onto its stack (one Stack::push + FW_ASSERT each) regardless of how many were used, so the mutex-held swap section still performs ~500 operations per cycle — the same order as the 500-flag loop it replaced. Consequence: the PR's stated benefit for TlmRecv callers (guarded port, blocked for the swap on every producer thread) does not materialize; worst-case blocking at the swap is unchanged, and Run_handler keeps an O(500) per-cycle floor even with zero updated channels. Smallest remedy: correct the SDD claim (below) and the matching comment in Run_handler; a code alternative (remove elements one by one while walking, O(m log m)) is slower than clear() at high m, so documentation is the right fix.

Suggested change

The `Run` handler swaps the active buffer under the component mutex, then clears the flags of only the buckets recorded in the newly active buffer's set and empties that set. It then walks the inactive buffer's set, in ascending bucket order, to serialize and send the updated channels. Both steps therefore cost time proportional to the number of channels updated in a cycle rather than to `TLMCHAN_HASH_BUCKETS`, and the mutex is held for the shorter of the two.

The `Run` handler swaps the active buffer under the component mutex, then clears the flags of only the buckets recorded in the newly active buffer's set and empties that set. It then walks the inactive buffer's set, in ascending bucket order, to serialize and send the updated channels. The flag-clearing walk and the send pass cost time proportional to the number of channels updated in a cycle; emptying the set (`Fw::RedBlackTreeSet::clear`) rebuilds its free-node stack and therefore still costs `TLMCHAN_HASH_BUCKETS` operations (500 at the default) under the mutex every cycle, so the worst-case time a `TlmRecv` caller can be blocked at the swap is unchanged from the previous full-table clear, not reduced to the number of updated channels.

[Operational] Fixed in 47fe24b. The SDD now describes the element-wise drain and its logarithmic factor; the O(TLMCHAN_HASH_BUCKETS) claim no longer applies.

lestarch-autobot · 2026-09-17T18:29:11Z

+    TlmSet& newActiveSet = this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)];
+    for (const FwChanIdType bucketNo : newActiveSet.updated) {
+        FW_ASSERT(bucketNo < TLMCHAN_HASH_BUCKETS, static_cast<FwAssertArgType>(bucketNo));
+        newActiveSet.buckets[bucketNo].updated = false;
    }
+    newActiveSet.updated.clear();


[Design] must fix design-code-mismatch: RedBlackTreeSet::clear() is O(capacity), so this step under the mutex still costs O(TLMCHAN_HASH_BUCKETS), contradicting the SDD and PR intent.

Fw/DataStructures/RedBlackTreeSetOrMapImpl.hpp clear() resets the root and then pushes all capacity node indices back onto the free stack (500 pushes at default sizing, each bounds-checked and asserted). The SDD (§ Run handler, and the changelog line) states both steps are proportional to the number of updated channels, and #5144's motivation was the mutex-held clearing pass blocking TlmRecv callers. As written the locked pass trades a 500-iteration flag scan for a 500-iteration free-list rebuild; only the unlocked send pass is actually O(m). Either drain the set element-wise (O(m log m), below) or amend the SDD/description to state that the locked pass remains O(TLMCHAN_HASH_BUCKETS).

Suggested change

TlmSet& newActiveSet = this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)];

for (const FwChanIdType bucketNo : newActiveSet.updated) {

FW_ASSERT(bucketNo < TLMCHAN_HASH_BUCKETS, static_cast<FwAssertArgType>(bucketNo));

newActiveSet.buckets[bucketNo].updated = false;

}

newActiveSet.updated.clear();

TlmSet& newActiveSet = this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)];

while (newActiveSet.updated.getSize() > 0) {

const FwChanIdType bucketNo = *newActiveSet.updated.begin();

FW_ASSERT(bucketNo < TLMCHAN_HASH_BUCKETS, static_cast<FwAssertArgType>(bucketNo));

newActiveSet.buckets[bucketNo].updated = false;

const Fw::Success removeStatus = newActiveSet.updated.remove(bucketNo);

FW_ASSERT(removeStatus == Fw::Success::SUCCESS, static_cast<FwAssertArgType>(bucketNo));

}

[Design] Fixed in 47fe24b.

lestarch-autobot

Automated review summary (run 1)

Per-agent results

Agent	must fix	suggestion	could fix	future work	outstanding	Verdict
Security Vulnerabilities	0	0	0	0	0	Go
Supply Chain / Runner Safety	0	0	0	0	0	Go
F Prime C/C++ Design	0	0	0	0	0	Go
Documentation Currency	0	1	1	0	2	Go
Design	1	0	1	0	2	No-Go
Architecture	0	0	0	0	0	Go
Test Quality	0	1	0	0	1	Go
Correctness	0	0	0	0	0	Go
Operational	1	0	1	0	2	No-Go
Maintainability	0	1	3	0	4	Go
CI safety	—	—	—	—	—	Go
Totals	2	3	6	0	11	No-Go

Supply-chain surfaces

Surface	Outstanding
Dependencies	clean
Vendored / submodule	clean
Build / test infrastructure	clean
Workflows / actions / scripts	clean
Generator output	clean
Prompt-injection	clean
Review-system integrity	clean

Outstanding must-fix items (2)

Design

RedBlackTreeSet::clear() in Run_handler is O(TLMCHAN_HASH_BUCKETS) under the mutex, contradicting the SDD/PR claim that the locked pass is O(updated channels) — drain element-wise or amend the SDD — #5994 (comment)

Operational

SDD Run handler text claims both steps cost O(updated channels); the mutex-held clear() still costs ~500 operations per cycle, so TlmRecv worst-case blocking at the swap is unchanged — correct the SDD (and matching comment) — #5994 (comment)

Merge readiness

Merge readiness: No-Go — Design and Operational each have 1 outstanding must-fix item (same root cause: clear() cost under the mutex; resolving one likely resolves both).

One clear() call stands between this telemetry set and a clean orbit — worth a look before liftoff.

…ment set memory cost Follow-up to the automated review on nasa#5994. Fw::RedBlackTreeSet::clear() rebuilds the set's free list by pushing all TLMCHAN_HASH_BUCKETS node indices, so the swap-time clearing pass was still O(buckets) under the component mutex, contradicting the SDD claim. Run_handler now drains the newly active buffer's set one element at a time, clearing each bucket's flag and removing it, so the locked pass is proportional to the number of updated buckets (with a logarithmic factor per removal). The loop is bounded by the bucket count and asserts the set is empty afterwards. Also from the review: - SDD Run-handler paragraph states the real locking contract: the mutex covers the swap and the drain, whose size is independent of the send pass. - SDD sizing guidance and TlmChanImplCfg.hpp note the static cost of the two updated sets (about 112 bytes per bucket on 64-bit targets, ~56 KB at 500). - Run_handler uses buffer aliases named as buffers, the send loop derives the entry from the same alias it iterates, and comments no longer restate the header or refer to the former scan. - UpdatedSetTracking re-arms a flag on a bucket still in the set before the swap, so the deferred-flag clearing path is exercised under the default config as well as the lowered-cap run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

lestarch-autobot · 2026-09-22T18:28:11Z

    // Clear the new active buffer's updated flags so it is clean for incoming
-    // writes.  Any entries that were deferred (skipped) in the previous cycle
-    // and still carry updated=true in this buffer are also cleared here.
-    // This is intentional: deferred entries are dropped rather than re-queued,
-    // which preserves Run_handler's bounded execution-time guarantee.
-    for (U32 entry = 0; entry < TLMCHAN_HASH_BUCKETS; entry++) {
-        this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)].buckets[entry].updated = false;
+    // writes.  Only the buckets recorded in its updated set can carry
+    // updated=true, so drain that set element by element rather than walking
+    // every bucket.  UpdatedSet::clear() is deliberately not used: it rebuilds
+    // the set's whole free list, which costs O(TLMCHAN_HASH_BUCKETS) under the
+    // mutex.  Entries deferred by the per-run cap in the previous cycle still
+    // carry updated=true here and are cleared too: deferred entries are
+    // dropped rather than re-queued, which preserves Run_handler's bounded
+    // execution-time guarantee.


[Maintainability] could fix maint-misleading-comment: nine-line comment; its deferred-entry rationale is already stated in the CPU-guard comment 30 lines below.

Two copies of "deferred entries are dropped at the swap" in one function drift independently. Keep the non-obvious part (why not clear()) and state the rest once.

Suggested change

// Clear the new active buffer's updated flags so it is clean for incoming

// writes. Any entries that were deferred (skipped) in the previous cycle

// and still carry updated=true in this buffer are also cleared here.

// This is intentional: deferred entries are dropped rather than re-queued,

// which preserves Run_handler's bounded execution-time guarantee.

for (U32 entry = 0; entry < TLMCHAN_HASH_BUCKETS; entry++) {

this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)].buckets[entry].updated = false;

// writes. Only the buckets recorded in its updated set can carry

// updated=true, so drain that set element by element rather than walking

// every bucket. UpdatedSet::clear() is deliberately not used: it rebuilds

// the set's whole free list, which costs O(TLMCHAN_HASH_BUCKETS) under the

// mutex. Entries deferred by the per-run cap in the previous cycle still

// carry updated=true here and are cleared too: deferred entries are

// dropped rather than re-queued, which preserves Run_handler's bounded

// execution-time guarantee.

// Drain the new active buffer's updated set, clearing each bucket's flag

// (entries deferred last cycle are dropped here, not re-queued). Removing

// element-wise keeps the mutex hold O(m log m); clear() costs O(TLMCHAN_HASH_BUCKETS).

lestarch-autobot · 2026-09-22T18:28:38Z

+    // dropped rather than re-queued, which preserves Run_handler's bounded
+    // execution-time guarantee.
+    TlmSet& newActiveBuffer = this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)];
+    for (FwSizeType drained = 0; (drained < TLMCHAN_HASH_BUCKETS) && (newActiveBuffer.updated.getSize() > 0);


[Operational] could fix ops-timing-resource (judgment call): at the dense extreme the mutex-held drain is now slower than both the devel flag sweep and clear().

Each iteration does begin() (leftmost descent from the root, O(log m)) plus remove() (find + red-black delete with recolor/rotations, O(log m)); with m buckets updated in a cycle the locked section is O(m log m). When every channel updates every cycle (m ≈ TLMCHAN_HASH_BUCKETS = 500, a common steady-state for rate-group-driven telemetry), that is ~500 × 2·log2(500) ≈ 9,000 node visits plus rebalancing under the lock, versus 500 plain stores on devel (or 500 stack pushes with clear()) — roughly an order of magnitude more time during which every TlmRecv producer (guarded port) is blocked at the swap. The PR trades a lower best case (sparse updates) for a higher worst case, and TLMCHAN_MAX_ENTRIES_PER_RUN does not bound the drain because deferred buckets stay in the set. The SDD's "logarithmic factor" note is accurate but does not tell an operator that the worst-case hold time went up. Smallest remedy: state the m ≈ N worst case explicitly in the SDD timing paragraph and the TLMCHAN_HASH_BUCKETS config comment so rate-group budgets are set against it; optionally use a hybrid drain (iterate the set to clear flags, then clear()) when getSize() exceeds roughly TLMCHAN_HASH_BUCKETS / log2(TLMCHAN_HASH_BUCKETS), keeping the locked cost ≤ the devel baseline at every m.

lestarch-autobot

Automated review summary (run 2)

Per-agent results

Agent	must fix	suggestion	could fix	future work	outstanding	Verdict
Security Vulnerabilities	0	0	0	0	0	Go
Supply Chain / Runner Safety	0	0	0	0	0	Go
F Prime C/C++ Design	0	0	0	0	0	Go
Documentation Currency	0	1	1	0	0	Go
Design	1	0	1	0	0	Go
Architecture	0	0	0	0	0	Go
Test Quality	0	1	0	0	0	Go
Correctness	0	0	0	0	0	Go
Operational	1	0	2	0	1	Go
Maintainability	0	1	4	0	1	Go
CI safety	—	—	—	—	—	Go
Totals	2	3	8	0	2	Go

Since last run

Agent	resolved	newly added
Security Vulnerabilities	0	0
Supply Chain / Runner Safety	0	0
F Prime C/C++ Design	0	0
Documentation Currency	2	0
Design	2	0
Architecture	0	0
Test Quality	1	0
Correctness	0	0
Operational	2	1
Maintainability	4	1

Duplicates consolidated this run: 0 (threads closed by the §5h post-pass)

Supply-chain surfaces

Surface	Outstanding
Dependencies	clean
Vendored / submodule	clean
Build / test infrastructure	clean
Workflows / actions / scripts	clean
Generator output	clean
Prompt-injection	clean
Review-system integrity	clean

Merge readiness

Merge readiness: Go — all 10 reviewers completed with zero outstanding must-fix; both run-1 must-fix items (clear() cost under the mutex) are fixed by the element-wise drain in 47fe24b. Two non-blocking could-fix threads remain open (Operational, Maintainability). Core maintainers @LeStarch and @thomas-bc have been requested for human review.

The bucket set now drains in O(updated) — telemetry is cleared for the next burn.

lestarch-autobot self-requested a review September 17, 2026 16:53

lestarch-autobot reviewed Sep 17, 2026

View reviewed changes

lestarch-autobot suggested changes Sep 17, 2026

View reviewed changes

lestarch-autobot reviewed Sep 22, 2026

View reviewed changes

lestarch-autobot requested review from LeStarch and thomas-bc September 22, 2026 18:34

lestarch-autobot approved these changes Sep 22, 2026

View reviewed changes

-    ASSERT_EQ(numUnique, set0.updated.getSize());
+    ASSERT_EQ(numUnique, set0.updated.getSize());
+    // Mimic an entry deferred by the per-run cap: flag set, bucket still in the set
+    set0.buckets[*set0.updated.begin()].updated = true;
+    ASSERT_EQ(1u, this->countUpdatedFlags(0));

Conversation

bitWarrior commented Sep 17, 2026

Change Description

Rationale

Testing/Review Recommendations

Future Work

AI Usage (see policy)

Uh oh!

github-actions Bot commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report — base devel

Regressions

Modules changed

New modules

Modules without UTs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lestarch-autobot left a comment

Choose a reason for hiding this comment

Automated review summary (run 1)

Per-agent results

Merge readiness

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lestarch-autobot left a comment

Choose a reason for hiding this comment

Automated review summary (run 2)

Per-agent results

Merge readiness

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

github-actions Bot commented Sep 17, 2026 •

edited

Loading

Coverage report — base `devel`