perf(connect): cut ~1.3 s off the connect handshake (skip peek when multiFLEX on; pipeline subs)#3391
Conversation
Two independent latency wins on the radio connect handshake — the dominant stage of time-to-first-panadapter-frame on a warm launch. 1. Skip the 400 ms multiFLEX conflict peek when mf_enable=1. The conflict check is (!m_multiFlexEnabled && hasOthers), so when multiFLEX is enabled the radio explicitly allows multiple GUI clients and no conflict is possible — the fixed QTimer::singleShot(400) window is pure dead time. Proceed immediately once "sub client all" returns and m_multiFlexEnabled is true (mf_enable arrives in the radio status burst from the preceding "sub radio all"). The mf-disabled path keeps the original 400 ms wait, so conflict detection is unchanged where it matters. onConnected -> first pan stream id: ~851 ms -> ~423 ms. 2. Pipeline the first subscription batch. "sub slice all -> ... -> sub xvtr all" was one nested sendCmd per command, each awaiting its R response. The radio processes TCP commands in send order and the *second* sub batch in this same function (sub tnf/dax/codec/ ...) already fires without waiting, so the per-command serialization was gratuitous (~11 round trips, ~0.7 s on a LAN). Flattened to back-to-back fire-and-forget. first pan stream id -> subscriptions ACKed: ~985 ms -> ~71 ms. Combined handshake (TCP connected -> subscriptions ACKed): ~1810 ms -> ~494 ms. Measured warm launch -> first panadapter frame ~3.2 s -> ~2.3 s on a FLEX-8400M over LAN. Verified on hardware: connects, pipelined subs sent in a single burst, no "peek window closed" warning, spectrum streams, clean disconnect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Nice perf work, @jensenpat — the analysis is convincing and the perf math checks out. The "radio processes TCP commands in send order" invariant is already established by the existing second sub batch (sub tnf etc.), so generalising it to the first batch is a legitimate cleanup, not a behavior change.
A couple of things worth considering before merge:
1. m_multiFlexEnabled default is true (RadioModel.h:649) — small race window
The skip-peek fast path assumes m_multiFlexEnabled is correctly populated by the radio status burst from sub radio all before the sub client all callback runs. On TCP, in-order delivery makes this very likely on a LAN. But because the member defaults to true, the failure mode is silent: if for any reason the status hasn't been processed (lossy WAN-style timing, status dispatched on a worker thread that hasn't drained yet, future refactor that changes processing order), we skip conflict detection without noticing.
You already log a warning when m_clientInfoMap is empty after the wait — that diagnostic path is now gone on the fast path. Consider one of:
- Reset
m_multiFlexEnabledtofalse(or astd::optional<bool>) on disconnect so the default doesn't bias the fast path on a fresh connect, or - Gate the skip on a separate "mf_enable status received this session" flag rather than the cached value.
Probably fine in practice — but the silent-default is the only thing about this change I'd want belt-and-braces on, since a missed conflict means the previously-connected GUI gets booted without warning.
2. armClientConnectionNoticeSuppression() ordering
In the old nested code, suppression was armed inside the sub apd all R callback, after the radio had finished processing prior subs and before sub client all was sent. In the new pipelined code, all 9 subs are queued first, then suppression is armed, then sub client all is sent. Since send-ordering happens on the main thread before any response can arrive, this is still safe — but only because none of the earlier subs trigger client-connection notices. Worth a one-line comment noting why it's positioned where it is, since the new layout makes "why is this in the middle?" less obvious than the callback-nested version.
3. Indentation (nit)
The body below sendCmd("sub xvtr all"); retains its old nested indentation (8–12 columns deep) even though it's now at the top level of the client gui callback. Hard to read in context — a quick reflow to match the new control flow would help reviewers and future maintainers. Not a blocker.
Nothing else jumped out. AppSettings usage at line 2005 is preserved, no new resource lifetime concerns, and the mf-disabled / WAN paths are correctly noted as untouched. Thanks for the thorough PR description and the LAN-vs-WAN scope analysis — that made this much easier to reason about.
🤖 aethersdr-agent · cost: $10.9797 · model: claude-opus-4-7
NF0T
left a comment
There was a problem hiding this comment.
Good perf work, @jensenpat — the analysis is thorough and the claimed wins are honest (main-thread −21%, flat total CPU, clear note on what the QRhi backing-store cost will take to fix structurally). Change 2's flattening is a clean cleanup of gratuitous serialization, and the m_multiFlexContinuation move pattern in the fast path is handled correctly. A few things to address before merge.
1. m_multiFlexEnabled not reset in onDisconnected() — minor, one-line fix
m_multiFlexEnabled defaults to true (RadioModel.h:649) and is absent from the onDisconnected() state-reset block, despite that block carrying the comment: "Must re-derive everything from the new radio's status on next connect." Every other radio-capability flag cleared there — m_hasAmplifier, m_fullDuplex, m_maxSlices, m_model, etc. — gets zeroed; this one doesn't.
The practical race risk is low given TCP ordering (all S(radio) bytes precede R(client) in the stream, so mf_enable is in the main-thread queue before the inner callback fires), but the default true is a latent trap: if mf_enable status doesn't arrive for any reason — old firmware, future parsing refactor — the fast path silently skips conflict detection. The fix is one line in onDisconnected():
m_multiFlexEnabled = false;Aligns with both the documented intent of the reset block and the WAN path, which never enters this function at all.
2. Indentation not reflowed after flattening — minor
The continuation code after sendCmd("sub xvtr all"); retains its old 12-space nesting even though it's now at the top level of the client gui callback. The contrast between the flat new sub-batch and the deeply-indented body below it reads as if there's still nesting that isn't there. A reflow to match the surrounding level would help future readers.
3. armClientConnectionNoticeSuppression() position — nit
Its placement between sub apd all and sub client all was previously self-documenting from the callback structure. A one-liner noting that suppression must be armed before sub client all is sent preserves that intent in the flat layout.
Nothing here touches the perf logic itself — all three items are about making the implementation hold up to its own stated invariants. Happy to re-review promptly once addressed.
73,
Ryan NF0T
…mment Three items, all converging between @aethersdr-agent and @NF0T's reviews on PR aethersdr#3391: 1. Reset m_multiFlexEnabled to false in onDisconnected(). The default-true in RadioModel.h:649 was a latent trap for the new skip-peek fast path in registerAsGuiClient: if mf_enable status doesn't arrive for any reason (older firmware, future status-dispatch refactor, lossy pre-handshake state), the fast path silently bypasses conflict detection and the previously-connected GUI gets booted without warning. Adds one line in onDisconnected()'s capability-reset block alongside m_hasAmplifier, m_fullDuplex, m_maxSlices, etc. — matches the documented intent of that block ("re-derive everything from the new radio's status on next connect"). 2. Reflow indentation of the flattened sub batch from 4 to 8 spaces. The pipelining refactor at line 2027-2045 left the new lines at the old nested depth even though they're now at top level of the client gui lambda body. Visually misleading in context — surrounding code is at 8 spaces, so the new block read as if there was still nesting that wasn't there. 3. Comment on armClientConnectionNoticeSuppression() position. In the old nested-callback layout, its placement between sub apd all and sub client all was self-documenting from the callback structure. The flat layout makes "why is this in the middle?" less obvious. Added a block comment noting it must precede sub client all (which triggers the client-status burst we want to suppress notices for) and that earlier subs in this batch don't generate client-connection notices, so positioning here is safe. Perf logic itself untouched — same flatten + same fast-path skip, just with the safety/readability items the reviewers flagged. Principle XI.
|
@jensenpat — pushed a fix-up commit (9fc60c8) addressing all three items from @aethersdr-agent + @NF0T's reviews. Same pattern as the other recent fix-ups; happy to revert if you'd rather take a swing yourself. 1. 2. Reflowed indentation of the flattened sub batch from 4 to 8 spaces to match the surrounding lambda body. Visual cue now matches the actual nesting level. 3. Comment on Perf logic itself untouched. @NF0T — ready for a re-look against your earlier CHANGES_REQUESTED. (side note: @NF0T, your review's opening paragraph references "main-thread −21%" and "QRhi backing-store" — looks like that one accidentally got drafted on top of the Lean Mode PR #3286 notes. The three substantive items are all PR-3391-specific and correct, just the framing sentence was off.) |
…Mode Addresses NF0T's four CHANGES_REQUESTED items on aethersdr#3286 plus the three items raised in my 2026-06-05 coordination comment. NF0T blockers: 1. Per-widget MeterSmoother repaint gate (was shared-static). The previous shared-static gate let whichever meter ticked first each ~83 ms window through and starved the other N−1 meters that window. With 5 active meters (S-Meter + 3 client-side dynamics applets + comp strip) the effective per-meter cadence dropped from the intended 12 Hz to ~2.4 Hz — visibly stuttery on GR bars and the S-meter needle. MeterSmoother::shouldRepaint() is now an instance method carrying its own QElapsedTimer + lastMs; every active meter independently hits kLeanRepaintHz. SMeterWidget runs its own custom needle animation and never owned a MeterSmoother — it now holds one solely as the gate source so the throttle policy stays uniform across every meter. 2. Principle V — nested-JSON settings for Lean Mode. The flat AppSettings key "LeanMode" is replaced by a "Display" root object containing { "leanMode": "..." }. New header-only helper DisplaySettings (mirrors CwDecodeSettings.h's pattern) wraps the getter/setter and runs a one-shot migration on first launch that reads any legacy flat "LeanMode" key, writes it into the JSON root, and removes the flat key. MainWindow now reads via DisplaySettings::leanMode() and writes via DisplaySettings::setLeanMode(bool). 3. 60 Hz / 30 Hz comment inconsistency. The repaint cap was changed to ~30 Hz (kLeanFrameMs = 33) during the final tuning pass but four comments still said "60 Hz" — updated in MainWindow.cpp/.h, SpectrumWidget.cpp, and SpectrumWidget.h with the constant cross-referenced so future readers see one number. 4. WaveApplet activation round-trip respects pre-Lean state. Previously toggling Lean on disabled the WAVE scope and toggling Lean off unconditionally re-enabled it — clobbering a user who had already disabled WAVE manually. We now snapshot wave->isActive() into m_preLeanWaveActive on Lean entry and restore that value on Lean exit. WaveApplet exposes a new isActive() getter for the snapshot; its setActive() doc is expanded to clarify that the "drop sample feed" framing means appendScopeSamples short-circuits — the upstream AudioEngine::{tx,rx}PostChainScopeReady signal still fires per audio callback, so the savings are no-m_waveform-work + no-repaint, not signal suppression. This follows the same maintainer-fix-up pattern used on aethersdr#3279 (KISS TNC), aethersdr#3335 (Reboot Radio), aethersdr#3350, and aethersdr#3391 (perf connect): stale community PR, NF0T review with concrete blockers, maintainer pushes the addressing commits back to the author's fork to unblock. Principle XI.
NF0T
left a comment
There was a problem hiding this comment.
All three items from my 2026-06-05 CHANGES_REQUESTED are addressed in `9fc60c8b`.
Verified against the full PR diff.
(Side note: @ten9876 is correct — the opening paragraph of my earlier review
accidentally led with Lean Mode profiling figures from a draft I had open for
#3286. The three substantive items were all #3391-specific and correct; apologies
for the noise.)
Blocker 1 — `m_multiFlexEnabled = false` in `onDisconnected()` ✓
Added in the capability-reset block alongside `m_fullDuplex`, `m_hasAmplifier`,
etc., exactly where the "Must re-derive everything from the new radio's status
on next connect" comment says it belongs. WHY comment is correct and references
the review that caught it. Default-true trap is closed.
Blocker 2 — Indentation reflowed ✓
Full sub batch at 8-space indentation matching the actual nesting level inside
the `client gui` callback. Old closing comment trail (`}); // sub xvtr all`,
etc.) correctly removed — those callbacks no longer exist. The
`// Memory status arrives via normal status handler` comment also moved from
12-space to 8-space. Consistent throughout.
Blocker 3 — `armClientConnectionNoticeSuppression()` comment ✓
Comment is substantive: explains why suppression must precede `sub client all`,
why the earlier subs are safe, and why the comment is needed at all given the
flat layout no longer self-documents via callback nesting. Exactly right.
Nothing else jumped out in the full diff — single file, surgical change, move
pattern on the continuation is correct, pipelined sub batch matches the
existing second-batch pattern.
✅ Approving.
…ultiFLEX on; pipeline subs) (aethersdr#3391) ## What Two independent latency wins on the radio **connect handshake** — the dominant stage of time-to-first-panadapter-frame on a warm launch. No new dependencies, no protocol additions, ~33 lines net in `RadioModel.cpp`. ### 1. Skip the 400 ms multiFLEX conflict peek when `mf_enable=1` `peekForMultiFlexConflictThen` subscribes to radio/client topics, then waits a fixed `QTimer::singleShot(400)` for the client-status burst before sending `client gui`. But the conflict check is `!m_multiFlexEnabled && hasOthers` — when multiFLEX is **enabled**, the radio explicitly allows multiple GUI clients, so no conflict is possible and that 400 ms is pure dead time. `mf_enable` arrives in the radio status burst triggered by the preceding `sub radio all`, so `m_multiFlexEnabled` is already set when the callback runs; we proceed immediately. The mf-**disabled** path keeps the original 400 ms wait, so conflict detection is unchanged where it actually matters. `onConnected → first pan stream id`: **~851 ms → ~423 ms** ### 2. Pipeline the first subscription batch `sub slice all → … → sub xvtr all` was one nested `sendCmd` per command, each awaiting its `R` response — ~11 serial round trips (~0.7 s on a LAN). The radio processes TCP commands in send order, and the **second** sub batch in this same function (`sub tnf/dax/codec/…`) already fires without waiting, so the per-command serialization was gratuitous. Flattened to back-to-back fire-and-forget, matching the existing pattern. `first pan stream id → subscriptions ACKed`: **~985 ms → ~71 ms** ## Applies to LAN, remote IP, and SmartLink — and helps remote *more* The subscription pipelining (aethersdr#2) lives in `registerAsGuiClient`, which is shared by **every** connection type. `onConnected` reaches it from both branches — LAN via `peekForMultiFlexConflictThen(…)`, and remote IP / SmartLink (WAN) via `sendCmd("client ip", … registerAsGuiClient)` — and `sendCmd` routes to `m_wanConn->sendCommand()` over the SmartLink TLS tunnel when WAN is active, or the LAN socket otherwise. Same flattened subs on all paths. The win is "11 serial round-trips → one burst," so it scales with per-RTT latency: on LAN that's ~60 ms/RTT (~0.7 s saved), but over a routed/SmartLink link at 50–200 ms+/RTT the serial chain can cost 1.5–3 s, making the absolute saving **larger on remote flows**. Order-preserving pipelining is already proven for the WAN tunnel — the existing second sub batch fires-and-forget through this exact path. The peek skip (aethersdr#1) is **LAN-only** by construction: the WAN/SmartLink branch never runs the peek (it does a `client ip` round-trip instead, and multiFLEX conflicts on WAN are caught pre-connection via `licensedClients`). So aethersdr#1 is a no-op there — no benefit, no regression. ## Impact Combined handshake (**TCP connected → subscriptions ACKed**): **~1810 ms → ~494 ms** on LAN. Measured warm launch → first panadapter frame: **~3.2 s → ~2.3 s** on a FLEX-8400M over LAN. (A "warm" launch = consent already granted; a cold first launch is dominated by the macOS local-network/mic consent dialogs, which are one-time human interaction and not representative.) **Remote IP / SmartLink numbers are not yet measured** (tested on a LAN FLEX-8400M only); pipelining aethersdr#2 covers those paths and is expected to help at least as much, but the figures above are LAN-measured. ## How it was found Profiled the launch→connect→first-frame path with a temporary timeline instrument (kept out of this PR per request). The connect handshake was ~55% of warm startup; this PR addresses it. The remaining stages — UI init (~0.76 s) and the discovery broadcast wait (~0.85 s, bounded by the radio's ~1 Hz beacon) — are untouched here and are candidate follow-ups. ## Verification (on hardware) - Connects cleanly to a FLEX-8400M over LAN. - Pipelined subs are sent in a single burst (`C32…C42` same millisecond). - No `peek window closed with no client status received` warning. - Spectrum streams (`streamAck=yes`), panadapter renders, clean disconnect. - No subscription/protocol errors introduced. ## Risk Low. Both changes preserve command **send order** (which the radio honors) and the existing conflict-detection semantics. The mf-disabled connect path is byte-for-byte unchanged. 💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Codex <noreply@openai.com> Co-authored-by: Jeremy [KK7GWY] <kk7gwy@aethersdr.com>
What
Two independent latency wins on the radio connect handshake — the dominant stage of time-to-first-panadapter-frame on a warm launch. No new dependencies, no protocol additions, ~33 lines net in
RadioModel.cpp.1. Skip the 400 ms multiFLEX conflict peek when
mf_enable=1peekForMultiFlexConflictThensubscribes to radio/client topics, then waits a fixedQTimer::singleShot(400)for the client-status burst before sendingclient gui. But the conflict check is!m_multiFlexEnabled && hasOthers— when multiFLEX is enabled, the radio explicitly allows multiple GUI clients, so no conflict is possible and that 400 ms is pure dead time.mf_enablearrives in the radio status burst triggered by the precedingsub radio all, som_multiFlexEnabledis already set when the callback runs; we proceed immediately. The mf-disabled path keeps the original 400 ms wait, so conflict detection is unchanged where it actually matters.onConnected → first pan stream id: ~851 ms → ~423 ms2. Pipeline the first subscription batch
sub slice all → … → sub xvtr allwas one nestedsendCmdper command, each awaiting itsRresponse — ~11 serial round trips (~0.7 s on a LAN). The radio processes TCP commands in send order, and the second sub batch in this same function (sub tnf/dax/codec/…) already fires without waiting, so the per-command serialization was gratuitous. Flattened to back-to-back fire-and-forget, matching the existing pattern.first pan stream id → subscriptions ACKed: ~985 ms → ~71 msApplies to LAN, remote IP, and SmartLink — and helps remote more
The subscription pipelining (#2) lives in
registerAsGuiClient, which is shared by every connection type.onConnectedreaches it from both branches — LAN viapeekForMultiFlexConflictThen(…), and remote IP / SmartLink (WAN) viasendCmd("client ip", … registerAsGuiClient)— andsendCmdroutes tom_wanConn->sendCommand()over the SmartLink TLS tunnel when WAN is active, or the LAN socket otherwise. Same flattened subs on all paths.The win is "11 serial round-trips → one burst," so it scales with per-RTT latency: on LAN that's ~60 ms/RTT (~0.7 s saved), but over a routed/SmartLink link at 50–200 ms+/RTT the serial chain can cost 1.5–3 s, making the absolute saving larger on remote flows. Order-preserving pipelining is already proven for the WAN tunnel — the existing second sub batch fires-and-forget through this exact path.
The peek skip (#1) is LAN-only by construction: the WAN/SmartLink branch never runs the peek (it does a
client ipround-trip instead, and multiFLEX conflicts on WAN are caught pre-connection vialicensedClients). So #1 is a no-op there — no benefit, no regression.Impact
Combined handshake (TCP connected → subscriptions ACKed): ~1810 ms → ~494 ms on LAN.
Measured warm launch → first panadapter frame: ~3.2 s → ~2.3 s on a FLEX-8400M over LAN. (A "warm" launch = consent already granted; a cold first launch is dominated by the macOS local-network/mic consent dialogs, which are one-time human interaction and not representative.)
Remote IP / SmartLink numbers are not yet measured (tested on a LAN FLEX-8400M only); pipelining #2 covers those paths and is expected to help at least as much, but the figures above are LAN-measured.
How it was found
Profiled the launch→connect→first-frame path with a temporary timeline instrument (kept out of this PR per request). The connect handshake was ~55% of warm startup; this PR addresses it. The remaining stages — UI init (~0.76 s) and the discovery broadcast wait (~0.85 s, bounded by the radio's ~1 Hz beacon) — are untouched here and are candidate follow-ups.
Verification (on hardware)
C32…C42same millisecond).peek window closed with no client status receivedwarning.streamAck=yes), panadapter renders, clean disconnect.Risk
Low. Both changes preserve command send order (which the radio honors) and the existing conflict-detection semantics. The mf-disabled connect path is byte-for-byte unchanged.
💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat
🤖 Generated with Claude Code